Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2666193002:
Switch RenderViewContextMenu to use RequestOpenURL (Closed)
|
Created:
3 years, 10 months ago by Patrick Noland Modified:
3 years, 10 months ago CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitch RenderViewContextMenu to use RequestOpenURL
Currently, RenderViewContextMenu calls OpenURL on the source WebContents
when "open link in new tab" is activated. This is almost identical to
RequestOpenURL, except that it doesn't notify WebContentsObservers. This
change:
Changes NavigatorDelegate's RequestOpenURL to OpenURL, with a signature
identical to PageNavigator's OpenURL
Adds source_render_frame_host_id and source_render_process_id to OpenURLParams
Populates the above ids in NavigatorImpl's RequestOpenURL
Populates the above ids in RenderViewContextMenu's call to OpenURL
Removes RenderViewContextMenu's NotifyURLOpened
BUG=688214
R=nasko@chromium.org, creis@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2666193002
Cr-Commit-Position: refs/heads/master@{#452151}
Committed: https://chromium.googlesource.com/chromium/src/+/48894465b91ec112cbb9bd92e37f4c83010c14be
Patch Set 1 #Patch Set 2 : Change existing interface #Patch Set 3 : Add started_from_context_menu #
Total comments: 7
Patch Set 4 : cleanup #
Total comments: 14
Patch Set 5 : Requested fixes #
Total comments: 4
Patch Set 6 : CHECK site instance matches #
Total comments: 2
Patch Set 7 : Initialize frame_id to MSG_ROUTING_NONE #
Total comments: 13
Patch Set 8 : Address Charlie's comments #
Total comments: 1
Messages
Total messages: 94 (65 generated)
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Switch RenderViewContextMenu to use RequestOpenURL BUG= ========== to ========== Switch RenderViewContextMenu to use RequestOpenURL BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Switch RenderViewContextMenu to use RequestOpenURL BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Expose RequestOpenURL on PageNavigator Change NavigatorDelegate signature to use a RenderFrameHost instead of a RenderFrameHostImpl Switch RenderViewContextMenu to use RequestOpenURL BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pnoland@chromium.org changed reviewers: + creis@chromium.org, nick@chromium.org
pnoland@chromium.org changed reviewers: + nasko@chromium.org
Description was changed from ========== Expose RequestOpenURL on PageNavigator Change NavigatorDelegate signature to use a RenderFrameHost instead of a RenderFrameHostImpl Switch RenderViewContextMenu to use RequestOpenURL BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Currently, RenderViewContextMenu calls OpenURL on the source WebContents when "open link in new tab" is activated. This is almost identical to RequestOpenURL, except that it doesn't notify WebContentsObservers. This change: Exposes RequestOpenURL on PageNavigator Changes NavigatorDelegate signature to use a RenderFrameHost instead of a RenderFrameHostImpl Switches RenderViewContextMenu to use RequestOpenURL BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Please let me know how this change looks. I'm not 100% committed to this approach, but it seems to be fairly small in scope. If it's a workable approach, I'll update with a bug and tests.
Thanks for putting this together. Can you file a bug to track this? I'd like to have a record of the motivation (i.e., WebContentsObservers should be able to observe "Open in new tab" events). One concern about the public API below. I'll be on vacation after today, so maybe Nick can help finish up the review. https://codereview.chromium.org/2666193002/diff/40001/components/renderer_con... File components/renderer_context_menu/render_view_context_menu_base.cc (left): https://codereview.chromium.org/2666193002/diff/40001/components/renderer_con... components/renderer_context_menu/render_view_context_menu_base.cc:399: NotifyURLOpened(url, new_contents); This looks like it would be dead code if we remove this call. Does the NOTIFICATION_RETARGETING happen somewhere else? Is it safe to remove NotifyURLOpened itself? https://codereview.chromium.org/2666193002/diff/40001/components/renderer_con... File components/renderer_context_menu/render_view_context_menu_base.cc (right): https://codereview.chromium.org/2666193002/diff/40001/components/renderer_con... components/renderer_context_menu/render_view_context_menu_base.cc:390: OpenURLParams open_url_params(url, referrer, disposition, transition, false, Side note: The "false" parameter here is why this is not the same as a control click from the renderer. Context menu navigations are treated as browser-initiated, and control clicks (etc) are treated as renderer-initiated. There's a big difference for security checks and the process model. That said, I agree that this case should fire the observer method as well. https://codereview.chromium.org/2666193002/diff/40001/content/public/browser/... File content/public/browser/page_navigator.h (right): https://codereview.chromium.org/2666193002/diff/40001/content/public/browser/... content/public/browser/page_navigator.h:104: virtual void RequestOpenURL(RenderFrameHost* render_frame_host, I think we'll need to find a way to improve this part, for a few reasons. One, it makes this PageNavigator interface confusing. Callers don't have a good way know when to use OpenURL vs RequestOpenURL, which have different return types but only differ in whether they fire DidOpenRequestedURL afterward. I feel like we shouldn't make them face this choice, though I suppose better documentation might be one option. Two, it introduces a confusion over |render_frame_host|, which can differ from the frame corresponding to OpenURLParams::frame_tree_node_id. If I understand correctly, |render_frame_host| should really be called |source_render_frame_host|, and frame_tree_node_id is the destination frame (or -1 for the main frame). I see that we need this to give context menu code (which is in components) a way to generate DidOpenRequestedURL with the correct source RFH. No one else should need to do this, though, so maybe we can move this out of PageNavigator and onto WebContents itself, with an explanation of the difference and a disclaimer that most people should use PageNavigator::OpenURL instead? (This still kind of feels like exposing an internal helper method as a public interface, which is unfortunate. I don't see a way around that given how the context menu code is in components, though.) https://codereview.chromium.org/2666193002/diff/40001/content/public/browser/... File content/public/browser/web_contents_observer.h (right): https://codereview.chromium.org/2666193002/diff/40001/content/public/browser/... content/public/browser/web_contents_observer.h:326: // clicked. The |source_render_frame_host| is the frame in which the action Please also mention "Open in new tab" from a context menu as an example, since it's a notable difference from before. https://codereview.chromium.org/2666193002/diff/40001/content/public/browser/... content/public/browser/web_contents_observer.h:334: bool started_from_context_meu) {} nit: started_from_context_menu
On 2017/02/02 23:34:23, Charlie Reis (OOO til Feb 13) wrote: > Thanks for putting this together. Can you file a bug to track this? I'd like > to have a record of the motivation (i.e., WebContentsObservers should be able to > observe "Open in new tab" events). > > One concern about the public API below. I'll be on vacation after today, so > maybe Nick can help finish up the review. > > https://codereview.chromium.org/2666193002/diff/40001/components/renderer_con... > File components/renderer_context_menu/render_view_context_menu_base.cc (left): > > https://codereview.chromium.org/2666193002/diff/40001/components/renderer_con... > components/renderer_context_menu/render_view_context_menu_base.cc:399: > NotifyURLOpened(url, new_contents); > This looks like it would be dead code if we remove this call. Does the > NOTIFICATION_RETARGETING happen somewhere else? Is it safe to remove > NotifyURLOpened itself? > > https://codereview.chromium.org/2666193002/diff/40001/components/renderer_con... > File components/renderer_context_menu/render_view_context_menu_base.cc (right): > > https://codereview.chromium.org/2666193002/diff/40001/components/renderer_con... > components/renderer_context_menu/render_view_context_menu_base.cc:390: > OpenURLParams open_url_params(url, referrer, disposition, transition, false, > Side note: The "false" parameter here is why this is not the same as a control > click from the renderer. Context menu navigations are treated as > browser-initiated, and control clicks (etc) are treated as renderer-initiated. > There's a big difference for security checks and the process model. > > That said, I agree that this case should fire the observer method as well. > > https://codereview.chromium.org/2666193002/diff/40001/content/public/browser/... > File content/public/browser/page_navigator.h (right): > > https://codereview.chromium.org/2666193002/diff/40001/content/public/browser/... > content/public/browser/page_navigator.h:104: virtual void > RequestOpenURL(RenderFrameHost* render_frame_host, > I think we'll need to find a way to improve this part, for a few reasons. > > One, it makes this PageNavigator interface confusing. Callers don't have a good > way know when to use OpenURL vs RequestOpenURL, which have different return > types but only differ in whether they fire DidOpenRequestedURL afterward. I > feel like we shouldn't make them face this choice, though I suppose better > documentation might be one option. > > Two, it introduces a confusion over |render_frame_host|, which can differ from > the frame corresponding to OpenURLParams::frame_tree_node_id. If I understand > correctly, |render_frame_host| should really be called > |source_render_frame_host|, and frame_tree_node_id is the destination frame (or > -1 for the main frame). > > I see that we need this to give context menu code (which is in components) a way > to generate DidOpenRequestedURL with the correct source RFH. No one else should > need to do this, though, so maybe we can move this out of PageNavigator and onto > WebContents itself, with an explanation of the difference and a disclaimer that > most people should use PageNavigator::OpenURL instead? > > (This still kind of feels like exposing an internal helper method as a public > interface, which is unfortunate. I don't see a way around that given how the > context menu code is in components, though.) > > https://codereview.chromium.org/2666193002/diff/40001/content/public/browser/... > File content/public/browser/web_contents_observer.h (right): > > https://codereview.chromium.org/2666193002/diff/40001/content/public/browser/... > content/public/browser/web_contents_observer.h:326: // clicked. The > |source_render_frame_host| is the frame in which the action > Please also mention "Open in new tab" from a context menu as an example, since > it's a notable difference from before. > > https://codereview.chromium.org/2666193002/diff/40001/content/public/browser/... > content/public/browser/web_contents_observer.h:334: bool > started_from_context_meu) {} > nit: started_from_context_menu I'm also on vacation starting basically now, so nasko@ is your best bet next week.
On 2017/02/02 23:47:07, ncarter (away) wrote: > I'm also on vacation starting basically now, so nasko@ is your best bet next > week. Ah, of course. Yes, Nasko can help. And one other idea we're considering, after stepping back a bit. Maybe this DidOpenRequestedURL case is really more of a DidCreateWebContents case, and OpenURL should always fire it if the disposition of the navigation causes it to open another WebContents. That might be more consistent (although I'm not sure how we'd include the source RFH if that's needed, since I don't want to require that as a parameter to OpenURL). Nick did have a concern that this might go too far, though. For example, control clicking on the bookmark bar might conceivably be implemented using OpenURL on the current tab, even though the current tab isn't the one navigating (and shouldn't get a notification if the user control clicks a bookmark). Anyway, I'm sure Nasko will have some useful thoughts here.
On 2017/02/02 23:57:04, Charlie Reis (OOO til Feb 13) wrote: > On 2017/02/02 23:47:07, ncarter (away) wrote: > > I'm also on vacation starting basically now, so nasko@ is your best bet next > > week. > > Ah, of course. Yes, Nasko can help. > > And one other idea we're considering, after stepping back a bit. Maybe this > DidOpenRequestedURL case is really more of a DidCreateWebContents case, and > OpenURL should always fire it if the disposition of the navigation causes it to > open another WebContents. That might be more consistent (although I'm not sure > how we'd include the source RFH if that's needed, since I don't want to require > that as a parameter to OpenURL). > > Nick did have a concern that this might go too far, though. For example, > control clicking on the bookmark bar might conceivably be implemented using > OpenURL on the current tab, even though the current tab isn't the one navigating > (and shouldn't get a notification if the user control clicks a bookmark). > > Anyway, I'm sure Nasko will have some useful thoughts here. This is nice because it avoids the API pollution problem you raised. One possibility to include the source RenderFrameHost would be to make both public functions(OpenURL and RequestOpenURL) thunks for an implementation that takes in a (potentially not present) source RFH. This would require observers to defensively check if the source RFH is present, but that makes a certain amount of sense to me. There's not always a RFH you can definitively say is the source. This might also help with the problem that Nick pointed out. We'll still notify when doing something like ctrl-clicking a bookmark, but the notification won't include a source RFH. Or maybe there are really two types of events(those with a source RFH, and those without), and there should be separate observer methods for each?
Description was changed from ========== Currently, RenderViewContextMenu calls OpenURL on the source WebContents when "open link in new tab" is activated. This is almost identical to RequestOpenURL, except that it doesn't notify WebContentsObservers. This change: Exposes RequestOpenURL on PageNavigator Changes NavigatorDelegate signature to use a RenderFrameHost instead of a RenderFrameHostImpl Switches RenderViewContextMenu to use RequestOpenURL BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Currently, RenderViewContextMenu calls OpenURL on the source WebContents when "open link in new tab" is activated. This is almost identical to RequestOpenURL, except that it doesn't notify WebContentsObservers. This change: Exposes RequestOpenURL on PageNavigator Changes NavigatorDelegate signature to use a RenderFrameHost instead of a RenderFrameHostImpl Switches RenderViewContextMenu to use RequestOpenURL BUG=688214 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
On 2017/02/03 01:35:36, Patrick Noland wrote: > On 2017/02/02 23:57:04, Charlie Reis (OOO til Feb 13) wrote: > > On 2017/02/02 23:47:07, ncarter (away) wrote: > > > I'm also on vacation starting basically now, so nasko@ is your best bet next > > > week. > > > > Ah, of course. Yes, Nasko can help. > > > > And one other idea we're considering, after stepping back a bit. Maybe this > > DidOpenRequestedURL case is really more of a DidCreateWebContents case, and > > OpenURL should always fire it if the disposition of the navigation causes it > to > > open another WebContents. That might be more consistent (although I'm not > sure > > how we'd include the source RFH if that's needed, since I don't want to > require > > that as a parameter to OpenURL). I actually tried to tackle this in the past, but didn't have time to finish it off. My approach was to always fire the notification and rename it later to something like "DidCreateWebContents", which is what it actually means in practice. My old CL is https://codereview.chromium.org/2383573002/ if you want to look at it as another attempt, though it was very far from complete/good. > > Nick did have a concern that this might go too far, though. For example, > > control clicking on the bookmark bar might conceivably be implemented using > > OpenURL on the current tab, even though the current tab isn't the one > navigating > > (and shouldn't get a notification if the user control clicks a bookmark). The current behavior is to only call DidOpenRequestedURL when a new WebContents is created through RequestOpenURL. As long as we keep it behaving the same way - only fire for new WebContents creation, it seems consistent to me and useful signal to have. > > Anyway, I'm sure Nasko will have some useful thoughts here. > > This is nice because it avoids the API pollution problem you raised. One > possibility to include the source RenderFrameHost would be to make both public > functions(OpenURL and RequestOpenURL) thunks for an implementation that takes in > a (potentially not present) source RFH. This would require observers to > defensively check if the source RFH is present, but that makes a certain amount > of sense to me. There's not always a RFH you can definitively say is the source. Each context menu "Open in another tab" is associate with a RenderFrameHost, isn't it? If you have that and the bit that it says it is created from a context menu, you have all the information you need to make all types of decisions. > This might also help with the problem that Nick pointed out. We'll still notify > when doing something like ctrl-clicking a bookmark, but the notification won't > include a source RFH. > > Or maybe there are really two types of events(those with a source RFH, and those > without), and there should be separate observer methods for each? I'd rather keep it one event and supply nullptr for the RenderFrameHost source, if truly not there, than have two APIs that are identical, except for a single parameter.
https://codereview.chromium.org/2666193002/diff/40001/components/renderer_con... File components/renderer_context_menu/render_view_context_menu_base.cc (left): https://codereview.chromium.org/2666193002/diff/40001/components/renderer_con... components/renderer_context_menu/render_view_context_menu_base.cc:399: NotifyURLOpened(url, new_contents); On 2017/02/02 23:34:22, Charlie Reis (OOO til Feb 13) wrote: > This looks like it would be dead code if we remove this call. Does the > NOTIFICATION_RETARGETING happen somewhere else? Is it safe to remove > NotifyURLOpened itself? I don't think it is safe to remove. Last time I looked, this is needed for webNavigation API to dispatch events for newly created WebContents.
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/07 19:39:34, nasko (very slow) wrote: > https://codereview.chromium.org/2666193002/diff/40001/components/renderer_con... > File components/renderer_context_menu/render_view_context_menu_base.cc (left): > > https://codereview.chromium.org/2666193002/diff/40001/components/renderer_con... > components/renderer_context_menu/render_view_context_menu_base.cc:399: > NotifyURLOpened(url, new_contents); > On 2017/02/02 23:34:22, Charlie Reis (OOO til Feb 13) wrote: > > This looks like it would be dead code if we remove this call. Does the > > NOTIFICATION_RETARGETING happen somewhere else? Is it safe to remove > > NotifyURLOpened itself? > > I don't think it is safe to remove. Last time I looked, this is needed for > webNavigation API to dispatch events for newly created WebContents. NOTIFICATION_RETARGETING is emitted in two places: this context menu, and in browser::WebContentsCreated . It's listened for in two places: the WebNavigation API and SafeBrowsingNavigationObserver. Both locations implement WebContentObserver, so refactoring them to use DidOpenRequestedURL shouldn't be super obtrusive in theory. In practice it's harder because both make assumptions specific to the current behavior of the retargeting notification, and that behavior needs to be replicated. For example, WebNavigationAPI stores pending WebContents(not in the tabstrip yet, which happens when opening a new window) and later acts on them once a TAB_ADDED notification is received. I think it's possible and desirable to safely remove the context menu's emission of the retargeting notififcation before later removing the notification entirely.
> The current behavior is to only call DidOpenRequestedURL when a new WebContents > is created through RequestOpenURL. As long as we keep it behaving the same way - > only fire for new WebContents creation, it seems consistent to me and useful > signal to have. Agreed. > Each context menu "Open in another tab" is associate with a RenderFrameHost, > isn't it? If you have that and the bit that it says it is created from a context > menu, you have all the information you need to make all types of decisions. Yes. In this case we have the information needed to provide a source rfh, but the current OpenURL interface doesn't allow us to supply it. > I'd rather keep it one event and supply nullptr for the RenderFrameHost source, > if truly not there, than have two APIs that are identical, except for a single > parameter. So if this is your preference, is it better tii add optional process_id and frame_id parameters to OpenURLParams or CreateParams? Then when a new WebContents is created in OpenURL, observers are notified regardless of if those parameters point to a valid FrameHost. I noticed that in your "remove NOTIFICATION_RETARGETING" change, you added this information to CreateParams.
On 2017/02/08 18:29:13, Patrick Noland wrote: > > The current behavior is to only call DidOpenRequestedURL when a new > WebContents > > is created through RequestOpenURL. As long as we keep it behaving the same way > - > > only fire for new WebContents creation, it seems consistent to me and useful > > signal to have. > Agreed. I might actually be incorrect in my statement here, we should do some more in-depth investigation to confirm/deny. Doing some cursory code inspection, it does seem that the call to OpenURL can return the source WebContents as the output value, which will result in the DidOpenRequestedURL call to be made. > > Each context menu "Open in another tab" is associate with a RenderFrameHost, > > isn't it? If you have that and the bit that it says it is created from a > context > > menu, you have all the information you need to make all types of decisions. > > Yes. In this case we have the information needed to provide a source rfh, but > the current OpenURL interface doesn't allow us to supply it. I also recall trying to go down that route and it wasn't very pleasant :(. > > I'd rather keep it one event and supply nullptr for the RenderFrameHost > source, > > if truly not there, than have two APIs that are identical, except for a single > > parameter. > > So if this is your preference, is it better tii add optional process_id and > frame_id parameters to OpenURLParams or CreateParams? Then when a new > WebContents is created in OpenURL, observers are notified regardless of if those > parameters point to a valid FrameHost. I noticed that in your "remove > NOTIFICATION_RETARGETING" change, you added this information to CreateParams. Would this be workable for your case? > NOTIFICATION_RETARGETING is emitted in two places: this context menu, and in > browser::WebContentsCreated . It's listened for in two places: the WebNavigation > API and SafeBrowsingNavigationObserver. Both locations implement > WebContentObserver, so refactoring them to use DidOpenRequestedURL shouldn't be > super obtrusive in theory. In theory, it should be identical and I've wanted to move both of these over to WebContentsObserver :). > In practice it's harder because both make assumptions > specific to the current behavior of the retargeting notification, and that > behavior needs to be replicated. What specific part of the behavior do we need to preserve? The only thing I hit in the past was the ordering of events, so if there is anything else, it will be good to know. > For example, WebNavigationAPI stores pending > WebContents(not in the tabstrip yet, which happens when opening a new window) > and later acts on them once a TAB_ADDED notification is received. I think it's > possible and desirable to safely remove the context menu's emission of the > retargeting notififcation before later removing the notification entirely. webNavigation API does need to continue receiving notifications when a context menu causes a new Webcontents to be created. It is our API contract with extensions and we can't change that.
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Nasko, This patchset: - Merges OpenURL and RequestOpenURL, although OpenURL exists on two separate interfaces. - Adds frame_id and process_id as optional parameters to OpenURLParams - Adds frame_id and process_id to RenderViewContextMenu's invocation of OpenURL - Notifies observers if the result of OpenURL is distinct from the current WebContents - Guards against a null source_render_frame_host in web_navigation_api PTAL and let me know what you think. https://codereview.chromium.org/2666193002/diff/40001/components/renderer_con... File components/renderer_context_menu/render_view_context_menu_base.cc (left): https://codereview.chromium.org/2666193002/diff/40001/components/renderer_con... components/renderer_context_menu/render_view_context_menu_base.cc:399: NotifyURLOpened(url, new_contents); On 2017/02/02 23:34:22, Charlie Reis (OOO til Feb 13) wrote: > This looks like it would be dead code if we remove this call. Does the > NOTIFICATION_RETARGETING happen somewhere else? Is it safe to remove > NotifyURLOpened itself? Yes, NotifyURLOpened would be dead code and safe to remove. However NOTIFICATION_RETARGETING is still emitted from Browser::WebContentsCreated. That might be removable with some additional changes to WebContentsImpl::CreateNewWindow.
Thanks a ton for this work! It is mostly there and I've added a few comments. https://codereview.chromium.org/2666193002/diff/100001/components/renderer_co... File components/renderer_context_menu/render_view_context_menu_base.cc (left): https://codereview.chromium.org/2666193002/diff/100001/components/renderer_co... components/renderer_context_menu/render_view_context_menu_base.cc:399: NotifyURLOpened(url, new_contents); Since there are no more calls to this, let's remove it in this CL. https://codereview.chromium.org/2666193002/diff/100001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/2666193002/diff/100001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl.cc:530: return nullptr; Let's add a NOTREACHED to ensure we don't accidentally hit that. https://codereview.chromium.org/2666193002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2666193002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2580: if (new_contents && new_contents != this) { The old code included a null check for the render_frame_host, let's keep it here to ensure we are consistent in behavior. https://codereview.chromium.org/2666193002/diff/100001/content/public/browser... File content/public/browser/page_navigator.h (right): https://codereview.chromium.org/2666193002/diff/100001/content/public/browser... content/public/browser/page_navigator.h:76: int frame_id; nit: render_frame_id https://codereview.chromium.org/2666193002/diff/100001/content/public/browser... content/public/browser/page_navigator.h:77: // Process id of the source RenderFrameHost. nit: Leave an extra empty line between to match how the rest of the file is formatted. https://codereview.chromium.org/2666193002/diff/100001/content/public/browser... content/public/browser/page_navigator.h:78: int process_id; nit: render_process_id https://codereview.chromium.org/2666193002/diff/100001/content/test/web_conte... File content/test/web_contents_observer_sanity_checker.cc (right): https://codereview.chromium.org/2666193002/diff/100001/content/test/web_conte... content/test/web_contents_observer_sanity_checker.cc:284: // AssertRenderFrameExists(source_render_frame_host); This shouldn't be commented out.
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Nasko, PTAL https://codereview.chromium.org/2666193002/diff/100001/components/renderer_co... File components/renderer_context_menu/render_view_context_menu_base.cc (left): https://codereview.chromium.org/2666193002/diff/100001/components/renderer_co... components/renderer_context_menu/render_view_context_menu_base.cc:399: NotifyURLOpened(url, new_contents); On 2017/02/14 00:10:22, nasko wrote: > Since there are no more calls to this, let's remove it in this CL. Done. https://codereview.chromium.org/2666193002/diff/100001/content/browser/frame_... File content/browser/frame_host/interstitial_page_impl.cc (right): https://codereview.chromium.org/2666193002/diff/100001/content/browser/frame_... content/browser/frame_host/interstitial_page_impl.cc:530: return nullptr; On 2017/02/14 00:10:23, nasko wrote: > Let's add a NOTREACHED to ensure we don't accidentally hit that. Done. https://codereview.chromium.org/2666193002/diff/100001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2666193002/diff/100001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2580: if (new_contents && new_contents != this) { On 2017/02/14 00:10:23, nasko wrote: > The old code included a null check for the render_frame_host, let's keep it here > to ensure we are consistent in behavior. Done. https://codereview.chromium.org/2666193002/diff/100001/content/public/browser... File content/public/browser/page_navigator.h (right): https://codereview.chromium.org/2666193002/diff/100001/content/public/browser... content/public/browser/page_navigator.h:76: int frame_id; On 2017/02/14 00:10:23, nasko wrote: > nit: render_frame_id Done. https://codereview.chromium.org/2666193002/diff/100001/content/public/browser... content/public/browser/page_navigator.h:77: // Process id of the source RenderFrameHost. On 2017/02/14 00:10:23, nasko wrote: > nit: Leave an extra empty line between to match how the rest of the file is > formatted. Done. https://codereview.chromium.org/2666193002/diff/100001/content/public/browser... content/public/browser/page_navigator.h:78: int process_id; On 2017/02/14 00:10:23, nasko wrote: > nit: render_process_id Done. https://codereview.chromium.org/2666193002/diff/100001/content/test/web_conte... File content/test/web_contents_observer_sanity_checker.cc (right): https://codereview.chromium.org/2666193002/diff/100001/content/test/web_conte... content/test/web_contents_observer_sanity_checker.cc:284: // AssertRenderFrameExists(source_render_frame_host); On 2017/02/14 00:10:23, nasko wrote: > This shouldn't be commented out. Done. https://codereview.chromium.org/2666193002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/2666193002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:157: remove this newline https://codereview.chromium.org/2666193002/diff/120001/content/public/browser... File content/public/browser/page_navigator.h (right): https://codereview.chromium.org/2666193002/diff/120001/content/public/browser... content/public/browser/page_navigator.h:17: #include "content/public/browser/render_frame_host.h" Remove this #include
https://codereview.chromium.org/2666193002/diff/120001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2666193002/diff/120001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2581: I realized we discussed one thing I forgot to mention in my previous review. Since we are now supplying a source RenderFrameHost and a source SiteInstance, we should ensure that they are matching, otherwise mismatches will likely be problematic. Let's put a DCHECK or a CHECK that the SiteInstance of the source RenderFrameHost, if supplied, matches the source SiteInstance, if supplied.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2666193002/diff/120001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2666193002/diff/120001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2581: On 2017/02/14 01:20:11, nasko wrote: > I realized we discussed one thing I forgot to mention in my previous review. > > Since we are now supplying a source RenderFrameHost and a source SiteInstance, > we should ensure that they are matching, otherwise mismatches will likely be > problematic. Let's put a DCHECK or a CHECK that the SiteInstance of the source > RenderFrameHost, if supplied, matches the source SiteInstance, if supplied. Done.
Let's wait for Charlie to look over the CL, as he was intrigued by the removal of RequestOpenURL. For me, LGTM once the comment is addressed. https://codereview.chromium.org/2666193002/diff/140001/content/public/browser... File content/public/browser/page_navigator.h (right): https://codereview.chromium.org/2666193002/diff/140001/content/public/browser... content/public/browser/page_navigator.h:75: int render_frame_id; I'm sorry I missed this on the previous iterations, but we should be initializing these to MSG_ROUTING_NONE when constructing the struct. Otherwise we can get uninitialized memory with garbage content in it and lead to incorrect values.
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Charlie, PTAL https://codereview.chromium.org/2666193002/diff/140001/content/public/browser... File content/public/browser/page_navigator.h (right): https://codereview.chromium.org/2666193002/diff/140001/content/public/browser... content/public/browser/page_navigator.h:75: int render_frame_id; On 2017/02/15 00:58:07, nasko wrote: > I'm sorry I missed this on the previous iterations, but we should be > initializing these to MSG_ROUTING_NONE when constructing the struct. Otherwise > we can get uninitialized memory with garbage content in it and lead to incorrect > values. Done.
I like it. One step closer to removing NOTIFICATION_RETARGETING, too, which is nice. Please update the CL description, which I think is talking about an earlier patch set. Otherwise LGTM with nits! https://codereview.chromium.org/2666193002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/2666193002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:124: case chrome::NOTIFICATION_RETARGETING: { I was curious if this was dead, but apparently it's still needed for one other case? Seems ok for now-- hopefully we can remove it entirely before too long. https://codereview.chromium.org/2666193002/diff/160001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (left): https://codereview.chromium.org/2666193002/diff/160001/chrome/browser/rendere... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1982: chrome::NOTIFICATION_RETARGETING, Caveat: I haven't fully thought through the implications of removing this, so I'll take your and Nasko's word for it. https://codereview.chromium.org/2666193002/diff/160001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2666193002/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2579: RenderFrameHost* render_frame_host = nit: source_render_frame_host https://codereview.chromium.org/2666193002/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2583: CHECK(render_frame_host->GetSiteInstance() == minor nit: Does CHECK_EQ work here, or is the compiler unhappy with that? https://codereview.chromium.org/2666193002/diff/160001/content/public/browser... File content/public/browser/page_navigator.h (right): https://codereview.chromium.org/2666193002/diff/160001/content/public/browser... content/public/browser/page_navigator.h:76: int render_frame_id = MSG_ROUTING_NONE; nit: source_render_frame_id (This is useful to clarify in the places it's used, since it could easily be confused with the target RenderFrameHost.) https://codereview.chromium.org/2666193002/diff/160001/content/public/browser... content/public/browser/page_navigator.h:79: int render_process_id; nit: source_process_id Also, we should probably initialize this to ChildProcessHost::kInvalidUniqueID, similar to the routing ID case above.
Description was changed from
==========
Currently, RenderViewContextMenu calls OpenURL on the source WebContents when
"open link in new tab" is activated. This is almost identical to RequestOpenURL,
except that it doesn't notify WebContentsObservers. This change:
Exposes RequestOpenURL on PageNavigator
Changes NavigatorDelegate signature to use a RenderFrameHost instead of a
RenderFrameHostImpl
Switches RenderViewContextMenu to use RequestOpenURL
BUG=688214
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Switch RenderViewContextMenu to use RequestOpenURL
Currently, RenderViewContextMenu calls OpenURL on the source WebContents
when "open link in new tab" is activated. This is almost identical to
RequestOpenURL, except that it doesn't notify WebContentsObservers. This
change:
Changes NavigatorDelegate's RequestOpenURL to OpenURL, with a signature
identical to PageNavigator's OpenURL
Adds source_render_frame_host_id and source_render_process_id to OpenURLParams
Populates the above ids in RenderFrameHostImpl's RequestOpenURL
Populates the above ids in RenderViewContextMenu's call to OpenURL
Removes RenderViewContextMenu's NotifyURLOpened
BUG=688214
R=nasko@chromium.org, creis@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
Description was changed from
==========
Switch RenderViewContextMenu to use RequestOpenURL
Currently, RenderViewContextMenu calls OpenURL on the source WebContents
when "open link in new tab" is activated. This is almost identical to
RequestOpenURL, except that it doesn't notify WebContentsObservers. This
change:
Changes NavigatorDelegate's RequestOpenURL to OpenURL, with a signature
identical to PageNavigator's OpenURL
Adds source_render_frame_host_id and source_render_process_id to OpenURLParams
Populates the above ids in RenderFrameHostImpl's RequestOpenURL
Populates the above ids in RenderViewContextMenu's call to OpenURL
Removes RenderViewContextMenu's NotifyURLOpened
BUG=688214
R=nasko@chromium.org, creis@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Switch RenderViewContextMenu to use RequestOpenURL
Currently, RenderViewContextMenu calls OpenURL on the source WebContents
when "open link in new tab" is activated. This is almost identical to
RequestOpenURL, except that it doesn't notify WebContentsObservers. This
change:
Changes NavigatorDelegate's RequestOpenURL to OpenURL, with a signature
identical to PageNavigator's OpenURL
Adds source_render_frame_host_id and source_render_process_id to OpenURLParams
Populates the above ids in NavigatorImpl's RequestOpenURL
Populates the above ids in RenderViewContextMenu's call to OpenURL
Removes RenderViewContextMenu's NotifyURLOpened
BUG=688214
R=nasko@chromium.org, creis@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pnoland@chromium.org changed reviewers: + avi@chromium.org
Adding avi@ for owners review of renderer_context_menu files https://codereview.chromium.org/2666193002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right): https://codereview.chromium.org/2666193002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:124: case chrome::NOTIFICATION_RETARGETING: { On 2017/02/16 00:21:10, Charlie Reis wrote: > I was curious if this was dead, but apparently it's still needed for one other > case? Seems ok for now-- hopefully we can remove it entirely before too long. Acknowledged. https://codereview.chromium.org/2666193002/diff/160001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (left): https://codereview.chromium.org/2666193002/diff/160001/chrome/browser/rendere... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1982: chrome::NOTIFICATION_RETARGETING, On 2017/02/16 00:21:10, Charlie Reis wrote: > Caveat: I haven't fully thought through the implications of removing this, so > I'll take your and Nasko's word for it. Acknowledged. https://codereview.chromium.org/2666193002/diff/160001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2666193002/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2579: RenderFrameHost* render_frame_host = On 2017/02/16 00:21:10, Charlie Reis wrote: > nit: source_render_frame_host Done. https://codereview.chromium.org/2666193002/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2583: CHECK(render_frame_host->GetSiteInstance() == On 2017/02/16 00:21:10, Charlie Reis wrote: > minor nit: Does CHECK_EQ work here, or is the compiler unhappy with that? Done. https://codereview.chromium.org/2666193002/diff/160001/content/public/browser... File content/public/browser/page_navigator.h (right): https://codereview.chromium.org/2666193002/diff/160001/content/public/browser... content/public/browser/page_navigator.h:76: int render_frame_id = MSG_ROUTING_NONE; On 2017/02/16 00:21:10, Charlie Reis wrote: > nit: source_render_frame_id > > (This is useful to clarify in the places it's used, since it could easily be > confused with the target RenderFrameHost.) Done. https://codereview.chromium.org/2666193002/diff/160001/content/public/browser... content/public/browser/page_navigator.h:79: int render_process_id; On 2017/02/16 00:21:10, Charlie Reis wrote: > nit: source_process_id > > Also, we should probably initialize this to ChildProcessHost::kInvalidUniqueID, > similar to the routing ID case above. How about source_render_process_id?
Thanks! https://codereview.chromium.org/2666193002/diff/160001/content/public/browser... File content/public/browser/page_navigator.h (right): https://codereview.chromium.org/2666193002/diff/160001/content/public/browser... content/public/browser/page_navigator.h:79: int render_process_id; On 2017/02/16 00:57:12, Patrick Noland wrote: > On 2017/02/16 00:21:10, Charlie Reis wrote: > > nit: source_process_id > > > > Also, we should probably initialize this to > ChildProcessHost::kInvalidUniqueID, > > similar to the routing ID case above. > > How about source_render_process_id? I'm ok with that.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
lgtm https://codereview.chromium.org/2666193002/diff/180001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (left): https://codereview.chromium.org/2666193002/diff/180001/chrome/browser/rendere... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1982: chrome::NOTIFICATION_RETARGETING, Whoo!
The CQ bit was checked by pnoland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2666193002/#ps180001 (title: "Address Charlie's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by pnoland@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by pnoland@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by pnoland@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pnoland@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1487789431840830,
"parent_rev": "58aea4974d2230c5900f79d9d6344747033a6500", "commit_rev":
"48894465b91ec112cbb9bd92e37f4c83010c14be"}
Message was sent while issue was closed.
Description was changed from
==========
Switch RenderViewContextMenu to use RequestOpenURL
Currently, RenderViewContextMenu calls OpenURL on the source WebContents
when "open link in new tab" is activated. This is almost identical to
RequestOpenURL, except that it doesn't notify WebContentsObservers. This
change:
Changes NavigatorDelegate's RequestOpenURL to OpenURL, with a signature
identical to PageNavigator's OpenURL
Adds source_render_frame_host_id and source_render_process_id to OpenURLParams
Populates the above ids in NavigatorImpl's RequestOpenURL
Populates the above ids in RenderViewContextMenu's call to OpenURL
Removes RenderViewContextMenu's NotifyURLOpened
BUG=688214
R=nasko@chromium.org, creis@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Switch RenderViewContextMenu to use RequestOpenURL
Currently, RenderViewContextMenu calls OpenURL on the source WebContents
when "open link in new tab" is activated. This is almost identical to
RequestOpenURL, except that it doesn't notify WebContentsObservers. This
change:
Changes NavigatorDelegate's RequestOpenURL to OpenURL, with a signature
identical to PageNavigator's OpenURL
Adds source_render_frame_host_id and source_render_process_id to OpenURLParams
Populates the above ids in NavigatorImpl's RequestOpenURL
Populates the above ids in RenderViewContextMenu's call to OpenURL
Removes RenderViewContextMenu's NotifyURLOpened
BUG=688214
R=nasko@chromium.org, creis@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2666193002
Cr-Commit-Position: refs/heads/master@{#452151}
Committed:
https://chromium.googlesource.com/chromium/src/+/48894465b91ec112cbb9bd92e37f...
==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as https://chromium.googlesource.com/chromium/src/+/48894465b91ec112cbb9bd92e37f... |
