Pass the RenderProcessHost id on retargeting.

Pass the RenderProcessHost id on retargeting.

In cases of out-of-process iframes, RetargetingDetails doesn't currently
contain enough information to find the correct RenderFrameHost.
This CL adds the RenderProcessHost id in the RetargetingDetails struct
to allow consumers of it to correctly discover RenderFrameHost that
created the new WebContents.

BUG=649855
Committed: https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf
Cr-Commit-Position: refs/heads/master@{#422621}

[nasko, 2016-10-03 21:44:43]

nasko@chromium.org changed reviewers:
+ creis@chromium.org

[nasko, 2016-10-03 21:44:43]

Hey Charlie,
Can you review this CL for me? It passes the RPH id through RetargetingDetails,
so the consumers of it can find the correct RFH.
Thanks in advance!
Nasko

[nasko, 2016-10-03 21:44:48]

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

[commit-bot: I haz the power, 2016-10-03 21:45:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dgozman, 2016-10-03 22:03:48]

dgozman@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgozman, 2016-10-03 22:03:49]

devtools lgtm

[lazyboy, 2016-10-03 22:06:58]

lazyboy@chromium.org changed reviewers:
+ lazyboy@chromium.org

[lazyboy, 2016-10-03 22:06:58]

web_view_guest.* lgtm.

[commit-bot: I haz the power, 2016-10-03 22:07:48]

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

[commit-bot: I haz the power, 2016-10-03 22:07:49]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Charlie Reis, 2016-10-03 22:15:24]

Thanks!  LGTM with a few nits, and a possible Android issue.

https://codereview.chromium.org/2385363002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
(right):

https://codereview.chromium.org/2385363002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:537: for
(const auto& frame : frames) {
This is a bit odd at first glance.  Maybe clarify in a comment that we want to
grab the first (and only?) frame?  Would it be clearer to go straight to
frames[1] after verifying that it exists?

https://codereview.chromium.org/2385363002/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/webnavigation/userAction/test_userAction.js
(right):

https://codereview.chromium.org/2385363002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webnavigation/userAction/test_userAction.js:24:
{ label: "a-onBeforeNavigate",
You'll need another reviewer for this part-- I can never remember how to read
these tests...

https://codereview.chromium.org/2385363002/diff/1/components/renderer_context...
File components/renderer_context_menu/render_view_context_menu_base.cc (right):

https://codereview.chromium.org/2385363002/diff/1/components/renderer_context...
components/renderer_context_menu/render_view_context_menu_base.cc:166:
render_process_id_(render_frame_host->GetProcess()->GetID()) {}
Yep, already have it here.

https://codereview.chromium.org/2385363002/diff/1/components/renderer_context...
File components/renderer_context_menu/render_view_context_menu_base.h (right):

https://codereview.chromium.org/2385363002/diff/1/components/renderer_context...
components/renderer_context_menu/render_view_context_menu_base.h:200: const int
render_process_id_;
Looks like we already have it?

https://codereview.chromium.org/2385363002/diff/1/components/web_contents_del...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/2385363002/diff/1/components/web_contents_del...
components/web_contents_delegate_android/web_contents_delegate_android.cc:230:
Java_WebContentsDelegateAndroid_webContentsCreated(
I won't ask you to dive into this in this CL, but Android appears to be at risk
of this bug as well.

[Charlie Reis, 2016-10-03 22:32:51]

https://codereview.chromium.org/2385363002/diff/1/chrome/test/data/extensions...
File
chrome/test/data/extensions/api_test/webnavigation/userAction/test_userAction.js
(right):

https://codereview.chromium.org/2385363002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/webnavigation/userAction/test_userAction.js:24:
{ label: "a-onBeforeNavigate",
On 2016/10/03 22:15:23, Charlie Reis wrote:
> You'll need another reviewer for this part-- I can never remember how to read
> these tests...

Thanks for explaining offline.  (It'd be nice to eventually move these to a more
self-documenting format, or at least include a link to documentation in each
such test.)

The events listed here seem reasonable to me, so LGTM for this as well.

[nasko, 2016-10-03 22:37:27]

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

[nasko, 2016-10-03 22:38:03]

nasko@chromium.org changed reviewers:
+ jam@chromium.org

[nasko, 2016-10-03 22:38:04]

Hey John,
Can you review for OWNERS on this CL?
Thanks in advance!
Nasko

https://codereview.chromium.org/2385363002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
(right):

https://codereview.chromium.org/2385363002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc:537: for
(const auto& frame : frames) {
On 2016/10/03 22:15:23, Charlie Reis wrote:
> This is a bit odd at first glance.  Maybe clarify in a comment that we want to
> grab the first (and only?) frame?  Would it be clearer to go straight to
> frames[1] after verifying that it exists?

Done.

https://codereview.chromium.org/2385363002/diff/1/components/renderer_context...
File components/renderer_context_menu/render_view_context_menu_base.cc (right):

https://codereview.chromium.org/2385363002/diff/1/components/renderer_context...
components/renderer_context_menu/render_view_context_menu_base.cc:166:
render_process_id_(render_frame_host->GetProcess()->GetID()) {}
On 2016/10/03 22:15:23, Charlie Reis wrote:
> Yep, already have it here.

Acknowledged.

https://codereview.chromium.org/2385363002/diff/1/components/renderer_context...
File components/renderer_context_menu/render_view_context_menu_base.h (right):

https://codereview.chromium.org/2385363002/diff/1/components/renderer_context...
components/renderer_context_menu/render_view_context_menu_base.h:200: const int
render_process_id_;
On 2016/10/03 22:15:23, Charlie Reis wrote:
> Looks like we already have it?

D'oh! I moved it to public, as it is used by a subclass.

https://codereview.chromium.org/2385363002/diff/1/components/web_contents_del...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://codereview.chromium.org/2385363002/diff/1/components/web_contents_del...
components/web_contents_delegate_android/web_contents_delegate_android.cc:230:
Java_WebContentsDelegateAndroid_webContentsCreated(
On 2016/10/03 22:15:23, Charlie Reis wrote:
> I won't ask you to dive into this in this CL, but Android appears to be at
risk
> of this bug as well.

I'll file a bug to get this fixed or do it in a separate CL.

[commit-bot: I haz the power, 2016-10-03 22:38:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-10-03 22:58:47]

lgtm

[nasko, 2016-10-03 23:23:28]

The CQ bit was unchecked by nasko@chromium.org

[nasko, 2016-10-03 23:24:10]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-10-03 23:24:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, creis@chromium.org, lazyboy@chromium.org
Link to the patchset: https://codereview.chromium.org/2385363002/#ps20001
(title: "Address review feedback and compile problem.")

[commit-bot: I haz the power, 2016-10-03 23:24:54]

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, 2016-10-04 00:20:37]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-04 00:23:53]

Description was changed from

==========
Pass the RenderProcessHost id on retargeting.

In cases of out-of-process iframes, RetargetingDetails doesn't currently
contain enough information to find the correct RenderFrameHost.
This CL adds the RenderProcessHost id in the RetargetingDetails struct
to allow consumers of it to correctly discover RenderFrameHost that
created the new WebContents.

BUG=649855
==========

to

==========
Pass the RenderProcessHost id on retargeting.

In cases of out-of-process iframes, RetargetingDetails doesn't currently
contain enough information to find the correct RenderFrameHost.
This CL adds the RenderProcessHost id in the RetargetingDetails struct
to allow consumers of it to correctly discover RenderFrameHost that
created the new WebContents.

BUG=649855

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

[commit-bot: I haz the power, 2016-10-04 00:23:55]

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