Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2901643002:
Remove AreCrossProcessFramesPossible from NavigatorImpl::DidNavigate (Closed)
|
Created:
3 years, 7 months ago by alexmos Modified:
3 years, 7 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove AreCrossProcessFramesPossible from NavigatorImpl::DidNavigate.
BUG=725265
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2901643002
Cr-Commit-Position: refs/heads/master@{#473770}
Committed: https://chromium.googlesource.com/chromium/src/+/d340a11e259f0837320cdfd4e95a2c6b9efed721
Patch Set 1 #
Total comments: 2
Patch Set 2 : Charlie's nit #Messages
Total messages: 16 (11 generated)
Description was changed from ========== Remove AreCrossProcessFramesPossible from NavigatorImpl::DidNavigate BUG= ========== to ========== Remove AreCrossProcessFramesPossible from NavigatorImpl::DidNavigate BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by alexmos@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 ========== Remove AreCrossProcessFramesPossible from NavigatorImpl::DidNavigate BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Remove AreCrossProcessFramesPossible from NavigatorImpl::DidNavigate. BUG=725265 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
alexmos@chromium.org changed reviewers: + creis@chromium.org
Charlie, can you take a quick look? This cleans up DidNavigate to not consult AreCrossProcessFramesPossible and just call DidNavigateFrame for all frames. Not doing this was causing some problems for feature policy (and probably sandbox flags tracking in browser process as well) due to not calling CommitPendingFramePolicy() on Android -- see https://codereview.chromium.org/2883213002/#msg11. I don't think this is likely to touch issue 690229 (i.e., regress Android perf), but if so, it'll be good to find out. :)
On 2017/05/22 23:00:22, alexmos wrote: > Charlie, can you take a quick look? This cleans up DidNavigate to not consult > AreCrossProcessFramesPossible and just call DidNavigateFrame for all frames. > Not doing this was causing some problems for feature policy (and probably > sandbox flags tracking in browser process as well) due to not calling > CommitPendingFramePolicy() on Android -- see > https://codereview.chromium.org/2883213002/#msg11. I don't think this is likely > to touch issue 690229 (i.e., regress Android perf), but if so, it'll be good to > find out. :) Thanks! That's pretty interesting about the feature policy issue-- I'm glad that moving in this direction helps with it. Hopefully it's a no-op otherwise, since we shouldn't have pending or speculative RFHs for subframes when AreCrossProcessFramesPossible is false. LGTM. https://codereview.chromium.org/2901643002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2901643002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:541: FrameTreeNode* ftn = render_frame_host->frame_tree_node(); nit: frame_tree_node (to be consistent with naming here, where we tend to spell out the name)
The CQ bit was checked by alexmos@chromium.org to run a CQ dry run
The CQ bit was unchecked by alexmos@chromium.org
Thanks! https://codereview.chromium.org/2901643002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2901643002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigator_impl.cc:541: FrameTreeNode* ftn = render_frame_host->frame_tree_node(); On 2017/05/22 23:29:02, Charlie Reis wrote: > nit: frame_tree_node (to be consistent with naming here, where we tend to spell > out the name) Done.
The CQ bit was checked by alexmos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2901643002/#ps20001 (title: "Charlie's nit")
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": 20001, "attempt_start_ts": 1495496618429950,
"parent_rev": "e9928ffd65c7b5def5a8a5af659ab647f437e3b8", "commit_rev":
"d340a11e259f0837320cdfd4e95a2c6b9efed721"}
Message was sent while issue was closed.
Description was changed from ========== Remove AreCrossProcessFramesPossible from NavigatorImpl::DidNavigate. BUG=725265 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Remove AreCrossProcessFramesPossible from NavigatorImpl::DidNavigate. BUG=725265 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2901643002 Cr-Commit-Position: refs/heads/master@{#473770} Committed: https://chromium.googlesource.com/chromium/src/+/d340a11e259f0837320cdfd4e95a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/d340a11e259f0837320cdfd4e95a... |
