PlzNavigate: Fix ServiceWorkerHandler not finding WebContents.

PlzNavigate: Fix ServiceWorkerHandler not finding WebContents.

With PlzNavigate, the RenderFrameHost associated with a navigation is
known only at commit time. For this reason, in
ServiceWorkerHandler::OnWorkerVersionUpdated(), client's process_id(-1)
and route_id(-2) are invalid. They will be set to something meaningful
as soon as ServiceWorkerProviderHost::FinalizeInitialization will be
called with PlzNavigate, but it's too late.

The solution with PlzNavigate is to use a WebContentsGetter instead. In
this way, there is no need to know the RenderFrameHost.

BUG=725818
TEST=http/tests/inspector/service-workers/service-workers-redundant.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel

Review-Url: https://codereview.chromium.org/2917643002
Cr-Commit-Position: refs/heads/master@{#476331}
Committed: https://chromium.googlesource.com/chromium/src/+/a7dc4e822a4cddc14b9929a6f7aa2a85e69252d8

[arthursonzogni, 2017-05-31 11:53:38]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 11:53:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[arthursonzogni, 2017-05-31 13:52:07]

Description was changed from

==========
PlzNavigate: Fix ServiceWorkerHandler not finding WebContents.

In void ServiceWorkerHandler::OnWorkerVersionUpdated(), client's
process_id(-1) and route_id(-2) and invalid. They will be set to
something meaningful as soon as
ServiceWorkerProviderHost::FinalizeInitialization will be called with
PlzNavigate.

The solution with PlzNavigate is to have a WebContentsGetter here. It
will provide the WebContents associated with the navigation without
having to determine what is the associated RenderFrameHost (only known
at navigation commit time).

BUG=725818
TEST=http/tests/inspector/service-workers/service-workers-redundant.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
==========

to

==========
PlzNavigate: Fix ServiceWorkerHandler not finding WebContents.

With PlzNavigate, the RenderFrameHost associated with a navigation is
known only at commit time. For this reason, in
ServiceWorkerHandler::OnWorkerVersionUpdated(), client's process_id(-1)
and route_id(-2) are invalid. They will be set to something meaningful
as soon as ServiceWorkerProviderHost::FinalizeInitialization will be
called with PlzNavigate, but it's too late.

The solution with PlzNavigate is to use a WebContentsGetter instead. In
this way, there is no need to know the RenderFrameHost.

BUG=725818
TEST=http/tests/inspector/service-workers/service-workers-redundant.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
==========

[arthursonzogni, 2017-05-31 13:57:48]

arthursonzogni@chromium.org changed reviewers:
+ falken@chromium.org

[arthursonzogni, 2017-05-31 13:57:49]

Hi falken@,
Could you please review this?

+CC clamy@, nasko@

[commit-bot: I haz the power, 2017-05-31 14:06:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 14:06:13]

Dry run: This issue passed the CQ dry run.

[falken, 2017-06-01 07:37:27]

lgtm, thanks for fixing this test!

https://codereview.chromium.org/2917643002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_observer.h (right):

https://codereview.chromium.org/2917643002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_observer.h:75: const
ConsoleMessage& message) {}
Should we say:

// |web_contents_getter| is only set in PlzNavigate.

https://codereview.chromium.org/2917643002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_info.h (right):

https://codereview.chromium.org/2917643002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_info.h:35: int route_id;
Does it make sense to say:

// |web_contents_getter_ is only set for PlzNavigate.
base::Callback<WebContents*(void)> web_contents_getter;

[arthursonzogni, 2017-06-01 08:06:54]

The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 08:07:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[arthursonzogni, 2017-06-01 08:14:08]

Thanks falken@!

https://codereview.chromium.org/2917643002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_observer.h (right):

https://codereview.chromium.org/2917643002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_observer.h:75: const
ConsoleMessage& message) {}
On 2017/06/01 07:37:27, falken wrote:
> Should we say:
> 
> // |web_contents_getter| is only set in PlzNavigate.

Done.

https://codereview.chromium.org/2917643002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_info.h (right):

https://codereview.chromium.org/2917643002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_info.h:35: int route_id;
On 2017/06/01 07:37:27, falken wrote:
> Does it make sense to say:
> 
> // |web_contents_getter_ is only set for PlzNavigate.
> base::Callback<WebContents*(void)> web_contents_getter;

Done.

[commit-bot: I haz the power, 2017-06-01 09:50:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 09:50:37]

Dry run: This issue passed the CQ dry run.

[arthursonzogni, 2017-06-01 10:47:08]

arthursonzogni@chromium.org changed reviewers:
+ clamy@chromium.org

[arthursonzogni, 2017-06-01 10:47:09]

Hi Camille,
I still need an OWNER for
content/browser/devtools/protocol/service_worker_handler.cc.
Please, do you think you could review it?

[clamy, 2017-06-01 14:44:56]

lgtm

[arthursonzogni, 2017-06-01 14:45:16]

The CQ bit was checked by arthursonzogni@chromium.org

[arthursonzogni, 2017-06-01 14:45:17]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2917643002/#ps40001
(title: "Shorter comment.")

[commit-bot: I haz the power, 2017-06-01 14:45:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 17:06:28]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496328316791580,
"parent_rev": "b239b006f7b5f342e2eabbace269d59cdb0cf2e3", "commit_rev":
"a7dc4e822a4cddc14b9929a6f7aa2a85e69252d8"}

[commit-bot: I haz the power, 2017-06-01 17:06:37]

Description was changed from

==========
PlzNavigate: Fix ServiceWorkerHandler not finding WebContents.

With PlzNavigate, the RenderFrameHost associated with a navigation is
known only at commit time. For this reason, in
ServiceWorkerHandler::OnWorkerVersionUpdated(), client's process_id(-1)
and route_id(-2) are invalid. They will be set to something meaningful
as soon as ServiceWorkerProviderHost::FinalizeInitialization will be
called with PlzNavigate, but it's too late.

The solution with PlzNavigate is to use a WebContentsGetter instead. In
this way, there is no need to know the RenderFrameHost.

BUG=725818
TEST=http/tests/inspector/service-workers/service-workers-redundant.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
==========

to

==========
PlzNavigate: Fix ServiceWorkerHandler not finding WebContents.

With PlzNavigate, the RenderFrameHost associated with a navigation is
known only at commit time. For this reason, in
ServiceWorkerHandler::OnWorkerVersionUpdated(), client's process_id(-1)
and route_id(-2) are invalid. They will be set to something meaningful
as soon as ServiceWorkerProviderHost::FinalizeInitialization will be
called with PlzNavigate, but it's too late.

The solution with PlzNavigate is to use a WebContentsGetter instead. In
this way, there is no need to know the RenderFrameHost.

BUG=725818
TEST=http/tests/inspector/service-workers/service-workers-redundant.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel

Review-Url: https://codereview.chromium.org/2917643002
Cr-Commit-Position: refs/heads/master@{#476331}
Committed:
https://chromium.googlesource.com/chromium/src/+/a7dc4e822a4cddc14b9929a6f7aa...
==========

[commit-bot: I haz the power, 2017-06-01 17:06:39]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/a7dc4e822a4cddc14b9929a6f7aa...