[BackgroundSync] Only allow SyncManager.register to occur from main frame

Only allow SyncManager.register to occur from main frame

This CL limits registrations of background syncs to:

1) Main frame documents
2) Service Workers with a main frame window client

The main frame document check occurs in SyncManager.cpp

The window client check occurs in BackgroundSyncManager.cpp.

There are browser tests to verify the functionality. I've removed
spurious unittests from background_sync_manager_service_impl.cc.
There is not currently a unittest of the ServiceWorkerContextCore
change, due to the difficulty of unit testing the render frame
host (mocking it out seemed pointless).

BUG=558388
Committed: https://crrev.com/0cafe7ecc2c99f9cd7c9392f56814eb83d7eb63d
Cr-Commit-Position: refs/heads/master@{#361707}

[jkarlin, 2015-11-23 18:04:11]

Description was changed from

==========
Only allow SyncManager.register to occur from main frame

This CL limits registrations of background syncs to:

1) Main frame documents
2) Service Workers with a main frame window client

BUG=558388
==========

to

==========
Only allow SyncManager.register to occur from main frame

This CL limits registrations of background syncs to:

1) Main frame documents
2) Service Workers with a main frame window client

The main frame document check occurs in SyncManager.cpp

The window client check occurs in BackgroundSyncManager.cpp.

There are browser tests to verify the functionality. I've removed spurious
unittests from background_sync_manager_service_impl.cc There is not currently a
unittest of the ServiceWorkerContextCore change, due to the difficulty of unit
testing the render frame host (and mocking it out seemed pointless).

BUG=558388
==========

[jkarlin, 2015-11-23 18:04:52]

Description was changed from

==========
Only allow SyncManager.register to occur from main frame

This CL limits registrations of background syncs to:

1) Main frame documents
2) Service Workers with a main frame window client

The main frame document check occurs in SyncManager.cpp

The window client check occurs in BackgroundSyncManager.cpp.

There are browser tests to verify the functionality. I've removed spurious
unittests from background_sync_manager_service_impl.cc There is not currently a
unittest of the ServiceWorkerContextCore change, due to the difficulty of unit
testing the render frame host (and mocking it out seemed pointless).

BUG=558388
==========

to

==========
Only allow SyncManager.register to occur from main frame

This CL limits registrations of background syncs to:

1) Main frame documents
2) Service Workers with a main frame window client

The main frame document check occurs in SyncManager.cpp

The window client check occurs in BackgroundSyncManager.cpp.

There are browser tests to verify the functionality. I've removed
spurious unittests from background_sync_manager_service_impl.cc.
There is not currently a unittest of the ServiceWorkerContextCore
change, due to the difficulty of unit testing the render frame
host (mocking it out seemed pointless).

BUG=558388
==========

[jkarlin, 2015-11-23 18:06:00]

jkarlin@chromium.org changed reviewers:
+ iclelland@chromium.org, michaeln@chromium.org

[jkarlin, 2015-11-23 18:06:01]

iclelland: PTAL at everything
michaeln: PTAL at service_worker code

Many thanks!

[jkarlin, 2015-11-24 16:52:37]

Ping!

[michaeln, 2015-11-25 01:09:46]

lgtm

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

https://codereview.chromium.org/1471763003/diff/80001/content/browser/backgro...
content/browser/background_sync/background_sync_manager_unittest.cc:1690:
test_background_sync_manager_->set_has_main_frame_provider_host(false);
nice simplification!

https://codereview.chromium.org/1471763003/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/1471763003/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_context_core.cc:314:
scoped_ptr<std::vector<std::pair<int, int>>> render_frames(
maybe move local heap alloc down after line 321

[jkarlin, 2015-11-25 13:15:08]

Thanks Michael.

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

https://codereview.chromium.org/1471763003/diff/80001/content/browser/backgro...
content/browser/background_sync/background_sync_manager_unittest.cc:1690:
test_background_sync_manager_->set_has_main_frame_provider_host(false);
On 2015/11/25 01:09:46, michaeln wrote:
> nice simplification!

Thanks!

https://codereview.chromium.org/1471763003/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/1471763003/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_context_core.cc:314:
scoped_ptr<std::vector<std::pair<int, int>>> render_frames(
On 2015/11/25 01:09:46, michaeln wrote:
> maybe move local heap alloc down after line 321

Done.

[iclelland, 2015-11-25 14:58:16]

https://codereview.chromium.org/1471763003/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1471763003/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:403: void
BackgroundSyncManager::RegisterCheckIfMainFrame(
I know it makes it a bit longer, but I'd prefer something like
`RegisterCheckIfHasMainFrame`, so it doesn't sound like it could be "Check if
*is* main frame"

https://codereview.chromium.org/1471763003/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/1471763003/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:70:
scoped_ptr<std::vector<std::pair<int, int>>> frames) {
Can we document what this method does, and what the `frames` data structure
should contain?

https://codereview.chromium.org/1471763003/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:315:
base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
Why is this callback task posted differently (through base::TTRH::Get) than in
the code below (which uses BrowserThread::UI explicitly) ?

[jkarlin, 2015-11-25 16:47:25]

Thanks for the review, PTAL!

https://codereview.chromium.org/1471763003/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1471763003/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:403: void
BackgroundSyncManager::RegisterCheckIfMainFrame(
On 2015/11/25 14:58:16, iclelland wrote:
> I know it makes it a bit longer, but I'd prefer something like
> `RegisterCheckIfHasMainFrame`, so it doesn't sound like it could be "Check if
> *is* main frame"

Done.

https://codereview.chromium.org/1471763003/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/1471763003/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:70:
scoped_ptr<std::vector<std::pair<int, int>>> frames) {
On 2015/11/25 14:58:16, iclelland wrote:
> Can we document what this method does, and what the `frames` data structure
> should contain?

Done.

https://codereview.chromium.org/1471763003/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:315:
base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
On 2015/11/25 14:58:16, iclelland wrote:
> Why is this callback task posted differently (through base::TTRH::Get) than in
> the code below (which uses BrowserThread::UI explicitly) ?

It's one less line of code that needs to specify a thread identifier. I see that
as preferable.

[iclelland, 2015-11-25 17:06:03]

lgtm

https://codereview.chromium.org/1471763003/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/1471763003/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_context_core.cc:315:
base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
On 2015/11/25 16:47:25, jkarlin wrote:
> On 2015/11/25 14:58:16, iclelland wrote:
> > Why is this callback task posted differently (through base::TTRH::Get) than
in
> > the code below (which uses BrowserThread::UI explicitly) ?
> 
> It's one less line of code that needs to specify a thread identifier. I see
that
> as preferable.

OK - I misread the intent of the code, and thought that the callback was
supposed to be called on the UI thread, where it's actually just the
FrameListContainsMainFrameOnUI method that is.

Looks good, thanks.

[jkarlin, 2015-11-25 17:42:00]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2015-11-25 17:42:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/1471763003/#ps120001
(title: "Address comments from PS6")

[commit-bot: I haz the power, 2015-11-25 17:43:04]

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

[commit-bot: I haz the power, 2015-11-25 18:42:23]

Description was changed from

==========
Only allow SyncManager.register to occur from main frame

This CL limits registrations of background syncs to:

1) Main frame documents
2) Service Workers with a main frame window client

The main frame document check occurs in SyncManager.cpp

The window client check occurs in BackgroundSyncManager.cpp.

There are browser tests to verify the functionality. I've removed
spurious unittests from background_sync_manager_service_impl.cc.
There is not currently a unittest of the ServiceWorkerContextCore
change, due to the difficulty of unit testing the render frame
host (mocking it out seemed pointless).

BUG=558388
==========

to

==========
Only allow SyncManager.register to occur from main frame

This CL limits registrations of background syncs to:

1) Main frame documents
2) Service Workers with a main frame window client

The main frame document check occurs in SyncManager.cpp

The window client check occurs in BackgroundSyncManager.cpp.

There are browser tests to verify the functionality. I've removed
spurious unittests from background_sync_manager_service_impl.cc.
There is not currently a unittest of the ServiceWorkerContextCore
change, due to the difficulty of unit testing the render frame
host (mocking it out seemed pointless).

BUG=558388
==========

[commit-bot: I haz the power, 2015-11-25 18:42:23]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-25 18:42:57]

Description was changed from

==========
Only allow SyncManager.register to occur from main frame

This CL limits registrations of background syncs to:

1) Main frame documents
2) Service Workers with a main frame window client

The main frame document check occurs in SyncManager.cpp

The window client check occurs in BackgroundSyncManager.cpp.

There are browser tests to verify the functionality. I've removed
spurious unittests from background_sync_manager_service_impl.cc.
There is not currently a unittest of the ServiceWorkerContextCore
change, due to the difficulty of unit testing the render frame
host (mocking it out seemed pointless).

BUG=558388
==========

to

==========
Only allow SyncManager.register to occur from main frame

This CL limits registrations of background syncs to:

1) Main frame documents
2) Service Workers with a main frame window client

The main frame document check occurs in SyncManager.cpp

The window client check occurs in BackgroundSyncManager.cpp.

There are browser tests to verify the functionality. I've removed
spurious unittests from background_sync_manager_service_impl.cc.
There is not currently a unittest of the ServiceWorkerContextCore
change, due to the difficulty of unit testing the render frame
host (mocking it out seemed pointless).

BUG=558388

Committed: https://crrev.com/0cafe7ecc2c99f9cd7c9392f56814eb83d7eb63d
Cr-Commit-Position: refs/heads/master@{#361707}
==========

[commit-bot: I haz the power, 2015-11-25 18:42:58]

Patchset 7 (id:??) landed as
https://crrev.com/0cafe7ecc2c99f9cd7c9392f56814eb83d7eb63d
Cr-Commit-Position: refs/heads/master@{#361707}