New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 649855 link

Starred by 4 users
Status: Fixed
Owner:
█ nasko@chromium.org
Out until 24 Jan
Closed: Oct 2016
Cc:
a...@chromium.org
█ nick@chromium.org
nparker@chromium.org
█ jialiul@chromium.org
mea...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 639467



Sign in to add a comment

OOPIF: WebNavigationEventRouter::Retargeting uses wrong process ID for subframes

Project Member Reported by creis@chromium.org, Sep 23 2016 
WebNavigationEventRouter::Retargeting uses RetargetingDetails::source_render_frame_id, but matches it up with details->source_web_contents->GetRenderProcessHost()->GetID().  That's the process ID for the main frame, which is wrong if the source RFH is an out-of-process iframe.

Nasko, can you help fix, since it's in the Web Navigation API?  We probably need to add the source_render_process_id to RetargetingDetails, unless we want to go further and remove NOTIFICATION_RETARGETING in favor of WebContentsObserver::DidOpenRequestedURL().

Comment 1 by a...@chromium.org, Sep 23 2016 

It would be great if we could ditch NOTIFICATION_RETARGETING. It's weird that we have a notification with a single listener.

Comment 2 by jialiul@chromium.org, Sep 27 2016

Blocking: 639467
Cc: nparker@chromium.org
Is there an ETA for either fixing RetargetingDetails::source_render_frame_id or 
WebContentsObserver::DidOpenRequestedURL()?

Comment 3 by nasko@chromium.org, Sep 27 2016 

No ETA at this point. I'll see if I can poke at it later this week, but cannot give anything more than that.
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 4 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf

commit e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf
Author: nasko <nasko@chromium.org>
Date: Tue Oct 04 00:20:17 2016

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 

Review-Url: https://codereview.chromium.org/2385363002
Cr-Commit-Position: refs/heads/master@{#422621}

[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/android_webview/native/aw_web_contents_delegate.cc
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/android_webview/native/aw_web_contents_delegate.h
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/chrome/browser/devtools/devtools_window.cc
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/chrome/browser/devtools/devtools_window.h
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/chrome/browser/renderer_context_menu/render_view_context_menu.cc
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/chrome/browser/tab_contents/retargeting_details.h
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/chrome/browser/ui/browser.cc
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/chrome/browser/ui/browser.h
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/chrome/test/data/extensions/api_test/webnavigation/userAction/a.html
[add] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/chrome/test/data/extensions/api_test/webnavigation/userAction/a.js
[add] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/chrome/test/data/extensions/api_test/webnavigation/userAction/subframe.html
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/chrome/test/data/extensions/api_test/webnavigation/userAction/test_userAction.js
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/components/renderer_context_menu/render_view_context_menu_base.cc
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/components/renderer_context_menu/render_view_context_menu_base.h
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/components/web_contents_delegate_android/web_contents_delegate_android.cc
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/components/web_contents_delegate_android/web_contents_delegate_android.h
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/content/public/browser/web_contents_delegate.h
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/extensions/browser/guest_view/web_view/web_view_guest.cc
[modify] https://crrev.com/e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf/extensions/browser/guest_view/web_view/web_view_guest.h
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 4 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a00c95e20ede74fb518518fd45c50136c972db57

commit a00c95e20ede74fb518518fd45c50136c972db57
Author: fwang <fwang@igalia.com>
Date: Tue Oct 04 10:46:12 2016

Fix build error in headless_web_contents_impl.cc

e2df1d707cc2a7ba4a5b49f5f1f6be249ff05baf adds a new parameter to
WebContentsDelegate::WebContentsCreated. This CL updates the
implementation in headless_web_contents_impl.cc accordingly.

BUG= 649855 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2385123004
Cr-Commit-Position: refs/heads/master@{#422745}

[modify] https://crrev.com/a00c95e20ede74fb518518fd45c50136c972db57/headless/lib/browser/headless_web_contents_impl.cc
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 10 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/04cfc1198b20fb1aea618c89ce9a8b579101a879

commit 04cfc1198b20fb1aea618c89ce9a8b579101a879
Author: nasko <nasko@chromium.org>
Date: Mon Oct 10 20:39:50 2016

Pass extra parameter to the Java WebContentsDelegateAndroid.

This CL is a follow up to https://codereview.chromium.org/2385363002/
which added a new parameter to WebContentsDelegate::WebContentsCreated.
It adds the extra parameter in the C++ to Java transition.

BUG= 649855 

Review-Url: https://codereview.chromium.org/2408523002
Cr-Commit-Position: refs/heads/master@{#424229}

[modify] https://crrev.com/04cfc1198b20fb1aea618c89ce9a8b579101a879/chrome/android/java/src/org/chromium/chrome/browser/tab/EmptyTabObserver.java
[modify] https://crrev.com/04cfc1198b20fb1aea618c89ce9a8b579101a879/chrome/android/java/src/org/chromium/chrome/browser/tab/TabObserver.java
[modify] https://crrev.com/04cfc1198b20fb1aea618c89ce9a8b579101a879/chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java
[modify] https://crrev.com/04cfc1198b20fb1aea618c89ce9a8b579101a879/components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java
[modify] https://crrev.com/04cfc1198b20fb1aea618c89ce9a8b579101a879/components/web_contents_delegate_android/web_contents_delegate_android.cc

Comment 7 by jialiul@chromium.org, Oct 11 2016

Cc: mea...@chromium.org

Comment 8 by nasko@chromium.org, Oct 11 2016

Status: Fixed (was: Assigned)
This is now fixed. I will try to remove NOTIFICATION_RETARGETING in general as part of fixing webNavigation API to work with PlzNavigate.
►

Sign in to add a comment