Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2632633006:
Implement NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE. (Closed)
|
Created:
3 years, 11 months ago by engedy Modified:
3 years, 7 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, blink-reviews-html_chromium.org, creis+watch_chromium.org, mlamouri+watch-blink_chromium.org, nasko+codewatch_chromium.org, jam, Randy Smith (Not in Mondays), dcheng, blink-reviews, dglazkov+blink, darin-cc_chromium.org, loading-reviews_chromium.org, kinuko+watch, blink-reviews-api_chromium.org, subresource-filter-reviews_chromium.org, clamy, alexmos Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE.
NavigationThrottle::WillStartRequest and WillRedirectRequest can now return
BLOCK_REQUEST_AND_COLLAPSE for navigations taking place in subframes.
Returning this throttle result will block the navigation, and load an error
page instead, similarly to BLOCK_REQUEST, but in addition, the child frame's
owner element in the parent frame will also be collapsed, i.e. removed from the
layout. The frame owner element is restored on the next successful
(non-error-page) navigation commit.
BUG=637415
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2632633006
Cr-Commit-Position: refs/heads/master@{#474735}
Committed: https://chromium.googlesource.com/chromium/src/+/6e2e0996275dd48dd4497f9c57e6370a423eb3ec
Patch Set 1 #Patch Set 2 : Remove surplus semicolon. #
Total comments: 14
Patch Set 3 : Addressed comments from clamy@. #Patch Set 4 : Rebase. #
Total comments: 4
Patch Set 5 : Moar tests. #
Total comments: 1
Patch Set 6 : Rebase. #
Total comments: 7
Patch Set 7 : Rebase + add checks for |net_error_code|. #Patch Set 8 : Extend tests, fix redirects. Non-PlzNavigate version still broken. #
Total comments: 24
Patch Set 9 : Rebase again. #Patch Set 10 : Addressed comments, made redirect response PlzNavigate-only. #
Total comments: 15
Patch Set 11 : Address comments from alexmos@. #Patch Set 12 : Rebase. #Patch Set 13 : Clean up frame name vs id. #Patch Set 14 : ... and without breaking stuff. #
Total comments: 16
Patch Set 15 : Rebase. #Patch Set 16 : Addressed comments from clamy@ and nasko@. #Patch Set 17 : Remove unused code. Wire up new throttle result in the subresource filter. #Patch Set 18 : Add tests for redirects. #Patch Set 19 : Update unittests. #
Total comments: 4
Patch Set 20 : Addressed comments from csharrison@. #
Total comments: 4
Patch Set 21 : Comment nits. #Patch Set 22 : Rebase. #
Total comments: 16
Patch Set 23 : Addressed comments from dcheng@. #Patch Set 24 : Rebase. #Patch Set 25 : Update SubframeNavigationFilteringThrottleTest.DelayMetrics #Patch Set 26 : Fix navigation transition type. #Messages
Total messages: 150 (93 generated)
Description was changed from ========== Implement NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE. NavigationThrottle::WillStartRequest and WillRedirectRequest can now return BLOCK_REQUEST_AND_COLLAPSE for navigations taking place in subframes. Returning this throttle result will block the navigation, and load an error page instead, similarly to BLOCK_REQUEST, but in addition, the child frame's owner element in the parent frame will also be collapsed, i.e. removed from the layout. The frame owner element is restored on the next successful (non-error-page) navigation commit. BUG=637415 ========== to ========== Implement NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE. NavigationThrottle::WillStartRequest and WillRedirectRequest can now return BLOCK_REQUEST_AND_COLLAPSE for navigations taking place in subframes. Returning this throttle result will block the navigation, and load an error page instead, similarly to BLOCK_REQUEST, but in addition, the child frame's owner element in the parent frame will also be collapsed, i.e. removed from the layout. The frame owner element is restored on the next successful (non-error-page) navigation commit. BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by engedy@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...
engedy@chromium.org changed reviewers: + clamy@chromium.org, nasko@chromium.org
@Nasko, @Camille, could you please take an initial look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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 engedy@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...
On 2017/01/13 17:54:15, engedy wrote: > @Nasko, @Camille, could you please take an initial look? Note that try failures with --site-per-process are due to the unrelated crbug.com/681077.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Friendly ping. Could you please have an initial look?
Thanks! I haven't reviewed the Blink code since I'm not familiar with it. A few comments below. Also note that since this touches the content/ public API we require an external usage of the code changed in the same patch. NavigationThrottleCheckResult is a bit of an exception, since it used internally as well. A usage in a content/ throttle would work as well. Tldr: it'd be also nice to have an idea of what the Throttle that will be using it will look like. https://codereview.chromium.org/2632633006/diff/20001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2632633006/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:146: // no effect unless the child frame is embedded using an <iframe> element. It's not very clear to me while reading this description what this means. Does it mean that the parent frame won't show or this one? https://codereview.chromium.org/2632633006/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2632633006/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:584: frame_tree_node_->SetFrameOwnerCollapsedState(true); Suggestion: maybe we could move this to the NavigationHandleImpl instead. This would make it easier to work with the current implementation as well :). https://codereview.chromium.org/2632633006/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:585: OnRequestFailed(false, net::ERR_BLOCKED_BY_CLIENT); Just checking: this means we will attempt to load an error page in the frame, that won't show AFAIU since it is collapsed. Is that the expected behavior or should we just cancel the request? https://codereview.chromium.org/2632633006/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2632633006/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:996: // FrameOwner in the parent via the child's current RenderFrame at any time. Does this mean that if we start a request for which we want to collapse the frame, we will collapse the current frame, even before we actually get a response for the request? https://codereview.chromium.org/2632633006/diff/20001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/2632633006/diff/20001/content/common/frame_me... content/common/frame_messages.h:746: IPC_MESSAGE_ROUTED1(FrameMsg_CollapseFrameOwner, bool /* collapsed */) Wouldn't is be easier to send the IPC to the parent instead, telling it to collapse a particular child? (Note sure if doable though). https://codereview.chromium.org/2632633006/diff/20001/content/public/browser/... File content/public/browser/navigation_throttle.h (right): https://codereview.chromium.org/2632633006/diff/20001/content/public/browser/... content/public/browser/navigation_throttle.h:45: BLOCK_REQUEST_AND_COLLAPSE, I'm wondering whether we should add a new ThrottleCheckResult vs providing a method on the NavigationHandle or the NavigationThrottle to collapse the frame. The NavigationThrottle would return an existing result in that case.
https://codereview.chromium.org/2632633006/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2632633006/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:584: frame_tree_node_->SetFrameOwnerCollapsedState(true); On 2017/01/18 15:10:55, clamy wrote: > Suggestion: maybe we could move this to the NavigationHandleImpl instead. This > would make it easier to work with the current implementation as well :). Done. https://codereview.chromium.org/2632633006/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:585: OnRequestFailed(false, net::ERR_BLOCKED_BY_CLIENT); On 2017/01/18 15:10:55, clamy wrote: > Just checking: this means we will attempt to load an error page in the frame, > that won't show AFAIU since it is collapsed. Is that the expected behavior or > should we just cancel the request? Yep, it is intended that we navigate away from the previously committed URL. Implementation-wise, it seemed that triggering an error page is the simplest way to get that behavior. Plus, this way the collapsed state is a function of the last committed URL, which is nice. (IIUC the error page will commit under the URL that errored out.) I was considering adding a new error code so that this error page would be emptier, but I am not sure if it makes a significant difference. https://codereview.chromium.org/2632633006/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2632633006/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:996: // FrameOwner in the parent via the child's current RenderFrame at any time. On 2017/01/18 15:10:55, clamy wrote: > Does this mean that if we start a request for which we want to collapse the > frame, we will collapse the current frame, even before we actually get a > response for the request? Yep, that's the plan. https://codereview.chromium.org/2632633006/diff/20001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/2632633006/diff/20001/content/common/frame_me... content/common/frame_messages.h:746: IPC_MESSAGE_ROUTED1(FrameMsg_CollapseFrameOwner, bool /* collapsed */) On 2017/01/18 15:10:55, clamy wrote: > Wouldn't is be easier to send the IPC to the parent instead, telling it to > collapse a particular child? (Note sure if doable though). Theoretically, we could use |unique_name|. This would also mean that we never have to send IPCs to proxies. @Nasko, are unique names reliable enough for this purpose? https://codereview.chromium.org/2632633006/diff/20001/content/public/browser/... File content/public/browser/navigation_throttle.h (right): https://codereview.chromium.org/2632633006/diff/20001/content/public/browser/... content/public/browser/navigation_throttle.h:45: BLOCK_REQUEST_AND_COLLAPSE, On 2017/01/18 15:10:55, clamy wrote: > I'm wondering whether we should add a new ThrottleCheckResult vs providing a > method on the NavigationHandle or the NavigationThrottle to collapse the frame. > The NavigationThrottle would return an existing result in that case. I am fine with either :-). @Nasko, WDYT?
I would've thought we can implement this as a network error code and we don't have to touch much of the code outside of Blink. I'm curious why does the FrameTreeNode need to know and keep the state of whether it is collapsed or not. https://codereview.chromium.org/2632633006/diff/60001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2632633006/diff/60001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:361: bool is_frame_owner_collapsed_; Why does the browser process need to know this? https://codereview.chromium.org/2632633006/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2632633006/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1006: // parent's site instance until the provisional load is committed, but the old nit: SiteInstance
> I would've thought we can implement this as a network error code and we don't > have to touch much of the code outside of Blink. I'm curious why does the > FrameTreeNode need to know and keep the state of whether it is collapsed or not. We have previously concluded that, in the spirit of not trusting attacker controlled processes, it makes more sense for the browser side to control collapsing. Implementing this purely as a network error code requires trusting the child RenderFrame to obediently interpret the error code and request its parent to collapse it. This leaves us with the option of directly talking to the FrameOwner. Why FrameTreeNode needs to persist state: I think it makes sense to make `collapsed` not a permanent state, but something that's roughly a function of the last committed URL, meaning we uncollapse the frame on the next successful proper navigation. Storing the state on FrameTreeNode seemed to be a convenient way to achieve this and: 1.) Avoid sending an "FrameMsg_Uncollapse" on every navigation, 2.) Avoid flickering (uncollapsing a frame just to collapse it a bit later during the navigation), 3.) Avoid getting confused by error page navigations. But I don't have strong arguments here, I'm open to alternatives. :-)
The CQ bit was checked by engedy@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
@Nasko, what do you think?
On 2017/01/19 01:02:07, engedy wrote: > > I would've thought we can implement this as a network error code and we don't > > have to touch much of the code outside of Blink. I'm curious why does the > > FrameTreeNode need to know and keep the state of whether it is collapsed or > not. > > We have previously concluded that, in the spirit of not trusting attacker > controlled processes, it makes more sense for the browser side to control > collapsing. Implementing this purely as a network error code requires trusting > the child RenderFrame to obediently interpret the error code and request its > parent to collapse it. This leaves us with the option of directly talking to the > FrameOwner. Good point on not trusting the renderer. That totally makes sense. > Why FrameTreeNode needs to persist state: I think it makes sense to make > `collapsed` not a permanent state, but something that's roughly a function of > the last committed URL, meaning we uncollapse the frame on the next successful > proper navigation. Storing the state on FrameTreeNode seemed to be a convenient > way to achieve this and: > 1.) Avoid sending an "FrameMsg_Uncollapse" on every navigation, Doesn't a successful commit imply that we restore the frame to uncollapsed state? If so, we can just do that in the renderer without having to send an explicit IPC. > 2.) Avoid flickering (uncollapsing a frame just to collapse it a bit later > during the navigation), What sequence of events will cause this flicker? If the frame was collapsed and navigated again to a blocked URL, the navigation will be blocked and nothing will uncollapse the frame. If the frame was displayed, then it will be collapsed. The only case I can see for flicker is to have a frame be added with explicit "about:blank" URL and then navigated to blocked URL. I don't think we can actually do something about it without breaking the assumptions of the web. > 3.) Avoid getting confused by error page navigations. How would that happen? > But I don't have strong arguments here, I'm open to alternatives. :-) Having looked over the CL again, I think it makes a lot of sense. If we can avoid storing the collapsed state on the FrameTreeNode, that will be ideal : ). Have you looked at making that be a property on FrameOwnerProperties?
https://codereview.chromium.org/2632633006/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2632633006/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_browsertest.cc:898: // frame. Why this change in comment? git cl format?
> > 1.) Avoid sending an "FrameMsg_Uncollapse" on every navigation, > > Doesn't a successful commit imply that we restore the frame to uncollapsed > state? If so, we can just do that in the renderer without having to send an > explicit IPC. If we do not want to trust the renderer with the collapsing, why can we trust it with the uncollapsing? If we don't, that rules out triggering uncollapsing from the navigated frame's renderer. > > 2.) Avoid flickering (uncollapsing a frame just to collapse it a bit later > > during the navigation), > > What sequence of events will cause this flicker? If the frame was collapsed and > navigated again to a blocked URL, the navigation will be blocked and nothing > will uncollapse the frame. If the frame was displayed, then it will be > collapsed. The only case I can see for flicker is to have a frame be added with > explicit "about:blank" URL and then navigated to blocked URL. I don't think we > can actually do something about it without breaking the assumptions of the web. > > > 3.) Avoid getting confused by error page navigations. > > How would that happen? The argument here is that we cannot restore before commit time, otherwise the frame may be shown for a short time between two consecutive blocked navigations. By commit time, however, the initiator of the navigation no longer has enough context to confidently uncollapse, e.g., loading an error page in a target frame also appears as a successful commit to the initiator. > > But I don't have strong arguments here, I'm open to alternatives. :-) > > Having looked over the CL again, I think it makes a lot of sense. If we can > avoid storing the collapsed state on the FrameTreeNode, that will be ideal : ). > Have you looked at making that be a property on FrameOwnerProperties? Currently, all FrameOwnerProperties are propagated in the direction FrameOwner -> subframe. The collapse message goes the other way around. Consistent two-way property updates are tough. Instead, we could let the FrameMsg_Collapse trigger a FrameHostMsg_DidChangeFrameOwnerProperties sent back, and only update the |is_collapsed| on the browser side / in proxies in response to this. The only real alternative to the current approach seems to be accepting that the frame might not be collapsed in case of a compromised renderer (the load would still be blocked in the NavigationThrottle), in which case we could trigger collapsing/restoring from RenderFrameImpl. Even with this solution, we would still need to either send an uncollapse message to RemoteFrameOwners on each committed load, or take care of propagating the |is_collapsed| state to RemoteFrameOwners (so that when the next navigation transfers after a blocked navigation, the target frame will know to restore its owner). What do you think?
On 2017/01/25 21:15:36, engedy wrote: > > > 1.) Avoid sending an "FrameMsg_Uncollapse" on every navigation, > > > > Doesn't a successful commit imply that we restore the frame to uncollapsed > > state? If so, we can just do that in the renderer without having to send an > > explicit IPC. > > If we do not want to trust the renderer with the collapsing, why can we trust it > with the uncollapsing? If we don't trust the renderer to render content for us, then why even render? :) We should not trust it for security decisions that the browser can do, but we still rely on it to turn instructions into pixels, right? > If we don't, that rules out triggering uncollapsing from > the navigated frame's renderer. Do you have a threat model of what we are trying to protect against? > > > 2.) Avoid flickering (uncollapsing a frame just to collapse it a bit later > > > during the navigation), > > > > What sequence of events will cause this flicker? If the frame was collapsed > and > > navigated again to a blocked URL, the navigation will be blocked and nothing > > will uncollapse the frame. If the frame was displayed, then it will be > > collapsed. The only case I can see for flicker is to have a frame be added > with > > explicit "about:blank" URL and then navigated to blocked URL. I don't think we > > can actually do something about it without breaking the assumptions of the > web. > > > > > 3.) Avoid getting confused by error page navigations. > > > > How would that happen? > > The argument here is that we cannot restore before commit time, otherwise the > frame may be shown for a short time between two consecutive blocked navigations. If the two navigations are consecutive and both are blocked, then there is no reason to show the frame at all. > By commit time, however, the initiator of the navigation no longer has enough > context to confidently uncollapse, e.g., loading an error page in a target frame > also appears as a successful commit to the initiator. Indeed. And we do want to show error pages for all other errors, right? > > > But I don't have strong arguments here, I'm open to alternatives. :-) > > > > Having looked over the CL again, I think it makes a lot of sense. If we can > > avoid storing the collapsed state on the FrameTreeNode, that will be ideal : > ). > > Have you looked at making that be a property on FrameOwnerProperties? > > Currently, all FrameOwnerProperties are propagated in the direction FrameOwner > -> subframe. The collapse message goes the other way around. Consistent two-way > property updates are tough. Instead, we could let the FrameMsg_Collapse trigger > a FrameHostMsg_DidChangeFrameOwnerProperties sent back, and only update the > |is_collapsed| on the browser side / in proxies in response to this. > > The only real alternative to the current approach seems to be accepting that the > frame might not be collapsed in case of a compromised renderer (the load would > still be blocked in the NavigationThrottle) Let's be precise about which renderer is compromised? The one that hosts the frame navigating to the blocked content or the parent of that frame? > , in which case we could trigger collapsing/restoring from RenderFrameImpl. We can always do that for frames that are in the same process and save ourselves the need to send an extra IPC message on commit. The browser knows when a frame and its parent are in the same process, so that the IPC can be skipped. In the renderer, when we commit, if the parent is in the same process, it is trivial to notify it to uncollapse. For collapsing, we always send the IPC to the parent frame, regardless. Now that I've thought more through it, storing this bit on the replicated state doesn't make as much sense. I still however don't see why can't we skip keeping the state on the browser side, unless it is only to allow us to not send duplicate IPCs if we are already in that state. > Even with this solution, we would > still need to either send an uncollapse message to RemoteFrameOwners on each > committed load, or take care of propagating the |is_collapsed| state to > RemoteFrameOwners (so that when the next navigation transfers after a blocked > navigation, the target frame will know to restore its owner). > > What do you think? Maybe we can whiteboard this, it might be more efficient than code review back/forth :).
Agreed that this discussion is becoming difficult, I'll write up these alternatives in the design doc.
The CQ bit was checked by engedy@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.
engedy@chromium.org changed reviewers: + dcheng@chromium.org
@Nasko, @Camille, please take another look. (Summary of off-line discussions below.) @Daniel, could you please review the Blink parts? https://codereview.chromium.org/2632633006/diff/20001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2632633006/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:146: // no effect unless the child frame is embedded using an <iframe> element. On 2017/01/18 15:10:55, clamy wrote: > It's not very clear to me while reading this description what this means. Does > it mean that the parent frame won't show or this one? I have done some wordsmithing, please take another look. https://codereview.chromium.org/2632633006/diff/20001/content/common/frame_me... File content/common/frame_messages.h (right): https://codereview.chromium.org/2632633006/diff/20001/content/common/frame_me... content/common/frame_messages.h:746: IPC_MESSAGE_ROUTED1(FrameMsg_CollapseFrameOwner, bool /* collapsed */) On 2017/01/18 23:37:14, engedy wrote: > On 2017/01/18 15:10:55, clamy wrote: > > Wouldn't is be easier to send the IPC to the parent instead, telling it to > > collapse a particular child? (Note sure if doable though). > > Theoretically, we could use |unique_name|. This would also mean that we never > have to send IPCs to proxies. @Nasko, are unique names reliable enough for this > purpose? @Nasko, what do think about this? To me, sending the IPC to the parent and then routing it to the right WebFrame child based on unique name sounds a bit like duplicating the main routing logic, but I am not strongly against this. Also, now that we settled on storing the state on the child FrameTreeNode, would be a bit awkward to pipe it through the parent RFManager. https://codereview.chromium.org/2632633006/diff/20001/content/public/browser/... File content/public/browser/navigation_throttle.h (right): https://codereview.chromium.org/2632633006/diff/20001/content/public/browser/... content/public/browser/navigation_throttle.h:45: BLOCK_REQUEST_AND_COLLAPSE, On 2017/01/18 23:37:14, engedy wrote: > On 2017/01/18 15:10:55, clamy wrote: > > I'm wondering whether we should add a new ThrottleCheckResult vs providing a > > method on the NavigationHandle or the NavigationThrottle to collapse the > frame. > > The NavigationThrottle would return an existing result in that case. > > I am fine with either :-). @Nasko, WDYT? On second thought, I think it makes for a better and harder-to-misuse API to bundle collapsing with blocking the navigation. Collapsing without blocking makes little sense to me, and would just waste of resources. https://codereview.chromium.org/2632633006/diff/60001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2632633006/diff/60001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:361: bool is_frame_owner_collapsed_; On 2017/01/19 00:11:46, nasko (out until Feb 27th) wrote: > Why does the browser process need to know this? As discussed offline, this is required to avoid needing to send `uncollapse` IPCs on every allowed navigation. Storing it on FrameOwnerProperties is not an option either, because the current unidirectional FOP propagation logic requires the HTMLIFrameElement to update it, but with this round trip we no longer have strong enough consistency guarantees to reliably tell on the browser side if the `collapsed` actually changed (and avoid sending IPCs if it didn't). I now believe this lack of consistency actually rules out renderer-side solutions too if we want to optimize sending IPCs based on whether there was an actual `collapsed` state transition; because the update lag affects the copy on RemoteFrameOwner too. https://codereview.chromium.org/2632633006/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2632633006/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_manager.cc:1006: // parent's site instance until the provisional load is committed, but the old On 2017/01/19 00:11:46, nasko (out until Feb 27th) wrote: > nit: SiteInstance Done.
The CQ bit was checked by engedy@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.
Thanks! A few questions below. https://codereview.chromium.org/2632633006/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2632633006/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:812: "a.com", "/cross-site/baz.com/title2.html")); It's not clear from these urls that they will redirect. Is that the case? https://codereview.chromium.org/2632633006/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:981: VerifyFormRequestContextType) { Could you also update this test to check that we get the expected error code on the throttle? Thanks!
The CQ bit was checked by engedy@chromium.org to run a CQ dry run
https://codereview.chromium.org/2632633006/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2632633006/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:812: "a.com", "/cross-site/baz.com/title2.html")); On 2017/02/21 15:12:49, clamy wrote: > It's not clear from these urls that they will redirect. Is that the case? Yes, the testing throttle would only return BLOCK_AND_COLLAPSE from WillRedirectRequest() in the iteration when |block_on_redirect| is set, so the test would inevitably fail otherwise. The `/cross-site/' prefix is called the `HandleCrossSiteRedirect` handler, so I think it should be okay to use. Especially so given it is important here to make these cross-site navigations, and using the `/server-redirect?' handler to change the host is awkward because we would need to manually supply the correct port the embedded server is running on. https://codereview.chromium.org/2632633006/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:981: VerifyFormRequestContextType) { On 2017/02/21 15:12:49, clamy wrote: > Could you also update this test to check that we get the expected error code on > the throttle? Thanks! I'm somewhat puzzled by this comment, could you please elaborate?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2632633006/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2632633006/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:812: "a.com", "/cross-site/baz.com/title2.html")); On 2017/02/22 13:14:58, engedy (slow) wrote: > On 2017/02/21 15:12:49, clamy wrote: > > It's not clear from these urls that they will redirect. Is that the case? > > Yes, the testing throttle would only return BLOCK_AND_COLLAPSE from > WillRedirectRequest() in the iteration when |block_on_redirect| is set, so the > test would inevitably fail otherwise. > > The `/cross-site/' prefix is called the `HandleCrossSiteRedirect` handler, so I > think it should be okay to use. Especially so given it is important here to make > these cross-site navigations, and using the `/server-redirect?' handler to > change the host is awkward because we would need to manually supply the correct > port the embedded server is running on. Acknowledged. https://codereview.chromium.org/2632633006/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:981: VerifyFormRequestContextType) { On 2017/02/22 13:14:58, engedy (slow) wrote: > On 2017/02/21 15:12:49, clamy wrote: > > Could you also update this test to check that we get the expected error code > on > > the throttle? Thanks! > > I'm somewhat puzzled by this comment, could you please elaborate? Sorry I posted this comment on the wrong test. I meant the thing above, but with ErrorCodeOnThrottleCancelNavigation (ie check that when the Throttle returns this new result, we have an appropriate error code on the NavigationHandle).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by engedy@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...
https://codereview.chromium.org/2632633006/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2632633006/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:981: VerifyFormRequestContextType) { On 2017/02/22 13:18:29, clamy wrote: > On 2017/02/22 13:14:58, engedy (slow) wrote: > > On 2017/02/21 15:12:49, clamy wrote: > > > Could you also update this test to check that we get the expected error code > > on > > > the throttle? Thanks! > > > > I'm somewhat puzzled by this comment, could you please elaborate? > > Sorry I posted this comment on the wrong test. I meant the thing above, but with > ErrorCodeOnThrottleCancelNavigation (ie check that when the Throttle returns > this new result, we have an appropriate error code on the NavigationHandle). Done, thanks for requesting this, this uncovered a bug in this CL, which I have fixed. Unfortunately, it also uncovered a more fundamental issue present when PlzNavigate is disabled. The ResourceMsg_ReceivedRedirect message is never sent if the NavigationThrottle blocks the load, only ResourceMsg_RequestComplete is sent. Hence the renderer never learns of the redirect, so the provisional load fails on the renderer side with the pre-redirect URL. This means that the error page is also committed under the pre-redirect URL, which causes a mismatch in TakeNavigationHandleForCommit, because the NavigationHandle already has the post-redirect URL. What do you suggest?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
On 2017/02/22 14:34:06, engedy (slow) wrote: > https://codereview.chromium.org/2632633006/diff/100001/content/browser/frame_... > File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): > > https://codereview.chromium.org/2632633006/diff/100001/content/browser/frame_... > content/browser/frame_host/navigation_handle_impl_browsertest.cc:981: > VerifyFormRequestContextType) { > On 2017/02/22 13:18:29, clamy wrote: > > On 2017/02/22 13:14:58, engedy (slow) wrote: > > > On 2017/02/21 15:12:49, clamy wrote: > > > > Could you also update this test to check that we get the expected error > code > > > on > > > > the throttle? Thanks! > > > > > > I'm somewhat puzzled by this comment, could you please elaborate? > > > > Sorry I posted this comment on the wrong test. I meant the thing above, but > with > > ErrorCodeOnThrottleCancelNavigation (ie check that when the Throttle returns > > this new result, we have an appropriate error code on the NavigationHandle). > > Done, thanks for requesting this, this uncovered a bug in this CL, which I have > fixed. > > Unfortunately, it also uncovered a more fundamental issue present when > PlzNavigate is disabled. The ResourceMsg_ReceivedRedirect message is never sent > if the NavigationThrottle blocks the load, only ResourceMsg_RequestComplete is > sent. Hence the renderer never learns of the redirect, so the provisional load > fails on the renderer side with the pre-redirect URL. > > This means that the error page is also committed under the pre-redirect URL, > which causes a mismatch in TakeNavigationHandleForCommit, because the > NavigationHandle already has the post-redirect URL. > > What do you suggest? Yes we uncovered that as well when trying to move CSP to the browser process. arthursonzogni@ is working on a fix.
Sorry for the slow review. Can you help me understand why we want to hide the entire layout object instead of something like what https://codereview.chromium.org/2564633002/ does (which only hides the child layout objects)? https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:211: GURL expected_start_url = GURL()) const GURL& -- this should still work even with a default param https://codereview.chromium.org/2632633006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/RemoteFrameOwner.cpp (right): https://codereview.chromium.org/2632633006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/RemoteFrameOwner.cpp:38: // Always set by the browser through FrameOwnerProperties. Since this is a local-only method, it should only be on HTMLFrameOwnerElement.
Mostly nits for the code in content/. https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/frame_tree_node.cc:269: render_manager_.OnDidChangeCollapsedByClientState(collapsed); Does the "ByClientState" part of the name add any meaningful distinction? Do we care at the RFHM layer who initiated the collapsing? Can anyone else do it? https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:148: // Returns whether the frame's owner element in the embedder document is nit: s/embedder/parent/, it is a bit more web platform friendly name and "embedder" is very overloaded term. https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:150: // request by the client. What is "client" in this case? The content/ interface to chrome/? Or something else? https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:156: // effect for <iframe> owner elements. Are there owner elements for things other than <iframe>? Maybe worthwhile adding an example. https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:380: // owner elements. nit: This comment is almost identical to the other two. Maybe keep the full comment here and simplify the other two? https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:383: bool IsChildFrameCollapsed(Shell* shell, const char* element_id) { nit: std::string? https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:829: : NavigationThrottle::PROCEED; I wonder if this test can be simplified a bit by creating an array of tuples with the actual results and just iterating through that. It will remove one level of nesting in the for loops and the above few lines, which can be directly indexed into the array of tuples. https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:684: // DO NOT ADD CODE after this. The previous call to OnRequestFailed has nit: Empty line before the comment to keep it consistent with the block above. https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:442: void OnDidChangeCollapsedByClientState(bool collapsed); As noted in the other file, I think the "ByClientState" part doesn't add much meaning to the name and only makes it more confusing. https://codereview.chromium.org/2632633006/diff/140001/content/public/browser... File content/public/browser/navigation_throttle.h (right): https://codereview.chromium.org/2632633006/diff/140001/content/public/browser... content/public/browser/navigation_throttle.h:44: // be be returned from WillStartRequest or WillRedirectRequest. nit: Only one "be" needed.
The CQ bit was checked by engedy@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...
engedy@chromium.org changed reviewers: + alexmos@chromium.org
Addressed all comments, sorry for the delay. @Daniel, @Camille, could you please take another look? @Alex, this is what we have previously discussed, could you please take a look in lieu of Nasko? https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/frame_tree_node.cc:269: render_manager_.OnDidChangeCollapsedByClientState(collapsed); On 2017/03/01 20:08:04, nasko (out until Apr 17th) wrote: > Does the "ByClientState" part of the name add any meaningful distinction? Do we > care at the RFHM layer who initiated the collapsing? Can anyone else do it? I wanted to disambiguate between client- and content-initiated collapsing, but you are right that it is confusing at this layer. Removed. https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:148: // Returns whether the frame's owner element in the embedder document is On 2017/03/01 20:08:04, nasko (out until Apr 17th) wrote: > nit: s/embedder/parent/, it is a bit more web platform friendly name and > "embedder" is very overloaded term. Done. https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:150: // request by the client. On 2017/03/01 20:08:04, nasko (out until Apr 17th) wrote: > What is "client" in this case? The content/ interface to chrome/? Or something > else? Tried to clarify. https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:156: // effect for <iframe> owner elements. On 2017/03/01 20:08:04, nasko (out until Apr 17th) wrote: > Are there owner elements for things other than <iframe>? Maybe worthwhile adding > an example. Done. https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:380: // owner elements. On 2017/03/01 20:08:04, nasko (out until Apr 17th) wrote: > nit: This comment is almost identical to the other two. Maybe keep the full > comment here and simplify the other two? Actually, I made this one shorter. https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:211: GURL expected_start_url = GURL()) On 2017/02/28 05:36:22, dcheng wrote: > const GURL& -- this should still work even with a default param Done. https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:383: bool IsChildFrameCollapsed(Shell* shell, const char* element_id) { On 2017/03/01 20:08:04, nasko (out until Apr 17th) wrote: > nit: std::string? Done. https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:829: : NavigationThrottle::PROCEED; On 2017/03/01 20:08:04, nasko (out until Apr 17th) wrote: > I wonder if this test can be simplified a bit by creating an array of tuples > with the actual results and just iterating through that. It will remove one > level of nesting in the for loops and the above few lines, which can be directly > indexed into the array of tuples. Is this what you had in mind? https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:684: // DO NOT ADD CODE after this. The previous call to OnRequestFailed has On 2017/03/01 20:08:04, nasko (out until Apr 17th) wrote: > nit: Empty line before the comment to keep it consistent with the block above. Refactored away, N/A. https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:442: void OnDidChangeCollapsedByClientState(bool collapsed); On 2017/03/01 20:08:04, nasko (out until Apr 17th) wrote: > As noted in the other file, I think the "ByClientState" part doesn't add much > meaning to the name and only makes it more confusing. Done. https://codereview.chromium.org/2632633006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/RemoteFrameOwner.cpp (right): https://codereview.chromium.org/2632633006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/RemoteFrameOwner.cpp:38: // Always set by the browser through FrameOwnerProperties. On 2017/02/28 05:36:22, dcheng wrote: > Since this is a local-only method, it should only be on HTMLFrameOwnerElement. Done.
On 2017/02/28 05:36:23, dcheng wrote: > Sorry for the slow review. Can you help me understand why we want to hide the > entire layout object instead of something like what > https://codereview.chromium.org/2564633002/ does (which only hides the child > layout objects)? In our case the embedder of content/ requests collapsing the frame that would normally not be display:none. My understanding was that the frame would still take up space in the parent document unless we hid the entire layout object in the parent document?
Description was changed from ========== Implement NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE. NavigationThrottle::WillStartRequest and WillRedirectRequest can now return BLOCK_REQUEST_AND_COLLAPSE for navigations taking place in subframes. Returning this throttle result will block the navigation, and load an error page instead, similarly to BLOCK_REQUEST, but in addition, the child frame's owner element in the parent frame will also be collapsed, i.e. removed from the layout. The frame owner element is restored on the next successful (non-error-page) navigation commit. BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE. NavigationThrottle::WillStartRequest and WillRedirectRequest can now return BLOCK_REQUEST_AND_COLLAPSE for navigations taking place in subframes. Returning this throttle result will block the navigation, and load an error page instead, similarly to BLOCK_REQUEST, but in addition, the child frame's owner element in the parent frame will also be collapsed, i.e. removed from the layout. The frame owner element is restored on the next successful (non-error-page) navigation commit. BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
+Charlie FYI.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks reasonable overall, just a few minor comments. I'll defer to Camille on PlzNavigate changes and Daniel for Blink. https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:359: result == NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE); Please update the comments on CancelDeferredNavigation in navigation_handle.h to include BLOCK_REQUEST_AND_COLLAPSE https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:862: EXPECT_TRUE(IsChildFrameCollapsed(shell(), kChildFrameId)); Could we also check that the corresponding FrameTreeNode::is_collapsed() is correct, here and below? https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:919: // frame. nit: unnecessary churn? https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:676: result == NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE) Just a thought (not for this CL), but perhaps we could simplify these kinds of checks slightly with a helper like DoesThrottleResultBlockRequest(result) which would return true for both values (and be convenient should we ever need to add a third value). And same for CANCEL || CANCEL_AND_IGNORE, which is the other check that seems to occur very often. https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:947: RenderFrameProxyHost* proxy_to_parent = GetProxyToParent(); optional: you could just use GetRenderFrameProxyHost(parent_site_instance) here to avoid repeating the parent SiteInstance lookup. (I also find our name for GetProxyToParent pretty confusing :) ) https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:435: // embedder document should be collapsed, that is, remove from the layout as nit: s/remove/removed/ https://codereview.chromium.org/2632633006/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLIFrameElement.h (right): https://codereview.chromium.org/2632633006/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLIFrameElement.h:93: bool collapsedByClient; collapsed_by_client_ https://codereview.chromium.org/2632633006/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right): https://codereview.chromium.org/2632633006/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:138: void WebRemoteFrameImpl::Collapse(bool collapsed) { Given that this does the same thing as WebLocalFramImpl::Collapse, can we just implement it once in WebFrame::Collapse, which uses ToImplBase()->GetFrame()->Owner() to handle both cases?
The CQ bit was checked by engedy@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...
@Camille, @Daniel, could please take another look? https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:359: result == NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE); On 2017/04/11 06:00:29, alexmos wrote: > Please update the comments on CancelDeferredNavigation in navigation_handle.h to > include BLOCK_REQUEST_AND_COLLAPSE Done. https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:862: EXPECT_TRUE(IsChildFrameCollapsed(shell(), kChildFrameId)); On 2017/04/11 06:00:29, alexmos wrote: > Could we also check that the corresponding FrameTreeNode::is_collapsed() is > correct, here and below? Sure. Done. https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:919: // frame. On 2017/04/11 06:00:29, alexmos wrote: > nit: unnecessary churn? Looks like it, undone. https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:676: result == NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE) On 2017/04/11 06:00:29, alexmos wrote: > Just a thought (not for this CL), but perhaps we could simplify these kinds of > checks slightly with a helper like DoesThrottleResultBlockRequest(result) which > would return true for both values (and be convenient should we ever need to add > a third value). And same for CANCEL || CANCEL_AND_IGNORE, which is the other > check that seems to occur very often. Happy to implement this in a follow-up CL if Camille agrees. https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2632633006/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:947: RenderFrameProxyHost* proxy_to_parent = GetProxyToParent(); On 2017/04/11 06:00:29, alexmos wrote: > optional: you could just use GetRenderFrameProxyHost(parent_site_instance) here > to avoid repeating the parent SiteInstance lookup. (I also find our name for > GetProxyToParent pretty confusing :) ) Nice, done! https://codereview.chromium.org/2632633006/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLIFrameElement.h (right): https://codereview.chromium.org/2632633006/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLIFrameElement.h:93: bool collapsedByClient; On 2017/04/11 06:00:30, alexmos wrote: > collapsed_by_client_ Wops, old CL from before the reformat. Done. https://codereview.chromium.org/2632633006/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp (right): https://codereview.chromium.org/2632633006/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp:138: void WebRemoteFrameImpl::Collapse(bool collapsed) { On 2017/04/11 06:00:30, alexmos wrote: > Given that this does the same thing as WebLocalFramImpl::Collapse, can we just > implement it once in WebFrame::Collapse, which uses > ToImplBase()->GetFrame()->Owner() to handle both cases? Excellent suggestion, done!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by engedy@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 checked by engedy@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 checked by engedy@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.
https://codereview.chromium.org/2632633006/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right): https://codereview.chromium.org/2632633006/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:63: if (GetDocument().InStyleRecalc()) Can you help me understand why this would be called in the middle of style recalc? And in that case, why do we need to do something different?
@Camille, @Nasko, please take another look.
https://codereview.chromium.org/2632633006/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right): https://codereview.chromium.org/2632633006/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:63: if (GetDocument().InStyleRecalc()) On 2017/04/18 07:01:36, dcheng (OOO through May 2) wrote: > Can you help me understand why this would be called in the middle of style > recalc? And in that case, why do we need to do something different? I was just trying to be defensive here. While I have found no obvious pumping of message loops during style recalc, I wasn't 100% sure there is no "sneaky" pumping of message loops. :-) If you can confirm that that is indeed the intention, then this indeed can be simplified.
Looks fine, I'll defer to clamy@ to ensure PlzNavigate bits are good. Few nits in the meantime. https://codereview.chromium.org/2632633006/diff/260001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2632633006/diff/260001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:151: // request by the embedder (of the content/ layer). nit: Does it make a difference that it was requested by the embedder? I wonder if this is just extra context that we don't need in the comment. https://codereview.chromium.org/2632633006/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2632633006/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:773: // Fall through. nit: Indent this to match the call above. https://codereview.chromium.org/2632633006/diff/260001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/2632633006/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:437: // <iframe> owner elements. nit: s/<iframe>/frame/ as I think we use the same object and code for frameset, which is rarely used, but exists. https://codereview.chromium.org/2632633006/diff/260001/content/test/data/fram... File content/test/data/frame_tree/page_with_one_frame.html (right): https://codereview.chromium.org/2632633006/diff/260001/content/test/data/fram... content/test/data/frame_tree/page_with_one_frame.html:12: </script> Why this indent?
Sorry I didn't have time to look at this today. Will go through it tomorrow.
Thanks! It mostly looks good, a few comments below. https://codereview.chromium.org/2632633006/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2632633006/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:697: frame_tree_node()->SetCollapsed(false); What about SameDocument navigations? We should check that we don't get them if the frame is collapsed, and I don't think they should uncollapse the frame right? https://codereview.chromium.org/2632633006/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:740: case NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE: nit: I'd prefer if we were consistent when using fall through with what we're doing below: either fall through in both cases or not fall through in both cases. https://codereview.chromium.org/2632633006/diff/260001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2632633006/diff/260001/content/public/browser... content/public/browser/navigation_handle.h:239: // - NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE. I believe we can cancel the Navigation with more values than those. This should rather be: |result| should not be equal to NavigationThrottle::PROCEED or NavigationThrottle::DEFER.
The CQ bit was checked by engedy@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 checked by engedy@chromium.org to run a CQ dry run
Patchset #16 (id:300001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addressed most comments, ask follow-up questions for others. @Nasko, @Camille, please take another look. https://codereview.chromium.org/2632633006/diff/260001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2632633006/diff/260001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:151: // request by the embedder (of the content/ layer). On 2017/04/20 15:53:40, nasko wrote: > nit: Does it make a difference that it was requested by the embedder? I wonder > if this is just extra context that we don't need in the comment. I removed this remark from most places, but here I'd like to keep to emphasize that the frame is collapsed for reasons other than the parent document setting `display: none` on the frame (in which case the frame will be collapsed but this flag will not be set to `true`). https://codereview.chromium.org/2632633006/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2632633006/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:697: frame_tree_node()->SetCollapsed(false); On 2017/04/24 13:15:18, clamy wrote: > What about SameDocument navigations? We should check that we don't get them if > the frame is collapsed, and I don't think they should uncollapse the frame > right? If the frame is currently collapsed, it will be showing an error page with |kUnreachableWebDataURL|, so I don't think that a same page navigation is technically possible here unless we need to worry about weird race conditions. (Do we need to?) https://codereview.chromium.org/2632633006/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:740: case NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE: On 2017/04/24 13:15:18, clamy wrote: > nit: I'd prefer if we were consistent when using fall through with what we're > doing below: either fall through in both cases or not fall through in both > cases. Done. https://codereview.chromium.org/2632633006/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:773: // Fall through. On 2017/04/20 15:53:40, nasko wrote: > nit: Indent this to match the call above. I moved it after the call, clang-format does not let me indent that. https://codereview.chromium.org/2632633006/diff/260001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.h (right): https://codereview.chromium.org/2632633006/diff/260001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.h:437: // <iframe> owner elements. On 2017/04/20 15:53:40, nasko wrote: > nit: s/<iframe>/frame/ as I think we use the same object and code for frameset, > which is rarely used, but exists. They share a base class (HTMLFrameElementBase), but they are implemented as separate classes in Blink. For historical reasons, we ignore `display: none` for <frame> elements (starting with the initial revision [1] until today [2]), so I wanted to be consistent with that and implement collapsing for <iframe> elements only. I have added a test for good measure. [1]: https://chromium.googlesource.com/chromium/src/+blame/d869b93fe74f4d6cb2dd6f6... [2]: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML... https://codereview.chromium.org/2632633006/diff/260001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2632633006/diff/260001/content/public/browser... content/public/browser/navigation_handle.h:239: // - NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE. On 2017/04/24 13:15:18, clamy wrote: > I believe we can cancel the Navigation with more values than those. This should > rather be: > |result| should not be equal to NavigationThrottle::PROCEED or > NavigationThrottle::DEFER. There is a DCHECK in [3] that used to allow only the two CANCEL's, and now I added B_R_A_C to that list, hence the comment. [3]: https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_ha... https://codereview.chromium.org/2632633006/diff/260001/content/test/data/fram... File content/test/data/frame_tree/page_with_one_frame.html (right): https://codereview.chromium.org/2632633006/diff/260001/content/test/data/fram... content/test/data/frame_tree/page_with_one_frame.html:12: </script> On 2017/04/20 15:53:41, nasko wrote: > Why this indent? Wops, sorry, I was tinkering with this file and left a mess in it. Fixed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by engedy@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: linux_chromium_chromeos_ozone_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 engedy@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...
engedy@chromium.org changed reviewers: + csharrison@chromium.org
+Charlie, I wired this up to the SubframeNavigationFilteringThrottle, please review that part, and optionally FYI review the entire CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
the filtering throttle code LGTM, I only briefly looked at the rest. https://codereview.chromium.org/2632633006/diff/370001/components/subresource... File components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.cc (left): https://codereview.chromium.org/2632633006/diff/370001/components/subresource... components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.cc:51: // metrics. Also, cancel with BLOCK_AND_COLLAPSE when it is implemented. Please update this comment. https://codereview.chromium.org/2632633006/diff/370001/components/subresource... File components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.cc (right): https://codereview.chromium.org/2632633006/diff/370001/components/subresource... components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.cc:58: (content::IsBrowserSideNavigationEnabled() || !was_deferring_redirect) Nit: I think this would be a bit more readable pulled into a separate local variable.
@Nasko, @Daniel, @Camille: please see resolved comments and take another look. https://codereview.chromium.org/2632633006/diff/370001/components/subresource... File components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.cc (left): https://codereview.chromium.org/2632633006/diff/370001/components/subresource... components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.cc:51: // metrics. Also, cancel with BLOCK_AND_COLLAPSE when it is implemented. On 2017/04/28 22:53:19, Charlie (ooo-ish until may 2) wrote: > Please update this comment. Done. https://codereview.chromium.org/2632633006/diff/370001/components/subresource... File components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.cc (right): https://codereview.chromium.org/2632633006/diff/370001/components/subresource... components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.cc:58: (content::IsBrowserSideNavigationEnabled() || !was_deferring_redirect) On 2017/04/28 22:53:19, Charlie (ooo-ish until may 2) wrote: > Nit: I think this would be a bit more readable pulled into a separate local > variable. Done.
The CQ bit was checked by engedy@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.
content/ LGTM. I'd still say wait for clamy@'s stamp. https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2632633006/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:829: : NavigationThrottle::PROCEED; On 2017/04/10 14:36:41, engedy wrote: > On 2017/03/01 20:08:04, nasko (out until Apr 17th) wrote: > > I wonder if this test can be simplified a bit by creating an array of tuples > > with the actual results and just iterating through that. It will remove one > > level of nesting in the for loops and the above few lines, which can be > directly > > indexed into the array of tuples. > > Is this what you had in mind? Yes. Thanks! https://codereview.chromium.org/2632633006/diff/380001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2632633006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:958: // parent's SiteInstance until the provisional load is committed, but the old nit: s/the provisional load/the navigation/ https://codereview.chromium.org/2632633006/diff/380001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2632633006/diff/380001/content/public/browser... content/public/browser/navigation_handle.h:238: // - NavigationThrottle::CANCEL_AND_IGNORE. or nit: s/./,/
@Camille, @Daniel, please take a look. https://codereview.chromium.org/2632633006/diff/380001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager.cc (right): https://codereview.chromium.org/2632633006/diff/380001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager.cc:958: // parent's SiteInstance until the provisional load is committed, but the old On 2017/05/03 16:58:30, nasko wrote: > nit: s/the provisional load/the navigation/ Done. https://codereview.chromium.org/2632633006/diff/380001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/2632633006/diff/380001/content/public/browser... content/public/browser/navigation_handle.h:238: // - NavigationThrottle::CANCEL_AND_IGNORE. or On 2017/05/03 16:58:30, nasko wrote: > nit: s/./,/ Done.
The CQ bit was checked by engedy@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.
Sorry I did not have time to review this before going ooo for a week, so I'm trusting nasko's review.
@Daniel, friendly ping. There is also one specific question for you above (search for `defensive` in comments).
Sorry for the review latency, feel free to ping more aggressively in the future. https://codereview.chromium.org/2632633006/diff/420001/components/subresource... File components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.h (right): https://codereview.chromium.org/2632633006/diff/420001/components/subresource... components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.h:44: bool is_redirect); It would be nice if this were an enum instead of a bool (it's not easy to tell from the function name what the bool means, so using bools ends up requiring extra comments anyway) https://codereview.chromium.org/2632633006/diff/420001/components/subresource... components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.h:45: void OnCalculatedLoadPolicy(bool was_deferring_redirect, LoadPolicy policy); Ditto https://codereview.chromium.org/2632633006/diff/420001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2632633006/diff/420001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:161: // called on sub-frames hosted in <frame>, <object>, and <embed> elements. Out of curiosity, why limited only to iframe? https://codereview.chromium.org/2632633006/diff/420001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2632633006/diff/420001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:740: // Collapsed frames show an error page with |kUnreachableWebDataURL|, so a Nit: "show" seems like a slight misnomer, as the frame is collapsed and has no layout tree associated with it. https://codereview.chromium.org/2632633006/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right): https://codereview.chromium.org/2632633006/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:164: Revert this, it's not a bad formatting cleanup, but there's no other relevant changes in this file. https://codereview.chromium.org/2632633006/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h (right): https://codereview.chromium.org/2632633006/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h:62: virtual void SetCollapsedByClient(bool) {} "client" is pretty vague. Maybe just remove "ByClient", as it's not really clear what client this refers to. https://codereview.chromium.org/2632633006/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right): https://codereview.chromium.org/2632633006/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:63: if (GetDocument().InStyleRecalc()) As this happens as a response to an IPC, I don't see how this can happen in the middle of style recalc, so let's just remove lines 63-64.
@Daniel, please take another look. @Camille, once you are back, and have some time, feel free to take a look. https://codereview.chromium.org/2632633006/diff/420001/components/subresource... File components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.h (right): https://codereview.chromium.org/2632633006/diff/420001/components/subresource... components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.h:44: bool is_redirect); On 2017/05/10 06:59:46, dcheng (in AEST) wrote: > It would be nice if this were an enum instead of a bool (it's not easy to tell > from the function name what the bool means, so using bools ends up requiring > extra comments anyway) Good point, done. https://codereview.chromium.org/2632633006/diff/420001/components/subresource... components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.h:45: void OnCalculatedLoadPolicy(bool was_deferring_redirect, LoadPolicy policy); On 2017/05/10 06:59:46, dcheng (in AEST) wrote: > Ditto Done. https://codereview.chromium.org/2632633006/diff/420001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2632633006/diff/420001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:161: // called on sub-frames hosted in <frame>, <object>, and <embed> elements. On 2017/05/10 06:59:46, dcheng (in AEST) wrote: > Out of curiosity, why limited only to iframe? For historical reasons, we ignore `display: none` for <frame> elements (starting with the initial revision [1] until today [2]), so I wanted to be consistent with that and implement collapsing for <iframe> elements only. Not sure what the original rationale was, but this makes some sense considering that a <frameset> represents a complete partitioning of a page, so collapsing one <frame> would affect all others in ill-defined ways. [1]: https://chromium.googlesource.com/chromium/src/+blame/d869b93fe74f4d6cb2dd6f6... [2]: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML... https://codereview.chromium.org/2632633006/diff/420001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2632633006/diff/420001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:740: // Collapsed frames show an error page with |kUnreachableWebDataURL|, so a On 2017/05/10 06:59:46, dcheng (in AEST) wrote: > Nit: "show" seems like a slight misnomer, as the frame is collapsed and has no > layout tree associated with it. Done. https://codereview.chromium.org/2632633006/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp (right): https://codereview.chromium.org/2632633006/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp:164: On 2017/05/10 06:59:46, dcheng (in AEST) wrote: > Revert this, it's not a bad formatting cleanup, but there's no other relevant > changes in this file. Done, sorry for not noticing this. https://codereview.chromium.org/2632633006/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h (right): https://codereview.chromium.org/2632633006/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h:62: virtual void SetCollapsedByClient(bool) {} On 2017/05/10 06:59:46, dcheng (in AEST) wrote: > "client" is pretty vague. Maybe just remove "ByClient", as it's not really clear > what client this refers to. Done. https://codereview.chromium.org/2632633006/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right): https://codereview.chromium.org/2632633006/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:63: if (GetDocument().InStyleRecalc()) On 2017/05/10 06:59:46, dcheng (in AEST) wrote: > As this happens as a response to an IPC, I don't see how this can happen in the > middle of style recalc, so let's just remove lines 63-64. Done and added a DCHECK.
@Daniel, friendly ping.
LGTM https://codereview.chromium.org/2632633006/diff/420001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2632633006/diff/420001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:161: // called on sub-frames hosted in <frame>, <object>, and <embed> elements. On 2017/05/15 13:35:51, engedy wrote: > On 2017/05/10 06:59:46, dcheng (in AEST) wrote: > > Out of curiosity, why limited only to iframe? > > For historical reasons, we ignore `display: none` for <frame> elements (starting > with the initial revision [1] until today [2]), so I wanted to be consistent > with that and implement collapsing for <iframe> elements only. > > Not sure what the original rationale was, but this makes some sense considering > that a <frameset> represents a complete partitioning of a page, so collapsing > one <frame> would affect all others in ill-defined ways. > > [1]: > https://chromium.googlesource.com/chromium/src/+blame/d869b93fe74f4d6cb2dd6f6... > [2]: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML... Probably makes sense to do this for <object> and <embed> though doesn't it? Otherwise, switching to either one of those would trivially circumvent this. I'm OK with poking at this in a followup, but it seems inconsistent to only do it for one type of embeddable frame.
Thanks for the reviews, everyone! https://codereview.chromium.org/2632633006/diff/420001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/2632633006/diff/420001/content/browser/frame_... content/browser/frame_host/frame_tree_node.h:161: // called on sub-frames hosted in <frame>, <object>, and <embed> elements. On 2017/05/18 09:35:55, dcheng (in AEST) wrote: > On 2017/05/15 13:35:51, engedy wrote: > > On 2017/05/10 06:59:46, dcheng (in AEST) wrote: > > > Out of curiosity, why limited only to iframe? > > > > For historical reasons, we ignore `display: none` for <frame> elements > (starting > > with the initial revision [1] until today [2]), so I wanted to be consistent > > with that and implement collapsing for <iframe> elements only. > > > > Not sure what the original rationale was, but this makes some sense > considering > > that a <frameset> represents a complete partitioning of a page, so collapsing > > one <frame> would affect all others in ill-defined ways. > > > > [1]: > > > https://chromium.googlesource.com/chromium/src/+blame/d869b93fe74f4d6cb2dd6f6... > > [2]: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTML... > > Probably makes sense to do this for <object> and <embed> though doesn't it? > Otherwise, switching to either one of those would trivially circumvent this. I'm > OK with poking at this in a followup, but it seems inconsistent to only do it > for one type of embeddable frame. That's a good point and I agree. Let me address that in a follow-up.
engedy@chromium.org changed reviewers: - alexmos@chromium.org, clamy@chromium.org
@Alex, Camille --> to CC. All right, here we go. Let's see if it still applies...
The CQ bit was checked by engedy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2632633006/#ps460001 (title: "Rebase.")
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_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
engedy@chromium.org changed reviewers: + bauerb@chromium.org
@Bernhard, could you please take a quick look at: chrome/browser/supervised_user/supervised_user_google_auth_navigation_throttle.cc
lgtm
The CQ bit was unchecked by engedy@chromium.org
The CQ bit was checked by engedy@chromium.org
The CQ bit was checked by engedy@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_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 engedy@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 engedy@chromium.org
The CQ bit was checked by engedy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, nasko@chromium.org, dcheng@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2632633006/#ps480001 (title: "Update SubframeNavigationFilteringThrottleTest.DelayMetrics")
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_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 engedy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, nasko@chromium.org, dcheng@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2632633006/#ps500001 (title: "Fix navigation transition type.")
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": 500001, "attempt_start_ts": 1495731893559130,
"parent_rev": "89f6d708d1be7d2c1cebaa74b11908740130f617", "commit_rev":
"55c66809818b2caaf33e6d9ba8295606331b6e09"}
CQ is committing da patch.
Bot data: {"patchset_id": 500001, "attempt_start_ts": 1495731893559130,
"parent_rev": "5fcf2f3d88764e920dc1fc35de4d24e9b5e71284", "commit_rev":
"6e2e0996275dd48dd4497f9c57e6370a423eb3ec"}
Message was sent while issue was closed.
Description was changed from ========== Implement NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE. NavigationThrottle::WillStartRequest and WillRedirectRequest can now return BLOCK_REQUEST_AND_COLLAPSE for navigations taking place in subframes. Returning this throttle result will block the navigation, and load an error page instead, similarly to BLOCK_REQUEST, but in addition, the child frame's owner element in the parent frame will also be collapsed, i.e. removed from the layout. The frame owner element is restored on the next successful (non-error-page) navigation commit. BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Implement NavigationThrottle::BLOCK_REQUEST_AND_COLLAPSE. NavigationThrottle::WillStartRequest and WillRedirectRequest can now return BLOCK_REQUEST_AND_COLLAPSE for navigations taking place in subframes. Returning this throttle result will block the navigation, and load an error page instead, similarly to BLOCK_REQUEST, but in addition, the child frame's owner element in the parent frame will also be collapsed, i.e. removed from the layout. The frame owner element is restored on the next successful (non-error-page) navigation commit. BUG=637415 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2632633006 Cr-Commit-Position: refs/heads/master@{#474735} Committed: https://chromium.googlesource.com/chromium/src/+/6e2e0996275dd48dd4497f9c57e6... ==========
Message was sent while issue was closed.
Committed patchset #26 (id:500001) as https://chromium.googlesource.com/chromium/src/+/6e2e0996275dd48dd4497f9c57e6...
Message was sent while issue was closed.
On 2017/05/25 18:59:27, commit-bot: I haz the power wrote: > Committed patchset #26 (id:500001) as > https://chromium.googlesource.com/chromium/src/+/6e2e0996275dd48dd4497f9c57e6... \o/! |
