Moved TestBackgroundSyncController to MockBackgroundSyncController

Moved TestBackgroundSyncController to MockBackgroundSyncController

This is the first in a series of patches to add a content setting to
Background Sync.

In particular, this change will enable adding a method to
BackgroundSyncController that returns a permission status and mocking
it for tests. We can't just add a method to the controller that
returns a granted permission - this would be risky, but we do want to
control permission during tests.

Original, 'big picture' patch: crrev.com/1673303002

TEST=Existing background sync tests don't break.
BUG=564052
Committed: https://crrev.com/db7b65c645901b995ca15cead932ebb4d04f83fa
Cr-Commit-Position: refs/heads/master@{#375646}

[Nina, 2016-02-12 16:24:20]

[email protected] changed reviewers:
+ [email protected]

[Nina, 2016-02-12 16:24:20]

[email protected] changed required reviewers:
+ [email protected]

[Nina, 2016-02-12 16:24:21]

PTAL :)

[Peter Beverloo, 2016-02-12 16:38:23]

Thanks! Could you explain why these changes are necessary? (Ideally in the CL
description.)

https://codereview.chromium.org/1695503004/diff/1/content/public/test/mock_ba...
File content/public/test/mock_background_sync_controller.cc (right):

https://codereview.chromium.org/1695503004/diff/1/content/public/test/mock_ba...
content/public/test/mock_background_sync_controller.cc:14: void
MockBackgroundSyncController::RunInBackground(bool enabled,
nit: newline between the methods (also on line 19/20).

https://codereview.chromium.org/1695503004/diff/1/content/public/test/mock_ba...
File content/public/test/mock_background_sync_controller.h (right):

https://codereview.chromium.org/1695503004/diff/1/content/public/test/mock_ba...
content/public/test/mock_background_sync_controller.h:6: #define
CONTENT_PUBLIC_TEST_MOCK_BACKGROUND_SYNC_CONTROLLER_H_
Looking at the usages in your other patch, I don't think this has to be in the
public API since they're all in //content. You could place it in //content/test/
instead.

https://codereview.chromium.org/1695503004/diff/1/content/public/test/mock_ba...
content/public/test/mock_background_sync_controller.h:11: 
IWYU:

+#include <stdint.h>
+#include "url/gurl.h"

https://codereview.chromium.org/1695503004/diff/1/content/public/test/mock_ba...
content/public/test/mock_background_sync_controller.h:14: class CONTENT_EXPORT
MockBackgroundSyncController
Could you add a brief one-line class-level comment about what it does? (I.e.
counting and tracking state.)

https://codereview.chromium.org/1695503004/diff/1/content/public/test/test_br...
File content/public/test/test_browser_context.cc (right):

https://codereview.chromium.org/1695503004/diff/1/content/public/test/test_br...
content/public/test/test_browser_context.cc:147:
background_sync_controller_.reset(new MockBackgroundSyncController());
Why are the changes in this (and the other browser_context files) significant?
If there is a reason, it would be good to detail that in the CL's description.

[Nina, 2016-02-15 09:16:58]

Description was changed from

==========
Moved TestBackgroundSyncController to MockBackgroundSyncController

This is the first in a series of patches to add a content setting to
Background Sync.

TEST=Existing background sync tests don't break.
BUG=564052
==========

to

==========
Moved TestBackgroundSyncController to MockBackgroundSyncController

This is the first in a series of patches to add a content setting to
Background Sync. In particular, this change will enable adding a
method to BackgroundSyncController that returns a permission status
and mocking it for tests.

Original, 'big picture' patch: crrev.com/1673303002

TEST=Existing background sync tests don't break.
BUG=564052
==========

[Nina, 2016-02-15 09:32:23]

Description was changed from

==========
Moved TestBackgroundSyncController to MockBackgroundSyncController

This is the first in a series of patches to add a content setting to
Background Sync. In particular, this change will enable adding a
method to BackgroundSyncController that returns a permission status
and mocking it for tests.

Original, 'big picture' patch: crrev.com/1673303002

TEST=Existing background sync tests don't break.
BUG=564052
==========

to

==========
Moved TestBackgroundSyncController to MockBackgroundSyncController

This is the first in a series of patches to add a content setting to
Background Sync.

In particular, this change will enable adding a method to
BackgroundSyncController that returns a permission status and mocking
it for tests. We can't just add a method to the controller that
returns a granted permission - this would be risky, but we do want to
control permission during tests.

Original, 'big picture' patch: crrev.com/1673303002

TEST=Existing background sync tests don't break.
BUG=564052
==========

[Nina, 2016-02-15 09:48:48]

[email protected] changed reviewers:
+ [email protected]

[Nina, 2016-02-15 09:48:48]

[email protected] changed required reviewers:
+ [email protected]

[Nina, 2016-02-15 09:48:49]

Peter, Josh, fixed comments, please take a look again.

https://codereview.chromium.org/1695503004/diff/1/content/public/test/mock_ba...
File content/public/test/mock_background_sync_controller.cc (right):

https://codereview.chromium.org/1695503004/diff/1/content/public/test/mock_ba...
content/public/test/mock_background_sync_controller.cc:14: void
MockBackgroundSyncController::RunInBackground(bool enabled,
On 2016/02/12 16:38:23, Peter Beverloo wrote:
> nit: newline between the methods (also on line 19/20).

Done.

https://codereview.chromium.org/1695503004/diff/1/content/public/test/mock_ba...
File content/public/test/mock_background_sync_controller.h (right):

https://codereview.chromium.org/1695503004/diff/1/content/public/test/mock_ba...
content/public/test/mock_background_sync_controller.h:6: #define
CONTENT_PUBLIC_TEST_MOCK_BACKGROUND_SYNC_CONTROLLER_H_
On 2016/02/12 16:38:23, Peter Beverloo wrote:
> Looking at the usages in your other patch, I don't think this has to be in the
> public API since they're all in //content. You could place it in
//content/test/
> instead.

Done.

https://codereview.chromium.org/1695503004/diff/1/content/public/test/mock_ba...
content/public/test/mock_background_sync_controller.h:11: 
On 2016/02/12 16:38:23, Peter Beverloo wrote:
> IWYU:
> 
> +#include <stdint.h>
> +#include "url/gurl.h"

Done.

https://codereview.chromium.org/1695503004/diff/1/content/public/test/mock_ba...
content/public/test/mock_background_sync_controller.h:14: class CONTENT_EXPORT
MockBackgroundSyncController
On 2016/02/12 16:38:23, Peter Beverloo wrote:
> Could you add a brief one-line class-level comment about what it does? (I.e.
> counting and tracking state.)

Done.

https://codereview.chromium.org/1695503004/diff/1/content/public/test/test_br...
File content/public/test/test_browser_context.cc (right):

https://codereview.chromium.org/1695503004/diff/1/content/public/test/test_br...
content/public/test/test_browser_context.cc:147:
background_sync_controller_.reset(new MockBackgroundSyncController());
On 2016/02/12 16:38:23, Peter Beverloo wrote:
> Why are the changes in this (and the other browser_context files) significant?
> If there is a reason, it would be good to detail that in the CL's description.

We'll need these in a future patch to provide a default, safe implementation to
BackgroundSyncController::GetPermission(). Updated CL description.

[Peter Beverloo, 2016-02-16 00:59:00]

Thanks! I'm OK with this, modulo one comment.

https://codereview.chromium.org/1695503004/diff/20001/content/browser/backgro...
File content/browser/background_sync/background_sync_manager_unittest.cc
(right):

https://codereview.chromium.org/1695503004/diff/20001/content/browser/backgro...
content/browser/background_sync/background_sync_manager_unittest.cc:265:
scoped_ptr<MockBackgroundSyncController> background_sync_controller(
We can drop this machinery (lines 265-269) completely from this file. The
helper_ (EmbeddedWorkerTestHelper) owns a TestBrowserContext, which you're
changing to have a MockBackgroundSyncController. That makes all of this
redundant.

Given that this is the only call to
TestBrowserContext::SetBackgroundSyncController(), that method can go as well
:-). May want to keep an accessor to the controller for the tests.

[Nina, 2016-02-16 11:14:51]

Done!

https://codereview.chromium.org/1695503004/diff/20001/content/browser/backgro...
File content/browser/background_sync/background_sync_manager_unittest.cc
(right):

https://codereview.chromium.org/1695503004/diff/20001/content/browser/backgro...
content/browser/background_sync/background_sync_manager_unittest.cc:265:
scoped_ptr<MockBackgroundSyncController> background_sync_controller(
On 2016/02/16 00:59:00, Peter Beverloo wrote:
> We can drop this machinery (lines 265-269) completely from this file. The
> helper_ (EmbeddedWorkerTestHelper) owns a TestBrowserContext, which you're
> changing to have a MockBackgroundSyncController. That makes all of this
> redundant.
> 
> Given that this is the only call to
> TestBrowserContext::SetBackgroundSyncController(), that method can go as well
> :-). May want to keep an accessor to the controller for the tests.

Good catch. Glad we can remove this code.

[Peter Beverloo, 2016-02-16 11:24:27]

lgtm

https://codereview.chromium.org/1695503004/diff/40001/content/public/test/tes...
File content/public/test/test_browser_context.h (right):

https://codereview.chromium.org/1695503004/diff/40001/content/public/test/tes...
content/public/test/test_browser_context.h:17: class
MockBackgroundSyncController;
nit: blank line after the namespace declaration. (Wasn't there before either,
but it should've been :-).)

https://codereview.chromium.org/1695503004/diff/40001/content/test/mock_backg...
File content/test/mock_background_sync_controller.h (right):

https://codereview.chromium.org/1695503004/diff/40001/content/test/mock_backg...
content/test/mock_background_sync_controller.h:48:
DISALLOW_COPY_AND_ASSIGN(MockBackgroundSyncController);
#include "base/macros.h"

[Nina, 2016-02-16 11:32:53]

jkarlin@, mind taking a look?

https://codereview.chromium.org/1695503004/diff/40001/content/public/test/tes...
File content/public/test/test_browser_context.h (right):

https://codereview.chromium.org/1695503004/diff/40001/content/public/test/tes...
content/public/test/test_browser_context.h:17: class
MockBackgroundSyncController;
On 2016/02/16 11:24:26, Peter Beverloo wrote:
> nit: blank line after the namespace declaration. (Wasn't there before either,
> but it should've been :-).)

Done. git cl lint should catch this :P

https://codereview.chromium.org/1695503004/diff/40001/content/test/mock_backg...
File content/test/mock_background_sync_controller.h (right):

https://codereview.chromium.org/1695503004/diff/40001/content/test/mock_backg...
content/test/mock_background_sync_controller.h:48:
DISALLOW_COPY_AND_ASSIGN(MockBackgroundSyncController);
On 2016/02/16 11:24:26, Peter Beverloo wrote:
> #include "base/macros.h"

Done.

[jkarlin, 2016-02-16 15:09:41]

lgtm

[Nina, 2016-02-16 15:26:46]

[email protected] changed reviewers:
+ [email protected]

[Nina, 2016-02-16 15:26:46]

[email protected] changed required reviewers:
+ [email protected]

[Nina, 2016-02-16 15:26:47]

avi, please review:

* content/public/test/
* content/test/

Thanks!

[Avi (use Gerrit), 2016-02-16 16:05:35]

lgtm

Those files 👍

[Nina, 2016-02-16 18:55:16]

The CQ bit was checked by [email protected]

[Nina, 2016-02-16 18:55:17]

The patchset sent to the CQ was uploaded after l-g-t-m from [email protected],
[email protected], [email protected]
Link to the patchset: https://codereview.chromium.org/1695503004/#ps100001
(title: "Trying to fix win compile again")

[commit-bot: I haz the power, 2016-02-16 18:56:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1695503004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1695503004/100001

[commit-bot: I haz the power, 2016-02-16 20:14:09]

Description was changed from

==========
Moved TestBackgroundSyncController to MockBackgroundSyncController

This is the first in a series of patches to add a content setting to
Background Sync.

In particular, this change will enable adding a method to
BackgroundSyncController that returns a permission status and mocking
it for tests. We can't just add a method to the controller that
returns a granted permission - this would be risky, but we do want to
control permission during tests.

Original, 'big picture' patch: crrev.com/1673303002

TEST=Existing background sync tests don't break.
BUG=564052
==========

to

==========
Moved TestBackgroundSyncController to MockBackgroundSyncController

This is the first in a series of patches to add a content setting to
Background Sync.

In particular, this change will enable adding a method to
BackgroundSyncController that returns a permission status and mocking
it for tests. We can't just add a method to the controller that
returns a granted permission - this would be risky, but we do want to
control permission during tests.

Original, 'big picture' patch: crrev.com/1673303002

TEST=Existing background sync tests don't break.
BUG=564052
==========

[commit-bot: I haz the power, 2016-02-16 20:14:10]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-16 22:54:31]

Description was changed from

==========
Moved TestBackgroundSyncController to MockBackgroundSyncController

This is the first in a series of patches to add a content setting to
Background Sync.

In particular, this change will enable adding a method to
BackgroundSyncController that returns a permission status and mocking
it for tests. We can't just add a method to the controller that
returns a granted permission - this would be risky, but we do want to
control permission during tests.

Original, 'big picture' patch: crrev.com/1673303002

TEST=Existing background sync tests don't break.
BUG=564052
==========

to

==========
Moved TestBackgroundSyncController to MockBackgroundSyncController

This is the first in a series of patches to add a content setting to
Background Sync.

In particular, this change will enable adding a method to
BackgroundSyncController that returns a permission status and mocking
it for tests. We can't just add a method to the controller that
returns a granted permission - this would be risky, but we do want to
control permission during tests.

Original, 'big picture' patch: crrev.com/1673303002

TEST=Existing background sync tests don't break.
BUG=564052

Committed: https://crrev.com/db7b65c645901b995ca15cead932ebb4d04f83fa
Cr-Commit-Position: refs/heads/master@{#375646}
==========

[commit-bot: I haz the power, 2016-02-16 22:54:32]

Patchset 6 (id:??) landed as
https://crrev.com/db7b65c645901b995ca15cead932ebb4d04f83fa
Cr-Commit-Position: refs/heads/master@{#375646}