Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(258)

Issue 2411693003: Move blocking of top-level navigations to nested URLs with extension origins from non-extension pro… (Closed)

Created:
4 years, 2 months ago by jam
Modified:
4 years, 1 month ago
Reviewers:
alexmos, nasko
CC:
chromium-reviews, extensions-reviews_chromium.org, cbentzel+watch_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, chromium-apps-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Move blocking of top-level navigations to nested URLs with extension origins from non-extension processes to the extension navigation throttle so that it works with PlzNavigate. The information to figure this out isn't available on the IO thread, so instead of plumbing it through just do it on the UI thread. This fixes ProcessManagerBrowserTest.NestedURLNavigationsToExtensionAllowed with PlzNavigate. BUG=504347, 645028 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/419c0f16840f7c9cf0cd600da55c4db4bd30c58d Cr-Commit-Position: refs/heads/master@{#424937}

Patch Set 1 #

Total comments: 13

Patch Set 2 : review comments #

Total comments: 10

Patch Set 3 : remove assert #

Patch Set 4 : review comments #

Total comments: 5

Patch Set 5 : review nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+86 lines, -41 lines) Patch
M chrome/browser/chrome_content_browser_client.cc View 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/net/chrome_extensions_network_delegate.cc View 2 chunks +0 lines, -27 lines 0 comments Download
M content/browser/frame_host/navigation_handle_impl.h View 1 2 3 3 chunks +2 lines, -1 line 0 comments Download
M content/browser/frame_host/navigation_handle_impl.cc View 1 2 3 4 2 chunks +8 lines, -0 lines 0 comments Download
M content/browser/site_per_process_browsertest.cc View 1 2 3 4 2 chunks +30 lines, -0 lines 0 comments Download
M content/public/browser/navigation_handle.h View 1 2 3 2 chunks +6 lines, -0 lines 0 comments Download
M extensions/browser/extension_navigation_throttle.cc View 1 2 3 4 chunks +39 lines, -11 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 37 (26 generated)
jam
This change supercedes https://codereview.chromium.org/2401443002/. Per Nasko's suggestion, I moved the blocking to the navigation throttle. ...
4 years, 2 months ago (2016-10-11 19:09:06 UTC) #6
nasko
Some preliminary comments. Alex and I have some brainstorming to do, so we'll update you ...
4 years, 2 months ago (2016-10-12 17:00:35 UTC) #9
alexmos
https://codereview.chromium.org/2411693003/diff/1/content/browser/frame_host/navigation_request.cc File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2411693003/diff/1/content/browser/frame_host/navigation_request.cc#newcode302 content/browser/frame_host/navigation_request.cc:302: frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this); To double-check what happens (since I was confused ...
4 years, 2 months ago (2016-10-12 17:18:16 UTC) #10
jam
https://codereview.chromium.org/2411693003/diff/1/content/browser/frame_host/navigation_handle_impl.h File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2411693003/diff/1/content/browser/frame_host/navigation_handle_impl.h#newcode341 content/browser/frame_host/navigation_handle_impl.h:341: GURL site_url_; On 2016/10/12 17:00:35, nasko (slow) wrote: > ...
4 years, 2 months ago (2016-10-12 22:10:45 UTC) #20
alexmos
Thanks, Nasko and I looked at this together, and I'm writing up the combined comments ...
4 years, 2 months ago (2016-10-12 23:13:29 UTC) #23
jam
https://codereview.chromium.org/2411693003/diff/40001/content/browser/frame_host/navigation_handle_impl.cc File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2411693003/diff/40001/content/browser/frame_host/navigation_handle_impl.cc#newcode97 content/browser/frame_host/navigation_handle_impl.cc:97: frame_tree_node_->render_manager()->current_frame_host(); On 2016/10/12 23:13:28, alexmos wrote: > nit: This ...
4 years, 2 months ago (2016-10-13 00:14:46 UTC) #26
alexmos
Thanks, LGTM with nits. https://codereview.chromium.org/2411693003/diff/80001/content/browser/frame_host/navigation_handle_impl.cc File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2411693003/diff/80001/content/browser/frame_host/navigation_handle_impl.cc#newcode17 content/browser/frame_host/navigation_handle_impl.cc:17: #include "content/browser/frame_host/render_frame_host_manager.h" nit: is this ...
4 years, 2 months ago (2016-10-13 00:46:27 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2411693003/100001
4 years, 2 months ago (2016-10-13 01:15:33 UTC) #32
commit-bot: I haz the power
Committed patchset #5 (id:100001)
4 years, 2 months ago (2016-10-13 02:09:41 UTC) #34
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/419c0f16840f7c9cf0cd600da55c4db4bd30c58d Cr-Commit-Position: refs/heads/master@{#424937}
4 years, 2 months ago (2016-10-13 02:11:57 UTC) #36
jam
4 years, 2 months ago (2016-10-13 03:12:28 UTC) #37
Message was sent while issue was closed.
https://codereview.chromium.org/2411693003/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2411693003/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:17: #include
"content/browser/frame_host/render_frame_host_manager.h"
On 2016/10/13 00:46:27, alexmos wrote:
> nit: is this still necessary?

Done.

https://codereview.chromium.org/2411693003/diff/80001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2411693003/diff/80001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:8350: class
NavigatigationHandleWatcher : public WebContentsObserver {
On 2016/10/13 00:46:27, alexmos wrote:
> nit: s/Navigatigation/Navigation/

Done.

Powered by Google App Engine
This is Rietveld 408576698