Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1269813002:
Add a NavigationThrottle to the public content/ interface (Closed)
|
Created:
5 years, 4 months ago by clamy Modified:
5 years, 1 month ago Reviewers:
Avi (use Gerrit), mnaganov (inactive), sky, ncarter (slow), carlosk, davidben, asargent_no_longer_on_chrome, nasko, Fabrice (no longer in Chrome), Charlie Reis CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, creis+watch_chromium.org, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@navigation-api Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a NavigationThrottle to the public content/ interface
This CL adds a NavigationThrottle class to the public content/ interface. A NavigationThrottle is used to control the flow of navigations. It lives entirely on the UI thread. Eventually, all components that want the functionality of a ResourceThrottle for navigations should transition to a NavigationThrottle: the new architecture for navigations (browser-side navigation) will not support ResourceThrottles for main resources load. See https://docs.google.com/document/d/1ICLLQoC9EsZ-bWH4ZKRhPCIoZKn6pOj02SlGl6SKH6Y/edit?pli=1#heading=h.fmxjmgvbgg7x for the design doc.
This Cl also transition the InterceptNavigationresourceThrottle to the Navigationthrottle model.
BUG=504347
Committed: https://crrev.com/394057986ff5d00a841a976f3bd8d599603acaa1
Cr-Commit-Position: refs/heads/master@{#350092}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 116
Patch Set 6 : Unit tests support #Patch Set 7 : Now properly using the factories #
Total comments: 2
Patch Set 8 : Addressed comments #
Total comments: 35
Patch Set 9 : Rebase #Patch Set 10 : Addressed comments #
Total comments: 53
Patch Set 11 : Rebase #
Total comments: 23
Patch Set 12 : Addressed comments #
Total comments: 4
Patch Set 13 : Removed TestNavigationHandle + pointer to WebContents #
Total comments: 23
Patch Set 14 : Addressed comments #
Total comments: 4
Patch Set 15 : Rebase + addressed comments #Patch Set 16 : Fixed compilation error on Android #Patch Set 17 : Rebase + android_webview + Linux compilation error #Patch Set 18 : Rebase on https://codereview.chromium.org/1312213010/ #Messages
Total messages: 78 (21 generated)
clamy@chromium.org changed reviewers: + carlosk@chromium.org, davidben@chromium.org, fdegans@chromium.org, nasko@chromium.org
@nasko, davidben: PTAL @carlosk, fdegans: FYI This is a first version of the NavigationThrottle patch, which passes the browser_tests for the PlatformAppUrlRedirector (the use case transitioned here). Note that I had to put the registration of NavigationThrottles in ContentBrowserClient. I am not terribly happy about that, but putting it in WebContentsDelegate did not work (we end up having ExtensionHost as our delegate in certain cases which cannot access chrome/ as per the DEPS file). Also note that in this version, NavigationThrottles perform their checks synchronously on the UI thread. If it turns out that some throttles need to do asynchronous checks, I'll add the functionality later.
Thanks for continuing these navigation changes. https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... File chrome/browser/apps/app_url_redirector.cc (left): https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:40: DCHECK_CURRENTLY_ON(BrowserThread::UI); Can this method now be called from other threads beyond the UI thread? https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... File chrome/browser/apps/app_url_redirector.cc (right): https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:84: return throttle.Pass(); nit: IIRC you can return nullptr for scoped_ptr return types as they are automatically converted. I think that ways makes the intent to return null clearer than how it's done now. https://codereview.chromium.org/1269813002/diff/80001/components/navigation_i... File components/navigation_interception/intercept_navigation_throttle.h (right): https://codereview.chromium.org/1269813002/diff/80001/components/navigation_i... components/navigation_interception/intercept_navigation_throttle.h:30: const NavigationParams& /* navigation_params */)> For a future change we should consider replacing this to directly accept the NavigationHandle instead of doing the conversion to NavigationParams. Maybe add a TODO? https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:140: sanitized_referrer_.url = new_referrer_url; Why setting this member URL here when the next line replaces the whole value? https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:58: // stack, to let it know whether the navigation should go on or be stopped. I don't understand how this comments fits here. https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:58: : frame_tree_node->render_manager()->current_frame_host(); This next-RFH logic should live inside RFHM. We've been in the verge of adding it there for some time already and this usage looks like the perfect use case. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... File content/public/browser/navigation_throttle.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_throttle.h:13: // A NavigationThrottle tracks is used to interact with a navigation on the UI /is used to interact/and allows interaction/ https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... File content/public/browser/web_contents_delegate.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/web_contents_delegate.h:16: #include "content/public/browser/navigation_handle.h" It doesn't seem this include is needed.
Super excited about this! Lots of comments initially, but it actually is in a pretty good shape. Adding avi@ as another pair of content/ API eyes to look it over. https://codereview.chromium.org/1269813002/diff/80001/components/navigation_i... File components/navigation_interception/intercept_navigation_throttle.h (right): https://codereview.chromium.org/1269813002/diff/80001/components/navigation_i... components/navigation_interception/intercept_navigation_throttle.h:34: content::WebContents* web_contents, I'd put web_contents as the first parameter, as it is the scope in which navigation_handle exists. https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:57: DCHECK(state_ != DID_START) nit: CHECK? Also in the methods below. https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:58: : frame_tree_node->render_manager()->current_frame_host(); On 2015/08/28 16:40:24, carlosk wrote: > This next-RFH logic should live inside RFHM. We've been in the verge of adding > it there for some time already and this usage looks like the perfect use case. This code seems a bit fragile. What happens if there is a speculative RFH and the url passed in doesn't match the SiteInstance for that one? The current code doesn't handle this case, does it? https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:247: common_params_.url, FilterURL(common_params_.url, frame_tree_node_), FilterURL should be a called separately and possibly kill the renderer if it fails. Even if we don't do the kill now, the code will be easier to refactor later on. https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:164: url, filtered_url, is_main_frame, delegate_)); Why do we need both? We never keep the two around in today's navigation logic, do we? https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:38: if (render_frame_host) { This doesn't seem right. Before we have gotten the response, we don't have RFH, do we? https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:80: : request_(request), weak_ptr_factory_(this) {} Each member initialization on new line, no? Or has clang-format changed its mind? https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:84: void NavigationResourceThrottle::WillStartRequest(bool* defer) { Let's add DCHECKs for which thread each function is meant to be called. It serves to both catch issues and as self-documentation. https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:88: Is it possible to know on the IO thread whether there are any throttles registered and skip the thread hop if not? I wonder if such an optimization is necessary. https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:102: render_frame_id, request_->method() == "POST", Do we have render_frame_id at the time of sending the network request? We are trying to divorce the navigation resource request from the renderer, right? https://codereview.chromium.org/1269813002/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:516: void AddNavigationThrottles(NavigationHandle* navigation_handle) override; Might be worthwhile adding a "for_main_frame_only" type parameter. Most features might not care about subframe navigations, so we shouldn't be installing their throttles for subframes. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/content_browser_client.h:656: // Allows the embedder to register NavigationThrottles for a navigation. nit: s/NavigationThrottles/one or more NavigationThrottle/ https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/content_browser_client.h:657: // NavigationThrottles are used to control the flow of a navigation on the UI nit: Singular NavigationThrottle. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/content_browser_client.h:659: // the order they were registered. The order is guaranteed for each embedder, but embedders order isn't, right? https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:34: virtual const GURL& GetValidatedURL() const = 0; Is there any reason to return non-validated URL? https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:37: // This reamins constant over the navigation. nit: navigation lifetime. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:47: // Whether the navigation is a post or not. This may change during the nit: s/post/POST/ https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:52: virtual const Referrer& GetSanitizedReferrer() const = 0; Since there is no other referrer that we are returning, why not just GetReferrer()? The comment should still say it is "sanitized", so it is clear to whoever is reading about the method. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:56: virtual bool HasUserGesture() const = 0; Usually, browser initiated navigations are in response to something coming from the user. Should that be factored into account? https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:61: // Whether the target URL cannot be handled by Chrome's internal protocol nit: s/Chrome/the browser/. Chrome is a branded version of Chromium, and we are the content layer, so we shouldn't be using Chrome directly. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... File content/public/browser/navigation_throttle.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_throttle.h:21: CANCEL_AND_IGNORE, Why not just "Cancel" instead of "Cancel and Ignore"? What additional meaning does "And Ignore" bring?
nasko@chromium.org changed reviewers: + avi@chromium.org
Actually adding avi@ : ).
https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2573: } Do we really want to do this for every WebContents everywhere? Do we at least want to make sure that it's a browser tab? https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:57: DCHECK(state_ != DID_START) On 2015/08/31 23:25:15, nasko wrote: > nit: CHECK? Also in the methods below. DCHECK is the usual Chromium style; why are you proposing CHECK? https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/content_browser_client.h:659: // the order they were registered. On 2015/08/31 23:25:15, nasko wrote: > The order is guaranteed for each embedder, but embedders order isn't, right? What do you mean by that question? There is always exactly one embedder. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:37: // This reamins constant over the navigation. typo: remains https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:42: // The following parameters are only available when the a network request is "when the network request" https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:43: // made for the navigation (or commit time if not network request is made). "or at commit time if no network request is made"
Some comments from a first pass. https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... File chrome/browser/apps/app_url_redirector.cc (right): https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:12: #include "chrome/browser/profiles/profile_io_data.h" No longer needed? https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:16: #include "content/public/browser/browser_thread.h" No longer needed? https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:18: #include "content/public/browser/render_view_host.h" No longer needed? https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:19: #include "content/public/browser/resource_request_info.h" No longer needed? https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:20: #include "content/public/browser/resource_throttle.h" No longer needed? https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:23: #include "extensions/browser/info_map.h" No longer needed? https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:28: using content::BrowserThread; No longer needed? https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:29: using content::ResourceRequestInfo; No longer needed? https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2573: } On 2015/09/01 16:38:47, Avi wrote: > Do we really want to do this for every WebContents everywhere? Do we at least > want to make sure that it's a browser tab? +1. If we can manage it, I think WebContentsDelegate seems a better place to latch this onto. As a bonus, that prerender block can just go away. Though I dunno if in practice we'll want a global one too. https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:345: InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); [This is probably for a separate CL, but... wow, talk about questionable logic. It just happens to hook into XHR user gestures and not, say, I tapped on a button. https://docs.google.com/document/d/1z2O07u5WoEtsLLUkx6RDjbVO5XrNwMJAQIBVvzRpE... ] On the plus side, I think is_prerendering check and the TODO can go away since they're not relevant for this call. Dunno if you want to add a TODO to revise this one though. https://codereview.chromium.org/1269813002/diff/80001/components/navigation_i... File components/navigation_interception/intercept_navigation_delegate.cc (right): https://codereview.chromium.org/1269813002/diff/80001/components/navigation_i... components/navigation_interception/intercept_navigation_delegate.cc:91: return scoped_ptr<InterceptNavigationThrottle>( Optional: This can be return make_scoped_ptr(new InterceptNavigationThrottle(...)); https://codereview.chromium.org/1269813002/diff/80001/components/navigation_i... File components/navigation_interception/intercept_navigation_throttle.cc (right): https://codereview.chromium.org/1269813002/diff/80001/components/navigation_i... components/navigation_interception/intercept_navigation_throttle.cc:5: #include "components/navigation_interception/intercept_navigation_throttle.h" [Wow, this file is so much smaller! :-D] https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:57: DCHECK(state_ != DID_START) I think this line doesn't have enough people commenting on it, so I'll add: Nit: DCHECK_NE (ditto below) https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:121: // Have each throttle be notified of the request. Nit: Perhaps "Notify each throttle of the request."? Slightly shorter or something. https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:122: for (auto it = throttles_.begin(); it != throttles_.end(); ++it) { (Is it possible to use a range-based for loop here, or does it being a ScopedVector cause you troubles?) https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:141: sanitized_referrer_ = Referrer::SanitizeForRequest(url_, sanitized_referrer_); I'm a little puzzled by this line. Wouldn't the referrer come pre-sanitized by the loader stack already? You're just trying to report on what the loader stack decided to use. Or is the problem that this hasn't happened yet and you need to repeat that logic? (I'm not that familiar with referrers.) https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:144: state_ = WILL_REDIRECT_REQUEST; Is anything actually sensitive to this state? You don't have asynchronous throttles, so you don't need to keep track of exactly where you are like that. https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:194: // TODO(clamy): pass the real value for |is_external_protocol|. Hrm. It seems to manage that, we'd need to either fire this until the network request has hit the IO thread (and so it becomes UI -> IO -> UI -> IO before starting the request), or we have a way to compute it on the UI thread, which means the OnRequestRedirect logic changes. I haven't looked closely, but this is only used by the Android Intents code, right? It seems some codepaths actually compute it from the URL in Java with UrlUtilities.isAcceptedScheme. Perhaps we should just switch it all to that and remove it from this API? (A little ugly to have the duplicated code like that, but maybe it's fine in this context? That code is at least top-level. I don't know what context it's used in.) https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:38: if (render_frame_host) { On 2015/08/31 23:25:15, nasko wrote: > This doesn't seem right. Before we have gotten the response, we don't have RFH, > do we? This is the pre-PlzNavigate codepath I believe, so we have an RFH and everything else. https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1444: // PlzNavigate: the thottle is unnecessary as communication with the UI Nit: throttle https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1449: resource_type == RESOURCE_TYPE_SUB_FRAME)) { IsResourceTypeFrame? https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/content_browser_client.h:661: WebContents* web_contents) {} It's a little odd that this API is called AddNavigationThrottles, but it's not really specific to navigation throttles. It seems to just be a thing that signals a particular point in the timeline (start of navigation or so). https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:81: scoped_ptr<NavigationThrottle> navigation_throttle) = 0; I wonder if this would be better done as the ResourceThrottle-style API where you have one change to staple all your NavigationThrottles. Thoughts? Right now you can add them at any point, which is kinda weird.
Responding to avi@'s question. https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:57: DCHECK(state_ != DID_START) On 2015/09/01 16:38:47, Avi wrote: > On 2015/08/31 23:25:15, nasko wrote: > > nit: CHECK? Also in the methods below. > > DCHECK is the usual Chromium style; why are you proposing CHECK? This is not a codepath that the renderer can induce us to go down, so it is purely bug in our implementation. As such, using a CHECK will allow us to get crash reports with status of what we got wrong (stack trace and some registers). DCHECKs should be used when a renderer process can cause us to hit it, otherwise we will create a denial-of-service condition where malicious page can continually crash the browser.
Thanks! PTAL at the new version, which includes unit test support. https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... File chrome/browser/apps/app_url_redirector.cc (left): https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:40: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2015/08/28 16:40:24, carlosk wrote: > Can this method now be called from other threads beyond the UI thread? No the class now resides strictly on the UI thread. https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... File chrome/browser/apps/app_url_redirector.cc (right): https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:12: #include "chrome/browser/profiles/profile_io_data.h" On 2015/09/01 21:55:17, David Benjamin wrote: > No longer needed? Done. https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:16: #include "content/public/browser/browser_thread.h" On 2015/09/01 21:55:17, David Benjamin wrote: > No longer needed? Done. https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:18: #include "content/public/browser/render_view_host.h" On 2015/09/01 21:55:17, David Benjamin wrote: > No longer needed? Done. https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:19: #include "content/public/browser/resource_request_info.h" On 2015/09/01 21:55:17, David Benjamin wrote: > No longer needed? Done. https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:20: #include "content/public/browser/resource_throttle.h" On 2015/09/01 21:55:17, David Benjamin wrote: > No longer needed? Done. https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:23: #include "extensions/browser/info_map.h" On 2015/09/01 21:55:17, David Benjamin wrote: > No longer needed? Done. https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:28: using content::BrowserThread; On 2015/09/01 21:55:17, David Benjamin wrote: > No longer needed? Done. https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:29: using content::ResourceRequestInfo; On 2015/09/01 21:55:17, David Benjamin wrote: > No longer needed? Done. https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/apps/app... chrome/browser/apps/app_url_redirector.cc:84: return throttle.Pass(); On 2015/08/28 16:40:24, carlosk wrote: > nit: IIRC you can return nullptr for scoped_ptr return types as they are > automatically converted. I think that ways makes the intent to return null > clearer than how it's done now. Done. https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2573: } On 2015/09/01 21:55:17, David Benjamin wrote: > On 2015/09/01 16:38:47, Avi wrote: > > Do we really want to do this for every WebContents everywhere? Do we at least > > want to make sure that it's a browser tab? > > +1. If we can manage it, I think WebContentsDelegate seems a better place to > latch this onto. As a bonus, that prerender block can just go away. Though I > dunno if in practice we'll want a global one too. I initially put that in WebContentsDelegate, but it did not work. The reason is that some apps need the AppURLRedirector to be installed, even if their WebContentsDelegate is an ExtensionHost. This particular class does not have access to /chrome (as per /extension DEPS), which is where the AppURLRedirector is located. I also become worried that we would have to duplicate the logic in many different WebContentsDelegate. Note that this is not a regression, as currently the ResourceThrottles are installed on all requests. https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/1269813002/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:345: InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request); On 2015/09/01 21:55:17, David Benjamin wrote: > [This is probably for a separate CL, but... wow, talk about questionable logic. > It just happens to hook into XHR user gestures and not, say, I tapped on a > button. > https://docs.google.com/document/d/1z2O07u5WoEtsLLUkx6RDjbVO5XrNwMJAQIBVvzRpE... > ] > > On the plus side, I think is_prerendering check and the TODO can go away since > they're not relevant for this call. Dunno if you want to add a TODO to revise > this one though. Done. https://codereview.chromium.org/1269813002/diff/80001/components/navigation_i... File components/navigation_interception/intercept_navigation_delegate.cc (right): https://codereview.chromium.org/1269813002/diff/80001/components/navigation_i... components/navigation_interception/intercept_navigation_delegate.cc:91: return scoped_ptr<InterceptNavigationThrottle>( On 2015/09/01 21:55:17, David Benjamin wrote: > Optional: This can be > > return make_scoped_ptr(new InterceptNavigationThrottle(...)); Due to a change in the interface for registering throttles, it now returns a raw pointer. https://codereview.chromium.org/1269813002/diff/80001/components/navigation_i... File components/navigation_interception/intercept_navigation_throttle.h (right): https://codereview.chromium.org/1269813002/diff/80001/components/navigation_i... components/navigation_interception/intercept_navigation_throttle.h:30: const NavigationParams& /* navigation_params */)> On 2015/08/28 16:40:24, carlosk wrote: > For a future change we should consider replacing this to directly accept the > NavigationHandle instead of doing the conversion to NavigationParams. Maybe add > a TODO? Nope because they get converted to an equivalent java struct. So the C++ struct will have to be initialized anyway. https://codereview.chromium.org/1269813002/diff/80001/components/navigation_i... components/navigation_interception/intercept_navigation_throttle.h:34: content::WebContents* web_contents, On 2015/08/31 23:25:15, nasko (slow to review) wrote: > I'd put web_contents as the first parameter, as it is the scope in which > navigation_handle exists. Done. https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:57: DCHECK(state_ != DID_START) On 2015/09/01 22:08:12, nasko (slow to review) wrote: > On 2015/09/01 16:38:47, Avi wrote: > > On 2015/08/31 23:25:15, nasko wrote: > > > nit: CHECK? Also in the methods below. > > > > DCHECK is the usual Chromium style; why are you proposing CHECK? > > This is not a codepath that the renderer can induce us to go down, so it is > purely bug in our implementation. As such, using a CHECK will allow us to get > crash reports with status of what we got wrong (stack trace and some registers). > DCHECKs should be used when a renderer process can cause us to hit it, otherwise > we will create a denial-of-service condition where malicious page can > continually crash the browser. I think CHECKS are ok in that case, but apart from a codepath the renderer can induce us to go down, there is another case where they should not be used. If a codepath is critical to performance, converting a DCHECK in a CHECK can have an impact on the user experience. https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:121: // Have each throttle be notified of the request. On 2015/09/01 21:55:17, David Benjamin wrote: > Nit: Perhaps "Notify each throttle of the request."? Slightly shorter or > something. Done. https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:122: for (auto it = throttles_.begin(); it != throttles_.end(); ++it) { On 2015/09/01 21:55:18, David Benjamin wrote: > (Is it possible to use a range-based for loop here, or does it being a > ScopedVector cause you troubles?) Done (it's been so long since I wrote a loop in Chromium code that I did not remember we can now use this C++11 feature). https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:140: sanitized_referrer_.url = new_referrer_url; On 2015/08/28 16:40:24, carlosk wrote: > Why setting this member URL here when the next line replaces the whole value? Because it needs to be updated before being sanitized (it will precisely look at that url). The alternative is: sanitized_referrer_ = Referrer::SanitizeForRequest(url_, Referrer(new_referrer_url, sanitized_referrer_.policy)), which I don't find tremendously clearer. https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:141: sanitized_referrer_ = Referrer::SanitizeForRequest(url_, sanitized_referrer_); On 2015/09/01 21:55:18, David Benjamin wrote: > I'm a little puzzled by this line. Wouldn't the referrer come pre-sanitized by > the loader stack already? You're just trying to report on what the loader stack > decided to use. Or is the problem that this hasn't happened yet and you need to > repeat that logic? (I'm not that familiar with referrers.) I based myself on the fact that the ResourceThrottle I was converting did sanitize Referrers, so I ported that functionality over. Since I'm not familiar with referrers, I did not want to miss a case where the Referrer would not be properly sanitized. https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:144: state_ = WILL_REDIRECT_REQUEST; On 2015/09/01 21:55:18, David Benjamin wrote: > Is anything actually sensitive to this state? You don't have asynchronous > throttles, so you don't need to keep track of exactly where you are like that. Done. Note that we may have to reintroduce it if throttles become asynchronous, which may happen). https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:58: // stack, to let it know whether the navigation should go on or be stopped. On 2015/08/28 16:40:24, carlosk wrote: > I don't understand how this comments fits here. Because it comes from an earlier version of the patch where the check result enum was defined here (but forgot to remove the comment apparently :). https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:58: : frame_tree_node->render_manager()->current_frame_host(); On 2015/08/31 23:25:15, nasko (slow to review) wrote: > On 2015/08/28 16:40:24, carlosk wrote: > > This next-RFH logic should live inside RFHM. We've been in the verge of adding > > it there for some time already and this usage looks like the perfect use case. > > This code seems a bit fragile. What happens if there is a speculative RFH and > the url passed in doesn't match the SiteInstance for that one? The current code > doesn't handle this case, does it? This would happen in the case of redirects. In that case we may not even have a renderer with the right site instance. I'm not sure what to do in that case: fall back to the current RFH? Note that the ResourceThrottles try to filter the url based on the RFH that initiated the navigation (with the redirect it may not be appropriate). https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:194: // TODO(clamy): pass the real value for |is_external_protocol|. On 2015/09/01 21:55:18, David Benjamin wrote: > Hrm. It seems to manage that, we'd need to either fire this until the network > request has hit the IO thread (and so it becomes UI -> IO -> UI -> IO before > starting the request), or we have a way to compute it on the UI thread, which > means the OnRequestRedirect logic changes. > > I haven't looked closely, but this is only used by the Android Intents code, > right? It seems some codepaths actually compute it from the URL in Java with > UrlUtilities.isAcceptedScheme. Perhaps we should just switch it all to that and > remove it from this API? > > (A little ugly to have the duplicated code like that, but maybe it's fine in > this context? That code is at least top-level. I don't know what context it's > used in.) So it seems it is only used on Android to log to the DevTools console that a navigation failed if it is not handled by an intent. This is some internal code that was upstreamed recently, so I'm thinking maybe we could get rid of it entirely. In any case, I don't want to do a UI -> IO -> UI -> IO hop unless really forced to do so. https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:247: common_params_.url, FilterURL(common_params_.url, frame_tree_node_), On 2015/08/31 23:25:15, nasko (slow to review) wrote: > FilterURL should be a called separately and possibly kill the renderer if it > fails. Even if we don't do the kill now, the code will be easier to refactor > later on. Done. I have added a TODO for the kill (note that the current code does not kill the renderer if it fails). https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:164: url, filtered_url, is_main_frame, delegate_)); On 2015/08/31 23:25:15, nasko (slow to review) wrote: > Why do we need both? We never keep the two around in today's navigation logic, > do we? We need to provide the filtered url to the throttles, and we also need to keep the unfiltered one to do proper checks with what is sent by the renderer. In order to filter the url, we need access to the process, which we currently can't do from the NavigationHandle (in particular with PlzNavigate, in the current architecture we could keep a pointer to the renderer). Hence why I'm keeping both. https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:38: if (render_frame_host) { On 2015/09/01 21:55:18, David Benjamin wrote: > On 2015/08/31 23:25:15, nasko wrote: > > This doesn't seem right. Before we have gotten the response, we don't have > RFH, > > do we? > > This is the pre-PlzNavigate codepath I believe, so we have an RFH and everything > else. Yes this is only used by the pre-PlzNavigate codepath (and I'm basically copy-pasting what's being done in various ResourceThrottles). https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:80: : request_(request), weak_ptr_factory_(this) {} On 2015/08/31 23:25:15, nasko (slow to review) wrote: > Each member initialization on new line, no? Or has clang-format changed its > mind? Yes apparently clang-format likes it that way nowadays. https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:84: void NavigationResourceThrottle::WillStartRequest(bool* defer) { On 2015/08/31 23:25:15, nasko (slow to review) wrote: > Let's add DCHECKs for which thread each function is meant to be called. It > serves to both catch issues and as self-documentation. Done. https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:88: On 2015/08/31 23:25:15, nasko (slow to review) wrote: > Is it possible to know on the IO thread whether there are any throttles > registered and skip the thread hop if not? I wonder if such an optimization is > necessary. For the redirects yes but not for the start. In any case, on Android, the InterceptNavigationResourceHandler was doing a thread hop on each non-prerender main frame navigation, so I don't think we worsen the situation dramatically here. On Windows, I think the performance hit is less of an issue. https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:102: render_frame_id, request_->method() == "POST", On 2015/08/31 23:25:15, nasko (slow to review) wrote: > Do we have render_frame_id at the time of sending the network request? We are > trying to divorce the navigation resource request from the renderer, right? As mentionned before, this is entirely pre-PlzNavigate, so we do have the render_frame_id, since this is initialized following a request by the renderer. PlzNavigate uses the NavigationURLLoader to do the IO/UI communication (effectively removing the need for renderer ids). https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1444: // PlzNavigate: the thottle is unnecessary as communication with the UI On 2015/09/01 21:55:18, David Benjamin wrote: > Nit: throttle Done. https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:1449: resource_type == RESOURCE_TYPE_SUB_FRAME)) { On 2015/09/01 21:55:18, David Benjamin wrote: > IsResourceTypeFrame? Done. https://codereview.chromium.org/1269813002/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:516: void AddNavigationThrottles(NavigationHandle* navigation_handle) override; On 2015/08/31 23:25:15, nasko (slow to review) wrote: > Might be worthwhile adding a "for_main_frame_only" type parameter. Most features > might not care about subframe navigations, so we shouldn't be installing their > throttles for subframes. I'm not sure the parameter is necessary, since the NavigationHandle is provided which has a method returning whether the navigation happens in a main frame. In fact, the example here is using it precisely to register throttles only on main frame navigations. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/content_browser_client.h:656: // Allows the embedder to register NavigationThrottles for a navigation. On 2015/08/31 23:25:15, nasko (slow to review) wrote: > nit: s/NavigationThrottles/one or more NavigationThrottle/ Done. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/content_browser_client.h:657: // NavigationThrottles are used to control the flow of a navigation on the UI On 2015/08/31 23:25:15, nasko (slow to review) wrote: > nit: Singular NavigationThrottle. Done. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/content_browser_client.h:661: WebContents* web_contents) {} On 2015/09/01 21:55:18, David Benjamin wrote: > It's a little odd that this API is called AddNavigationThrottles, but it's not > really specific to navigation throttles. It seems to just be a thing that > signals a particular point in the timeline (start of navigation or so). Due to changes to the NavigationHandle interface, it now returns a vector of NavigationThrottles. I think it makes more sense now. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:34: virtual const GURL& GetValidatedURL() const = 0; On 2015/08/31 23:25:15, nasko (slow to review) wrote: > Is there any reason to return non-validated URL? I think some observers/throttles may be interested in the original one. I'm thinking of devtools in particular. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:37: // This reamins constant over the navigation. On 2015/09/01 16:38:47, Avi wrote: > typo: remains Done. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:42: // The following parameters are only available when the a network request is On 2015/09/01 16:38:47, Avi wrote: > "when the network request" Done. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:43: // made for the navigation (or commit time if not network request is made). On 2015/09/01 16:38:47, Avi wrote: > "or at commit time if no network request is made" Done. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:47: // Whether the navigation is a post or not. This may change during the On 2015/08/31 23:25:15, nasko (slow to review) wrote: > nit: s/post/POST/ Done. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:52: virtual const Referrer& GetSanitizedReferrer() const = 0; On 2015/08/31 23:25:15, nasko (slow to review) wrote: > Since there is no other referrer that we are returning, why not just > GetReferrer()? The comment should still say it is "sanitized", so it is clear to > whoever is reading about the method. Done. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:56: virtual bool HasUserGesture() const = 0; On 2015/08/31 23:25:15, nasko (slow to review) wrote: > Usually, browser initiated navigations are in response to something coming from > the user. Should that be factored into account? It's possible to do that for PlzNavigate, but difficult for current navigations. I'm not terribly thrilled about having this interface return two different things whether PlzNavigate is enabled or not, so I included a TODO to change that when PlzNavigate launches. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:61: // Whether the target URL cannot be handled by Chrome's internal protocol On 2015/08/31 23:25:15, nasko (slow to review) wrote: > nit: s/Chrome/the browser/. Chrome is a branded version of Chromium, and we are > the content layer, so we shouldn't be using Chrome directly. Done. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:81: scoped_ptr<NavigationThrottle> navigation_throttle) = 0; On 2015/09/01 21:55:18, David Benjamin wrote: > I wonder if this would be better done as the ResourceThrottle-style API where > you have one change to staple all your NavigationThrottles. Thoughts? Right now > you can add them at any point, which is kinda weird. Well the goal was to have that happen in just one point in the content browser client, where we add all throttles in one go (using this precise method). I moved this method to the implementation and have the content browser client return a vector of throttles. This way there will be less possibility of mistakes. I'm keeping a test version of this method though, since it's handy for testing throttles. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... File content/public/browser/navigation_throttle.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_throttle.h:13: // A NavigationThrottle tracks is used to interact with a navigation on the UI On 2015/08/28 16:40:25, carlosk wrote: > /is used to interact/and allows interaction/ Done. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_throttle.h:21: CANCEL_AND_IGNORE, On 2015/08/31 23:25:15, nasko (slow to review) wrote: > Why not just "Cancel" instead of "Cancel and Ignore"? What additional meaning > does "And Ignore" bring? This matches the actions in ResourceController. One of the option is CancelWithErrorCode, which we may introduce as we transition other Throttles. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... File content/public/browser/web_contents_delegate.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/web_contents_delegate.h:16: #include "content/public/browser/navigation_handle.h" On 2015/08/28 16:40:25, carlosk wrote: > It doesn't seem this include is needed. Done.
I haven't done a full pass quite yet, but I wonder why the interface change from scoped_ptr to raw pointers. Sending some of the comments as well. https://codereview.chromium.org/1269813002/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1269813002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:144: state_ = WILL_REDIRECT_REQUEST; Why did we lose the state update? https://codereview.chromium.org/1269813002/diff/140001/chrome/browser/apps/ap... File chrome/browser/apps/app_url_redirector.cc (right): https://codereview.chromium.org/1269813002/diff/140001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.cc:68: content::NavigationThrottle* AppUrlRedirector::MaybeCreateThrottleFor( Why not keep the scoped_ptr? It is much easier to reason about ownership if we explicitly see Pass vs just returns out as the former propagates through multiple returns and the latter exists only on the initial method. https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:60: // NavigationHandles are properly created in unit tests. Why not make this method private then? If it is not meant to be called by anyone other than the factory, it can be private with the factory being a friend to it. https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:247: const GURL filtered_new_url = FilterURL(common_params_.url, frame_tree_node_); nit: Why not use "validated_url", which we do commonly in navigation code elsewhere? https://codereview.chromium.org/1269813002/diff/140001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/140001/content/public/browser... content/public/browser/navigation_handle.h:43: // made for the navigation (or at commit time if not network request is made). nit: "no network request"
Thanks! I've explained the reason for switching from scoped_ptrs to raw ptrs in teh specific comment in the files. https://codereview.chromium.org/1269813002/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1269813002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:144: state_ = WILL_REDIRECT_REQUEST; On 2015/09/04 00:01:52, nasko (slow to review) wrote: > Why did we lose the state update? davidben@ noticed we were not doing anything useful with this state and suggested we remove it. It's true that we don't use it right now, so I removed it with the idea of re-adding it later if needed. https://codereview.chromium.org/1269813002/diff/140001/chrome/browser/apps/ap... File chrome/browser/apps/app_url_redirector.cc (right): https://codereview.chromium.org/1269813002/diff/140001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.cc:68: content::NavigationThrottle* AppUrlRedirector::MaybeCreateThrottleFor( On 2015/09/04 00:01:52, nasko (slow to review) wrote: > Why not keep the scoped_ptr? It is much easier to reason about ownership if we > explicitly see Pass vs just returns out as the former propagates through > multiple returns and the latter exists only on the initial method. This is because I changed the ContentBrowserClient method to return an array of NavigationThrottles instead of explicitly adding them on the NavigationHandle. I thought it may be weird to return a scoped_vector instead of a more traditional vector<NavigationThrottle*>, and so returns the latter. This meant the constructors were changed to return raw pointers. Now if you think it's fine to return a scoped_vector, then it makes sense for these constructors to return scoped_ptrs. https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:60: // NavigationHandles are properly created in unit tests. On 2015/09/04 00:01:52, nasko (slow to review) wrote: > Why not make this method private then? If it is not meant to be called by anyone > other than the factory, it can be private with the factory being a friend to it. Ha I wasn't sure about making non-test classes friend of other classes. Will do so in the next patch set.
https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:57: DCHECK(state_ != DID_START) On 2015/09/03 15:30:51, clamy wrote: > On 2015/09/01 22:08:12, nasko (slow to review) wrote: > > On 2015/09/01 16:38:47, Avi wrote: > > > On 2015/08/31 23:25:15, nasko wrote: > > > > nit: CHECK? Also in the methods below. > > > > > > DCHECK is the usual Chromium style; why are you proposing CHECK? > > > > This is not a codepath that the renderer can induce us to go down, so it is > > purely bug in our implementation. As such, using a CHECK will allow us to get > > crash reports with status of what we got wrong (stack trace and some > registers). > > DCHECKs should be used when a renderer process can cause us to hit it, > otherwise > > we will create a denial-of-service condition where malicious page can > > continually crash the browser. > > I think CHECKS are ok in that case, but apart from a codepath the renderer can > induce us to go down, there is another case where they should not be used. If a > codepath is critical to performance, converting a DCHECK in a CHECK can have an > impact on the user experience. In general this is a great point. It must be a really hot codepath for a single comparison statement to have an user visible impact. https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:58: : frame_tree_node->render_manager()->current_frame_host(); On 2015/09/03 15:30:51, clamy wrote: > On 2015/08/31 23:25:15, nasko (slow to review) wrote: > > On 2015/08/28 16:40:24, carlosk wrote: > > > This next-RFH logic should live inside RFHM. We've been in the verge of > adding > > > it there for some time already and this usage looks like the perfect use > case. > > > > This code seems a bit fragile. What happens if there is a speculative RFH and > > the url passed in doesn't match the SiteInstance for that one? The current > code > > doesn't handle this case, does it? > > This would happen in the case of redirects. In that case we may not even have a > renderer with the right site instance. I'm not sure what to do in that case: > fall back to the current RFH? Note that the ResourceThrottles try to filter the > url based on the RFH that initiated the navigation (with the redirect it may not > be appropriate). I'm not sure we need to call FilterURL on redirects though. We don't do it today and it is meant to ensure the renderer doesn't request something invalid. If the server redirects us to some bogus place, so be it. Maybe add a TODO on what the problem is and let's leave it to be resolved later on. https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:164: url, filtered_url, is_main_frame, delegate_)); On 2015/09/03 15:30:51, clamy wrote: > On 2015/08/31 23:25:15, nasko (slow to review) wrote: > > Why do we need both? We never keep the two around in today's navigation logic, > > do we? > > We need to provide the filtered url to the throttles, and we also need to keep > the unfiltered one to do proper checks with what is sent by the renderer. In > order to filter the url, we need access to the process, which we currently can't > do from the NavigationHandle (in particular with PlzNavigate, in the current > architecture we could keep a pointer to the renderer). Hence why I'm keeping > both. Where do we do those checks? Why can't we frontload the checks? Also, if we got an invalid URL from a renderer, it is likely a good reason for terminating the process. https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:38: if (render_frame_host) { On 2015/09/03 15:30:52, clamy wrote: > On 2015/09/01 21:55:18, David Benjamin wrote: > > On 2015/08/31 23:25:15, nasko wrote: > > > This doesn't seem right. Before we have gotten the response, we don't have > > RFH, > > > do we? > > > > This is the pre-PlzNavigate codepath I believe, so we have an RFH and > everything > > else. > > Yes this is only used by the pre-PlzNavigate codepath (and I'm basically > copy-pasting what's being done in various ResourceThrottles). Let's add a comment to clarify this then. Also a TODO to remove it once we are in PlzNavigate-only mode. https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:80: : request_(request), weak_ptr_factory_(this) {} On 2015/09/03 15:30:52, clamy wrote: > On 2015/08/31 23:25:15, nasko (slow to review) wrote: > > Each member initialization on new line, no? Or has clang-format changed its > > mind? > > Yes apparently clang-format likes it that way nowadays. Yuck :( https://codereview.chromium.org/1269813002/diff/80001/content/browser/loader/... content/browser/loader/navigation_resource_throttle.cc:88: On 2015/09/03 15:30:52, clamy wrote: > On 2015/08/31 23:25:15, nasko (slow to review) wrote: > > Is it possible to know on the IO thread whether there are any throttles > > registered and skip the thread hop if not? I wonder if such an optimization is > > necessary. > > For the redirects yes but not for the start. In any case, on Android, the > InterceptNavigationResourceHandler was doing a thread hop on each non-prerender > main frame navigation, so I don't think we worsen the situation dramatically > here. On Windows, I think the performance hit is less of an issue. I think performance is always an issue. nick@chromium.org has strong position on thread hopping, so it will be good to ensure he is on board with this. https://codereview.chromium.org/1269813002/diff/80001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:516: void AddNavigationThrottles(NavigationHandle* navigation_handle) override; On 2015/09/03 15:30:52, clamy wrote: > On 2015/08/31 23:25:15, nasko (slow to review) wrote: > > Might be worthwhile adding a "for_main_frame_only" type parameter. Most > features > > might not care about subframe navigations, so we shouldn't be installing their > > throttles for subframes. > > I'm not sure the parameter is necessary, since the NavigationHandle is provided > which has a method returning whether the navigation happens in a main frame. In > fact, the example here is using it precisely to register throttles only on main > frame navigations. Acknowledged. https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/80001/content/public/browser/... content/public/browser/navigation_handle.h:34: virtual const GURL& GetValidatedURL() const = 0; On 2015/09/03 15:30:52, clamy wrote: > On 2015/08/31 23:25:15, nasko (slow to review) wrote: > > Is there any reason to return non-validated URL? > > I think some observers/throttles may be interested in the original one. I'm > thinking of devtools in particular. If we need to keep this, I'd flip it around such that we have GetURL that returns the validated one and an oddly named one that returns the raw value. https://codereview.chromium.org/1269813002/diff/140001/chrome/browser/apps/ap... File chrome/browser/apps/app_url_redirector.cc (right): https://codereview.chromium.org/1269813002/diff/140001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.cc:68: content::NavigationThrottle* AppUrlRedirector::MaybeCreateThrottleFor( On 2015/09/04 12:59:14, clamy wrote: > On 2015/09/04 00:01:52, nasko (slow to review) wrote: > > Why not keep the scoped_ptr? It is much easier to reason about ownership if we > > explicitly see Pass vs just returns out as the former propagates through > > multiple returns and the latter exists only on the initial method. > > This is because I changed the ContentBrowserClient method to return an array of > NavigationThrottles instead of explicitly adding them on the NavigationHandle. I > thought it may be weird to return a scoped_vector instead of a more traditional > vector<NavigationThrottle*>, and so returns the latter. This meant the > constructors were changed to return raw pointers. Now if you think it's fine to > return a scoped_vector, then it makes sense for these constructors to return > scoped_ptrs. Yes, using ScopedVector is probably going to be nicer. https://codereview.chromium.org/1269813002/diff/140001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1269813002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2554: ChromeContentBrowserClient::AddNavigationThrottles( This method doesn't really add anything, maybe GetNavigationThrottles, since it returns you a vector of these? https://codereview.chromium.org/1269813002/diff/140001/components/navigation_... File components/navigation_interception/intercept_navigation_delegate.h (right): https://codereview.chromium.org/1269813002/diff/140001/components/navigation_... components/navigation_interception/intercept_navigation_delegate.h:56: content::NavigationHandle* handle, I wonder if it will be useful to just expose a way to get the WebContents from NavigationHandle. No real reason to pass both, as they have 1:1 correspondence. https://codereview.chromium.org/1269813002/diff/140001/components/navigation_... File components/navigation_interception/intercept_navigation_throttle.cc (right): https://codereview.chromium.org/1269813002/diff/140001/components/navigation_... components/navigation_interception/intercept_navigation_throttle.cc:17: web_contents_(web_contents), If we expose WebContents from NavigationHandle, we don't even need to keep this member variable. https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_factory.cc (right): https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_factory.cc:13: Either no empty line above, or one after the variable. https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_factory.h (right): https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_factory.h:16: class NavigationHandleFactory { Some class comments will be useful : ). Method ones will be great! https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigator_delegate.h (right): https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigator_delegate.h:49: virtual std::vector<NavigationThrottle*> AddNavigationThrottles( As I commented elsewhere, GetNavigationThrottles, so they can be registered on the handle. https://codereview.chromium.org/1269813002/diff/140001/content/browser/loader... File content/browser/loader/navigation_resource_handler.cc (right): https://codereview.chromium.org/1269813002/diff/140001/content/browser/loader... content/browser/loader/navigation_resource_handler.cc:69: !request()->context()->job_factory()->IsHandledURL(redirect_info.new_url); Can some of those smaller bits, like propagating the is_external_protocol value be split out in separate CLs? That way we have more manageable/smaller CLs. https://codereview.chromium.org/1269813002/diff/140001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/1269813002/diff/140001/content/public/browser... content/public/browser/content_browser_client.h:657: // Allows the embedder to register one or more NavigationThrottle for a nit: one or more implies the next word being plural. https://codereview.chromium.org/1269813002/diff/140001/content/public/browser... content/public/browser/content_browser_client.h:660: // will be executed in the order they were registered. nit: s/registered/provided/, since the embedder on longer registers them directly. https://codereview.chromium.org/1269813002/diff/140001/content/public/browser... content/public/browser/content_browser_client.h:662: // by the NavigationHandle. This note won't be necessary if we have ScopedVector, which will clearly indicate ownership semantics. https://codereview.chromium.org/1269813002/diff/140001/content/public/test/na... File content/public/test/navigation_handle_tester.h (right): https://codereview.chromium.org/1269813002/diff/140001/content/public/test/na... content/public/test/navigation_handle_tester.h:21: // NavigationHandleTester, use the static methods provided. nit: s/methods/method/ as there is only one. https://codereview.chromium.org/1269813002/diff/140001/content/public/test/te... File content/public/test/test_renderer_host.h (right): https://codereview.chromium.org/1269813002/diff/140001/content/public/test/te... content/public/test/test_renderer_host.h:186: scoped_ptr<TestNavigationHandleFactory> handle_factory_; handle_factory is too generic. Let's prepend navigation_ to it.
Thanks! https://codereview.chromium.org/1269813002/diff/140001/chrome/browser/apps/ap... File chrome/browser/apps/app_url_redirector.cc (right): https://codereview.chromium.org/1269813002/diff/140001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.cc:68: content::NavigationThrottle* AppUrlRedirector::MaybeCreateThrottleFor( On 2015/09/04 23:36:49, nasko (out until Sept 11) wrote: > On 2015/09/04 12:59:14, clamy wrote: > > On 2015/09/04 00:01:52, nasko (slow to review) wrote: > > > Why not keep the scoped_ptr? It is much easier to reason about ownership if > we > > > explicitly see Pass vs just returns out as the former propagates through > > > multiple returns and the latter exists only on the initial method. > > > > This is because I changed the ContentBrowserClient method to return an array > of > > NavigationThrottles instead of explicitly adding them on the NavigationHandle. > I > > thought it may be weird to return a scoped_vector instead of a more > traditional > > vector<NavigationThrottle*>, and so returns the latter. This meant the > > constructors were changed to return raw pointers. Now if you think it's fine > to > > return a scoped_vector, then it makes sense for these constructors to return > > scoped_ptrs. > > Yes, using ScopedVector is probably going to be nicer. Done. https://codereview.chromium.org/1269813002/diff/140001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1269813002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2554: ChromeContentBrowserClient::AddNavigationThrottles( On 2015/09/04 23:36:49, nasko (out until Sept 11) wrote: > This method doesn't really add anything, maybe GetNavigationThrottles, since it > returns you a vector of these? Done. https://codereview.chromium.org/1269813002/diff/140001/components/navigation_... File components/navigation_interception/intercept_navigation_delegate.h (right): https://codereview.chromium.org/1269813002/diff/140001/components/navigation_... components/navigation_interception/intercept_navigation_delegate.h:56: content::NavigationHandle* handle, On 2015/09/04 23:36:49, nasko (out until Sept 11) wrote: > I wonder if it will be useful to just expose a way to get the WebContents from > NavigationHandle. No real reason to pass both, as they have 1:1 correspondence. Done. https://codereview.chromium.org/1269813002/diff/140001/components/navigation_... File components/navigation_interception/intercept_navigation_throttle.cc (right): https://codereview.chromium.org/1269813002/diff/140001/components/navigation_... components/navigation_interception/intercept_navigation_throttle.cc:17: web_contents_(web_contents), On 2015/09/04 23:36:49, nasko (out until Sept 11) wrote: > If we expose WebContents from NavigationHandle, we don't even need to keep this > member variable. Done. https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_factory.cc (right): https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_factory.cc:13: On 2015/09/04 23:36:49, nasko (out until Sept 11) wrote: > Either no empty line above, or one after the variable. Done. https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_factory.h (right): https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_factory.h:16: class NavigationHandleFactory { On 2015/09/04 23:36:49, nasko (out until Sept 11) wrote: > Some class comments will be useful : ). Method ones will be great! Done. https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:60: // NavigationHandles are properly created in unit tests. On 2015/09/04 12:59:14, clamy wrote: > On 2015/09/04 00:01:52, nasko (slow to review) wrote: > > Why not make this method private then? If it is not meant to be called by > anyone > > other than the factory, it can be private with the factory being a friend to > it. > > Ha I wasn't sure about making non-test classes friend of other classes. Will do > so in the next patch set. I removed it entirely, and have NavigationHandleFactory a friend of NavigationHandleImpl, to call the constructor directly. https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:247: const GURL filtered_new_url = FilterURL(common_params_.url, frame_tree_node_); On 2015/09/04 00:01:52, nasko (out until Sept 11) wrote: > nit: Why not use "validated_url", which we do commonly in navigation code > elsewhere? Done. https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigator_delegate.h (right): https://codereview.chromium.org/1269813002/diff/140001/content/browser/frame_... content/browser/frame_host/navigator_delegate.h:49: virtual std::vector<NavigationThrottle*> AddNavigationThrottles( On 2015/09/04 23:36:49, nasko (out until Sept 11) wrote: > As I commented elsewhere, GetNavigationThrottles, so they can be registered on > the handle. This method no longer necessary, as the WebContents can now be accessed from the NavigationHandle. Removed it. https://codereview.chromium.org/1269813002/diff/140001/content/browser/loader... File content/browser/loader/navigation_resource_handler.cc (right): https://codereview.chromium.org/1269813002/diff/140001/content/browser/loader... content/browser/loader/navigation_resource_handler.cc:69: !request()->context()->job_factory()->IsHandledURL(redirect_info.new_url); On 2015/09/04 23:36:49, nasko (out until Sept 11) wrote: > Can some of those smaller bits, like propagating the is_external_protocol value > be split out in separate CLs? That way we have more manageable/smaller CLs. I've taken out the is_external_protocol update part. It's only used for logging on Android, so I'll see if I can get rid of it entirely. https://codereview.chromium.org/1269813002/diff/140001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/1269813002/diff/140001/content/public/browser... content/public/browser/content_browser_client.h:657: // Allows the embedder to register one or more NavigationThrottle for a On 2015/09/04 23:36:49, nasko (out until Sept 11) wrote: > nit: one or more implies the next word being plural. Done. https://codereview.chromium.org/1269813002/diff/140001/content/public/browser... content/public/browser/content_browser_client.h:660: // will be executed in the order they were registered. On 2015/09/04 23:36:49, nasko (out until Sept 11) wrote: > nit: s/registered/provided/, since the embedder on longer registers them > directly. Done. https://codereview.chromium.org/1269813002/diff/140001/content/public/browser... content/public/browser/content_browser_client.h:662: // by the NavigationHandle. On 2015/09/04 23:36:49, nasko (out until Sept 11) wrote: > This note won't be necessary if we have ScopedVector, which will clearly > indicate ownership semantics. Done. https://codereview.chromium.org/1269813002/diff/140001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/140001/content/public/browser... content/public/browser/navigation_handle.h:43: // made for the navigation (or at commit time if not network request is made). On 2015/09/04 00:01:53, nasko (out until Sept 11) wrote: > nit: "no network request" Done. https://codereview.chromium.org/1269813002/diff/140001/content/public/test/na... File content/public/test/navigation_handle_tester.h (right): https://codereview.chromium.org/1269813002/diff/140001/content/public/test/na... content/public/test/navigation_handle_tester.h:21: // NavigationHandleTester, use the static methods provided. On 2015/09/04 23:36:49, nasko (out until Sept 11) wrote: > nit: s/methods/method/ as there is only one. Done. https://codereview.chromium.org/1269813002/diff/140001/content/public/test/te... File content/public/test/test_renderer_host.h (right): https://codereview.chromium.org/1269813002/diff/140001/content/public/test/te... content/public/test/test_renderer_host.h:186: scoped_ptr<TestNavigationHandleFactory> handle_factory_; On 2015/09/04 23:36:49, nasko (out until Sept 11) wrote: > handle_factory is too generic. Let's prepend navigation_ to it. Done.
clamy@chromium.org changed reviewers: + creis@chromium.org
+creis: PTAL
I haven't made it through a full review yet, but here are some initial questions/comments, mostly on content/public. https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... File chrome/browser/apps/app_url_redirector.cc (left): https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.cc:40: DCHECK_CURRENTLY_ON(BrowserThread::UI); Why remove this? https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.cc:58: DCHECK(!params.is_post()); Why remove this? https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... File chrome/browser/apps/app_url_redirector.cc (right): https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.cc:74: if (!browser_context) Is this possible? https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... File chrome/browser/apps/app_url_redirector.h (right): https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.h:19: // a matching URL handler in the 'url_handlers' manifest key. Is this class moving from the IO thread to the UI thread? Might want to mention that it's a UI thread class now in the comment, to avoid confusion for people who have seen it before. https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/content_browser_client.h:102: class NavigationHandle; nit: Alphabetize. https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/content_browser_client.h:673: // navigation. A NavigationThrottle is used to control the flow of a nit: for the navigation indicated by |navigation_handle|. https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/content_browser_client.h:676: virtual ScopedVector<NavigationThrottle> GetNavigationThrottles( "GetNavigationThrottles" sounds like an accessor for general purpose use. Maybe CreateThrottlesForNavigation? https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/navigation_handle.h:31: virtual const GURL& GetURL() const = 0; From http://www.chromium.org/developers/content-module/content-api: "don't add the const identifier to interfaces. For interfaces implemented by the embedder, we can't make assumptions about what the embedder needs to implement it. For interfaces implemented by content, the implementation details doesn't have to be exposed." John has wanted to be fairly strict on this, so we should update these previous ones as well as not introduce new consts. https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/navigation_handle.h:35: virtual const GURL& GetValidatedURL() const = 0; It's not initially clear to me what this refers to. What kind of sanitization or validation? Why is it needed? This matters because other code is going to have to choose between calling this and calling GetURL() (which most people will do without thinking about). I'm hesitant to expose both without a strong reason. https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/navigation_handle.h:62: virtual bool HasUserGesture() const = 0; I'm not sure why many of these are inaccessible until network request start time. Aren't things like GetReferrer, HasUserGesture, etc known at navigation start time?Actually, all of them seem like they would be. What is the distinction for? https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/navigation_handle.h:88: // ContentBrowserClient::AddNavigationThrottles. This ensures proper ordering Stale name? https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/navigation_handle.h:90: virtual void RegisterThrottleForTesting( Everything so far has been an accessor with no state changes. Maybe this belongs in a separate comment-delimited section? https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... File content/public/browser/navigation_throttle.h (right): https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/navigation_throttle.h:17: // This is returned to the navigation handler to allow the navigation to What's a navigation handler? (Let's try to be more specific, especially with terms like NavigationHandle in use.)
Thanks! I've answered some of the questions, will wait for the rest of the review before submitting a new patchset. https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... File chrome/browser/apps/app_url_redirector.cc (left): https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.cc:40: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2015/09/11 00:06:48, Charlie Reis (slow till 9-14) wrote: > Why remove this? I thought it was less needed since the class is now entirely on the UI thread. I can bring it back. https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.cc:58: DCHECK(!params.is_post()); On 2015/09/11 00:06:48, Charlie Reis (slow till 9-14) wrote: > Why remove this? It was removed in one version where I would check for whether the navigation is a post just above. Since that changed, I will put it back again. https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... File chrome/browser/apps/app_url_redirector.cc (right): https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.cc:74: if (!browser_context) On 2015/09/11 00:06:48, Charlie Reis (slow till 9-14) wrote: > Is this possible? Not sure. I will replace with a DCHECK and see what happens. https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... File chrome/browser/apps/app_url_redirector.h (right): https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.h:19: // a matching URL handler in the 'url_handlers' manifest key. On 2015/09/11 00:06:48, Charlie Reis (slow till 9-14) wrote: > Is this class moving from the IO thread to the UI thread? Might want to mention > that it's a UI thread class now in the comment, to avoid confusion for people > who have seen it before. Yes it's moving to be entirely on the UI thread. I will update the comment. https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/navigation_handle.h:35: virtual const GURL& GetValidatedURL() const = 0; On 2015/09/11 00:06:48, Charlie Reis (slow till 9-14) wrote: > It's not initially clear to me what this refers to. What kind of sanitization > or validation? Why is it needed? > > This matters because other code is going to have to choose between calling this > and calling GetURL() (which most people will do without thinking about). I'm > hesitant to expose both without a strong reason. When transitioning the InterceptNavigationResourceThrottle, they filter the url using RenderProcessHost::FilterURL. This returns the url filtered by the same method. We had a discussion with nasko@ about whether GetURL should just always returns the filtered url, but I was worried that some future users of the interface, such as SafeBrowsing might need the original unfiltered url. What we could do is have GetURL return the filtered url by default, and add a GetUnfilteredURL method that returns the original url if needed. Wdyt? https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/navigation_handle.h:62: virtual bool HasUserGesture() const = 0; On 2015/09/11 00:06:49, Charlie Reis (slow till 9-14) wrote: > I'm not sure why many of these are inaccessible until network request start > time. Aren't things like GetReferrer, HasUserGesture, etc known at navigation > start time?Actually, all of them seem like they would be. What is the > distinction for? It's for supporting the current architecture without too many additional changes. DidStartNavigation is executed following a DidStartProvisionalLoad IPC. In order to know all those parameters at that time, I'd need to add more parameters to the DidStartProvisionalLoad IPC. https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... File content/public/browser/navigation_throttle.h (right): https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/navigation_throttle.h:17: // This is returned to the navigation handler to allow the navigation to On 2015/09/11 00:06:49, Charlie Reis (slow till 9-14) wrote: > What's a navigation handler? (Let's try to be more specific, especially with > terms like NavigationHandle in use.) I meant a NavigationHandle, will change that in the next patch set.
https://codereview.chromium.org/1269813002/diff/180001/components/navigation_... File components/navigation_interception/intercept_navigation_delegate.cc (right): https://codereview.chromium.org/1269813002/diff/180001/components/navigation_... components/navigation_interception/intercept_navigation_delegate.cc:89: content::WebContents* web_contents) { Isn't the web_contents parameter dropped on this method now? https://codereview.chromium.org/1269813002/diff/180001/components/navigation_... File components/navigation_interception/intercept_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/1269813002/diff/180001/components/navigation_... components/navigation_interception/intercept_navigation_throttle_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. This needs to be updated to new style and 2015. https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_factory.h (right): https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_factory.h:21: // in tests to create TestNavigationHandles instead of plain nit: Which type of tests? unit, browser, or both? It will be good to be explicit here. https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:114: // Register the navigation throttles. The scoped_vector returned by nit: s/scoped_vector/ScopedVector/ https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:88: void set_web_contents(WebContents* web_contents) { Uh, this looks scary/weird. Why do we need it? https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:155: // A pointer to the WebContents the navigation is taking place in. nit: Add to the comment to explain why it is ok to use a raw pointer here and what the lifetime guarantees are. https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:912: GetProcess()->FilterURL(false, &filtered_url); This is already done on line 881, why do we need one more of these calls? https://codereview.chromium.org/1269813002/diff/180001/content/browser/loader... File content/browser/loader/navigation_resource_handler.cc (right): https://codereview.chromium.org/1269813002/diff/180001/content/browser/loader... content/browser/loader/navigation_resource_handler.cc:21: #include "net/url_request/url_request_job_factory.h" Why do we need these new includes if we haven't changed any code in this file? https://codereview.chromium.org/1269813002/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1269813002/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2781: static_cast<NavigationHandleImpl*>(navigation_handle)->set_web_contents(this); Is this the only usage of set_web_contents? Can we avoid having this settable through a public method if noone else needs it? https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/navigation_handle.h:35: virtual const GURL& GetValidatedURL() const = 0; On 2015/09/11 13:30:44, clamy wrote: > On 2015/09/11 00:06:48, Charlie Reis (slow till 9-14) wrote: > > It's not initially clear to me what this refers to. What kind of sanitization > > or validation? Why is it needed? > > > > This matters because other code is going to have to choose between calling > this > > and calling GetURL() (which most people will do without thinking about). I'm > > hesitant to expose both without a strong reason. > > When transitioning the InterceptNavigationResourceThrottle, they filter the url > using RenderProcessHost::FilterURL. This returns the url filtered by the same > method. We had a discussion with nasko@ about whether GetURL should just always > returns the filtered url, but I was worried that some future users of the > interface, such as SafeBrowsing might need the original unfiltered url. > > What we could do is have GetURL return the filtered url by default, and add a > GetUnfilteredURL method that returns the original url if needed. Wdyt? I am all for only having one accessor, as I've stated before. Let's not anticipate what we might need and just expose what we really need. I'm fine exposing only the filtered URL, which is likely what we do in other places.
Thanks! A few answers on the WebContents and NavigationHandleImpl. https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:88: void set_web_contents(WebContents* web_contents) { On 2015/09/14 22:00:04, nasko (slow to review) wrote: > Uh, this looks scary/weird. Why do we need it? Because I cannot static_cast the NavigatorDelegate to a WebContentsImpl, and still need a way for the handle to know about the WebContents (can't include the web_contents_impl header file inside /frame_host). https://codereview.chromium.org/1269813002/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1269813002/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2781: static_cast<NavigationHandleImpl*>(navigation_handle)->set_web_contents(this); On 2015/09/14 22:00:04, nasko (slow to review) wrote: > Is this the only usage of set_web_contents? Can we avoid having this settable > through a public method if noone else needs it? Well if I have a handy way to get to the WebContents from somewhere inside /frame_host yes, otherwise that requires making WebContentsImpl a friend of NavigationHandleImpl.
On 2015/09/14 22:12:10, clamy wrote: > Thanks! A few answers on the WebContents and NavigationHandleImpl. > > https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... > File content/browser/frame_host/navigation_handle_impl.h (right): > > https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... > content/browser/frame_host/navigation_handle_impl.h:88: void > set_web_contents(WebContents* web_contents) { > On 2015/09/14 22:00:04, nasko (slow to review) wrote: > > Uh, this looks scary/weird. Why do we need it? > > Because I cannot static_cast the NavigatorDelegate to a WebContentsImpl, and > still need a way for the handle to know about the WebContents (can't include the > web_contents_impl header file inside /frame_host). > > https://codereview.chromium.org/1269813002/diff/180001/content/browser/web_co... > File content/browser/web_contents/web_contents_impl.cc (right): > > https://codereview.chromium.org/1269813002/diff/180001/content/browser/web_co... > content/browser/web_contents/web_contents_impl.cc:2781: > static_cast<NavigationHandleImpl*>(navigation_handle)->set_web_contents(this); > On 2015/09/14 22:00:04, nasko (slow to review) wrote: > > Is this the only usage of set_web_contents? Can we avoid having this settable > > through a public method if noone else needs it? > > Well if I have a handy way to get to the WebContents from somewhere inside > /frame_host yes, otherwise that requires making WebContentsImpl a friend of > NavigationHandleImpl. Oh! It is great that DEPS is keeping us honest here. It is good that NavigationHandle can't cast/convert to WebContents, as it is not supposed to know about it.
Thanks. Here's some comments and questions on the rest, in addition to my previous ones. Feel free to find some time to chat in person about them if that helps. https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:155: // A pointer to the WebContents the navigation is taking place in. On 2015/09/14 22:00:04, nasko (slow to review) wrote: > nit: Add to the comment to explain why it is ok to use a raw pointer here and > what the lifetime guarantees are. It's probably also worth explaining (here or elsewhere) why this code in frame_host depends on WebContents (due to the relationship between WebContentsObserver and NavigationHandle). https://codereview.chromium.org/1269813002/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1269813002/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2781: static_cast<NavigationHandleImpl*>(navigation_handle)->set_web_contents(this); On 2015/09/14 22:12:10, clamy wrote: > On 2015/09/14 22:00:04, nasko (slow to review) wrote: > > Is this the only usage of set_web_contents? Can we avoid having this settable > > through a public method if noone else needs it? > > Well if I have a handy way to get to the WebContents from somewhere inside > /frame_host yes, otherwise that requires making WebContentsImpl a friend of > NavigationHandleImpl. Yeah, this is one of the tricky aspects of having frame_host code know about WebContents. I see that it's needed, though, since NavigationHandle really is tied to WebContentsObserver. Not sure if I see a better way to handle it than this. https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/navigation_handle.h:35: virtual const GURL& GetValidatedURL() const = 0; On 2015/09/14 22:00:04, nasko (slow to review) wrote: > On 2015/09/11 13:30:44, clamy wrote: > > On 2015/09/11 00:06:48, Charlie Reis (slow till 9-14) wrote: > > > It's not initially clear to me what this refers to. What kind of > sanitization > > > or validation? Why is it needed? > > > > > > This matters because other code is going to have to choose between calling > > this > > > and calling GetURL() (which most people will do without thinking about). > I'm > > > hesitant to expose both without a strong reason. > > > > When transitioning the InterceptNavigationResourceThrottle, they filter the > url > > using RenderProcessHost::FilterURL. This returns the url filtered by the same > > method. We had a discussion with nasko@ about whether GetURL should just > always > > returns the filtered url, but I was worried that some future users of the > > interface, such as SafeBrowsing might need the original unfiltered url. > > > > What we could do is have GetURL return the filtered url by default, and add a > > GetUnfilteredURL method that returns the original url if needed. Wdyt? > > I am all for only having one accessor, as I've stated before. Let's not > anticipate what we might need and just expose what we really need. I'm fine > exposing only the filtered URL, which is likely what we do in other places. Yes, let's try to have only GetURL which returns a validated URL. FilterURL is intended to be applied to all incoming URLs immediately, before they're seen by any other code. Hopefully we can avoid any cases where the unfiltered URL is needed. (If those come up, please let us know.) https://codereview.chromium.org/1269813002/diff/200001/chrome/browser/apps/ap... File chrome/browser/apps/app_url_redirector.cc (right): https://codereview.chromium.org/1269813002/diff/200001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.cc:78: if (handle->IsPost()) { Sanity check: Can we guarantee a navigation is either GET or POST and never another method? There are other HTTP methods, but I wouldn't be surprised if none of them can commit as a navigation. (We should be clear whether !IsPost implies GET in navigation_handle.h.) https://codereview.chromium.org/1269813002/diff/200001/components/navigation_... File components/navigation_interception/intercept_navigation_throttle.h (right): https://codereview.chromium.org/1269813002/diff/200001/components/navigation_... components/navigation_interception/intercept_navigation_throttle.h:26: // level navigations. Let's mention this is a UI thread class. Might want corresponding DCHECKs in the .cc file, since these used to be IO thread objects. https://codereview.chromium.org/1269813002/diff/200001/content/browser/frame_... File content/browser/frame_host/navigation_handle_factory.h (right): https://codereview.chromium.org/1269813002/diff/200001/content/browser/frame_... content/browser/frame_host/navigation_handle_factory.h:23: class NavigationHandleFactory { See my question in TestNavigationHandle about whether this is necessary. https://codereview.chromium.org/1269813002/diff/200001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1269813002/diff/200001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:54: // Filters the URL for the navigation based. I'm not sure what "for the navigation based" means. https://codereview.chromium.org/1269813002/diff/200001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:55: GURL FilterURL(const GURL& url, FrameTreeNode* frame_tree_node) { This feels like the wrong place to be enforcing this, if we don't know what process it came from. RPH::FilterURL is intended to be called immediately after any URL is received from the renderer process, before it gets passed deeper into code that might look at it. Is there an earlier place this can be handled? https://codereview.chromium.org/1269813002/diff/200001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:57: frame_tree_node->render_manager()->speculative_frame_host() Is this only used in PlzNavigate? https://codereview.chromium.org/1269813002/diff/200001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1269813002/diff/200001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:163: render_frame_host->GetProcess()->FilterURL(false, &filtered_url); Again, I'm not sure this is the right place to be calling FilterURL. Maybe we can chat about this while you're in town. https://codereview.chromium.org/1269813002/diff/200001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1269813002/diff/200001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:912: GetProcess()->FilterURL(false, &filtered_url); This was filtered above on line 881. https://codereview.chromium.org/1269813002/diff/200001/content/browser/loader... File content/browser/loader/navigation_resource_handler.cc (right): https://codereview.chromium.org/1269813002/diff/200001/content/browser/loader... content/browser/loader/navigation_resource_handler.cc:21: #include "net/url_request/url_request_job_factory.h" Are these stale? https://codereview.chromium.org/1269813002/diff/200001/content/test/test_navi... File content/test/test_navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/200001/content/test/test_navi... content/test/test_navigation_handle.h:16: // navigation call sby embedders of content/. nit: calls by https://codereview.chromium.org/1269813002/diff/200001/content/test/test_navi... content/test/test_navigation_handle.h:17: class TestNavigationHandle : public NavigationHandleImpl, I'm probably missing something here, but why do we need this class? It looks like it's just wrapping methods of NavigationHandleImpl which are already public. I'm lamenting all the overhead with this test-specific class and the NavigationHandleFactory. It would be nice to avoid TestNavigationHandle and NavigationHandleFactory, if NavigationHandleImpl itself is primarily a data structure.
Thanks! https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... File chrome/browser/apps/app_url_redirector.cc (left): https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.cc:40: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2015/09/11 13:30:43, clamy wrote: > On 2015/09/11 00:06:48, Charlie Reis (slow till 9-14) wrote: > > Why remove this? > > I thought it was less needed since the class is now entirely on the UI thread. I > can bring it back. Now back. https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.cc:58: DCHECK(!params.is_post()); On 2015/09/11 13:30:44, clamy wrote: > On 2015/09/11 00:06:48, Charlie Reis (slow till 9-14) wrote: > > Why remove this? > > It was removed in one version where I would check for whether the navigation is > a post just above. Since that changed, I will put it back again. Now back. https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... File chrome/browser/apps/app_url_redirector.h (right): https://codereview.chromium.org/1269813002/diff/180001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.h:19: // a matching URL handler in the 'url_handlers' manifest key. On 2015/09/11 13:30:44, clamy wrote: > On 2015/09/11 00:06:48, Charlie Reis (slow till 9-14) wrote: > > Is this class moving from the IO thread to the UI thread? Might want to > mention > > that it's a UI thread class now in the comment, to avoid confusion for people > > who have seen it before. > > Yes it's moving to be entirely on the UI thread. I will update the comment. Updated the comment. https://codereview.chromium.org/1269813002/diff/180001/components/navigation_... File components/navigation_interception/intercept_navigation_delegate.cc (right): https://codereview.chromium.org/1269813002/diff/180001/components/navigation_... components/navigation_interception/intercept_navigation_delegate.cc:89: content::WebContents* web_contents) { On 2015/09/14 22:00:04, nasko (slow to review) wrote: > Isn't the web_contents parameter dropped on this method now? Done. https://codereview.chromium.org/1269813002/diff/180001/components/navigation_... File components/navigation_interception/intercept_navigation_throttle_unittest.cc (right): https://codereview.chromium.org/1269813002/diff/180001/components/navigation_... components/navigation_interception/intercept_navigation_throttle_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2015/09/14 22:00:04, nasko (slow to review) wrote: > This needs to be updated to new style and 2015. Done. https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_factory.h (right): https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_factory.h:21: // in tests to create TestNavigationHandles instead of plain On 2015/09/14 22:00:04, nasko (slow to review) wrote: > nit: Which type of tests? unit, browser, or both? It will be good to be explicit > here. Done. https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:114: // Register the navigation throttles. The scoped_vector returned by On 2015/09/14 22:00:04, nasko (slow to review) wrote: > nit: s/scoped_vector/ScopedVector/ Done. https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:155: // A pointer to the WebContents the navigation is taking place in. On 2015/09/16 00:09:24, Charlie Reis (slow till 9-14) wrote: > On 2015/09/14 22:00:04, nasko (slow to review) wrote: > > nit: Add to the comment to explain why it is ok to use a raw pointer here and > > what the lifetime guarantees are. > > It's probably also worth explaining (here or elsewhere) why this code in > frame_host depends on WebContents (due to the relationship between > WebContentsObserver and NavigationHandle). Done. https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1269813002/diff/180001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:912: GetProcess()->FilterURL(false, &filtered_url); On 2015/09/14 22:00:04, nasko (slow to review) wrote: > This is already done on line 881, why do we need one more of these calls? Done. https://codereview.chromium.org/1269813002/diff/180001/content/browser/loader... File content/browser/loader/navigation_resource_handler.cc (right): https://codereview.chromium.org/1269813002/diff/180001/content/browser/loader... content/browser/loader/navigation_resource_handler.cc:21: #include "net/url_request/url_request_job_factory.h" On 2015/09/14 22:00:04, nasko (slow to review) wrote: > Why do we need these new includes if we haven't changed any code in this file? Done. https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/content_browser_client.h:102: class NavigationHandle; On 2015/09/11 00:06:48, Charlie Reis (slow till 9-14) wrote: > nit: Alphabetize. Done. https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/content_browser_client.h:673: // navigation. A NavigationThrottle is used to control the flow of a On 2015/09/11 00:06:48, Charlie Reis (slow till 9-14) wrote: > nit: for the navigation indicated by |navigation_handle|. Done. https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/content_browser_client.h:676: virtual ScopedVector<NavigationThrottle> GetNavigationThrottles( On 2015/09/11 00:06:48, Charlie Reis (slow till 9-14) wrote: > "GetNavigationThrottles" sounds like an accessor for general purpose use. Maybe > CreateThrottlesForNavigation? Done. https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/navigation_handle.h:31: virtual const GURL& GetURL() const = 0; On 2015/09/11 00:06:49, Charlie Reis (slow till 9-14) wrote: > From http://www.chromium.org/developers/content-module/content-api: > "don't add the const identifier to interfaces. For interfaces implemented by the > embedder, we can't make assumptions about what the embedder needs to implement > it. For interfaces implemented by content, the implementation details doesn't > have to be exposed." > > John has wanted to be fairly strict on this, so we should update these previous > ones as well as not introduce new consts. Done. https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/navigation_handle.h:35: virtual const GURL& GetValidatedURL() const = 0; On 2015/09/16 00:09:24, Charlie Reis (slow till 9-14) wrote: > On 2015/09/14 22:00:04, nasko (slow to review) wrote: > > On 2015/09/11 13:30:44, clamy wrote: > > > On 2015/09/11 00:06:48, Charlie Reis (slow till 9-14) wrote: > > > > It's not initially clear to me what this refers to. What kind of > > sanitization > > > > or validation? Why is it needed? > > > > > > > > This matters because other code is going to have to choose between calling > > > this > > > > and calling GetURL() (which most people will do without thinking about). > > I'm > > > > hesitant to expose both without a strong reason. > > > > > > When transitioning the InterceptNavigationResourceThrottle, they filter the > > url > > > using RenderProcessHost::FilterURL. This returns the url filtered by the > same > > > method. We had a discussion with nasko@ about whether GetURL should just > > always > > > returns the filtered url, but I was worried that some future users of the > > > interface, such as SafeBrowsing might need the original unfiltered url. > > > > > > What we could do is have GetURL return the filtered url by default, and add > a > > > GetUnfilteredURL method that returns the original url if needed. Wdyt? > > > > I am all for only having one accessor, as I've stated before. Let's not > > anticipate what we might need and just expose what we really need. I'm fine > > exposing only the filtered URL, which is likely what we do in other places. > > Yes, let's try to have only GetURL which returns a validated URL. FilterURL is > intended to be applied to all incoming URLs immediately, before they're seen by > any other code. Hopefully we can avoid any cases where the unfiltered URL is > needed. (If those come up, please let us know.) Done. https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/navigation_handle.h:88: // ContentBrowserClient::AddNavigationThrottles. This ensures proper ordering On 2015/09/11 00:06:48, Charlie Reis (slow till 9-14) wrote: > Stale name? Done. https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/navigation_handle.h:90: virtual void RegisterThrottleForTesting( On 2015/09/11 00:06:48, Charlie Reis (slow till 9-14) wrote: > Everything so far has been an accessor with no state changes. Maybe this > belongs in a separate comment-delimited section? Well I did group it with the last 3 accessors in a Navigation control flow section, should it still be separated? https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... File content/public/browser/navigation_throttle.h (right): https://codereview.chromium.org/1269813002/diff/180001/content/public/browser... content/public/browser/navigation_throttle.h:17: // This is returned to the navigation handler to allow the navigation to On 2015/09/11 13:30:44, clamy wrote: > On 2015/09/11 00:06:49, Charlie Reis (slow till 9-14) wrote: > > What's a navigation handler? (Let's try to be more specific, especially with > > terms like NavigationHandle in use.) > > I meant a NavigationHandle, will change that in the next patch set. Done. https://codereview.chromium.org/1269813002/diff/200001/chrome/browser/apps/ap... File chrome/browser/apps/app_url_redirector.cc (right): https://codereview.chromium.org/1269813002/diff/200001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.cc:78: if (handle->IsPost()) { On 2015/09/16 00:09:24, Charlie Reis (slow till 9-14) wrote: > Sanity check: Can we guarantee a navigation is either GET or POST and never > another method? There are other HTTP methods, but I wouldn't be surprised if > none of them can commit as a navigation. > > (We should be clear whether !IsPost implies GET in navigation_handle.h.) For PlzNavigate it is either a POST or a GET. For regular navigations, I have added DCHECKs in the code to guarantee that. If it appears that this cannot be guaranteed, I will replace the IsPost method on the navigation handle by a GetMethod that will return an explicit string. (Also updated the comment in navigation_handle.h). https://codereview.chromium.org/1269813002/diff/200001/components/navigation_... File components/navigation_interception/intercept_navigation_throttle.h (right): https://codereview.chromium.org/1269813002/diff/200001/components/navigation_... components/navigation_interception/intercept_navigation_throttle.h:26: // level navigations. On 2015/09/16 00:09:24, Charlie Reis (slow till 9-14) wrote: > Let's mention this is a UI thread class. Might want corresponding DCHECKs in > the .cc file, since these used to be IO thread objects. Done. https://codereview.chromium.org/1269813002/diff/200001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1269813002/diff/200001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:54: // Filters the URL for the navigation based. On 2015/09/16 00:09:24, Charlie Reis (slow till 9-14) wrote: > I'm not sure what "for the navigation based" means. Method was removed. https://codereview.chromium.org/1269813002/diff/200001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:55: GURL FilterURL(const GURL& url, FrameTreeNode* frame_tree_node) { On 2015/09/16 00:09:25, Charlie Reis (slow till 9-14) wrote: > This feels like the wrong place to be enforcing this, if we don't know what > process it came from. RPH::FilterURL is intended to be called immediately after > any URL is received from the renderer process, before it gets passed deeper into > code that might look at it. > > Is there an earlier place this can be handled? I see. In that case, since this method is PlzNavigate specific it can be replaced by filtering the url when we get it from the renderer in renderer-initiated navigations (RenderFrameHost::OnBeginNavigation). All other urls are gotten from the browser. https://codereview.chromium.org/1269813002/diff/200001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:57: frame_tree_node->render_manager()->speculative_frame_host() On 2015/09/16 00:09:25, Charlie Reis (slow till 9-14) wrote: > Is this only used in PlzNavigate? Yes - but I removed this method, see above. https://codereview.chromium.org/1269813002/diff/200001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1269813002/diff/200001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:163: render_frame_host->GetProcess()->FilterURL(false, &filtered_url); On 2015/09/16 00:09:25, Charlie Reis (slow till 9-14) wrote: > Again, I'm not sure this is the right place to be calling FilterURL. Maybe we > can chat about this while you're in town. This was removed since we already call FilterURL a few lines above. https://codereview.chromium.org/1269813002/diff/200001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1269813002/diff/200001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:912: GetProcess()->FilterURL(false, &filtered_url); On 2015/09/16 00:09:25, Charlie Reis (slow till 9-14) wrote: > This was filtered above on line 881. Done. https://codereview.chromium.org/1269813002/diff/200001/content/browser/loader... File content/browser/loader/navigation_resource_handler.cc (right): https://codereview.chromium.org/1269813002/diff/200001/content/browser/loader... content/browser/loader/navigation_resource_handler.cc:21: #include "net/url_request/url_request_job_factory.h" On 2015/09/16 00:09:25, Charlie Reis (slow till 9-14) wrote: > Are these stale? Yes. https://codereview.chromium.org/1269813002/diff/200001/content/test/test_navi... File content/test/test_navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/200001/content/test/test_navi... content/test/test_navigation_handle.h:16: // navigation call sby embedders of content/. On 2015/09/16 00:09:25, Charlie Reis (slow till 9-14) wrote: > nit: calls by Done. https://codereview.chromium.org/1269813002/diff/200001/content/test/test_navi... content/test/test_navigation_handle.h:17: class TestNavigationHandle : public NavigationHandleImpl, On 2015/09/16 00:09:25, Charlie Reis (slow till 9-14) wrote: > I'm probably missing something here, but why do we need this class? It looks > like it's just wrapping methods of NavigationHandleImpl which are already > public. > > I'm lamenting all the overhead with this test-specific class and the > NavigationHandleFactory. It would be nice to avoid TestNavigationHandle and > NavigationHandleFactory, if NavigationHandleImpl itself is primarily a data > structure. Well the goal here is to have embedders of content be able to write unit tests for NavigationThrottles, which is considerably simpler if you have a NavigationHandleTester class that allows you to have these methods explicitly called on the NavigationHandleImpl. Since NavigationHandleTester is a test interface, it can't be implemented by NavigationHandleImpl. And I added the factories to make sure that if people try to static_cast a NavigationHandleImpl in its test version it does not create an error. I guess an alternative would be to have static methods on WebContentsTester/RenderFrameHostTester to create a NavigationHandle and have the methods called on it.
creis@chromium.org changed reviewers: + nick@chromium.org
Thanks for all the updates! A little more discussion on the test infrastructure below. (I can be overruled, but it seems like there's some downsides to this approach.) https://codereview.chromium.org/1269813002/diff/200001/content/test/test_navi... File content/test/test_navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/200001/content/test/test_navi... content/test/test_navigation_handle.h:17: class TestNavigationHandle : public NavigationHandleImpl, On 2015/09/16 01:03:22, clamy wrote: > On 2015/09/16 00:09:25, Charlie Reis (slow till 9-14) wrote: > > I'm probably missing something here, but why do we need this class? It looks > > like it's just wrapping methods of NavigationHandleImpl which are already > > public. > > > > I'm lamenting all the overhead with this test-specific class and the > > NavigationHandleFactory. It would be nice to avoid TestNavigationHandle and > > NavigationHandleFactory, if NavigationHandleImpl itself is primarily a data > > structure. > > Well the goal here is to have embedders of content be able to write unit tests > for NavigationThrottles, which is considerably simpler if you have a > NavigationHandleTester class that allows you to have these methods explicitly > called on the NavigationHandleImpl. Since NavigationHandleTester is a test > interface, it can't be implemented by NavigationHandleImpl. And I added the > factories to make sure that if people try to static_cast a NavigationHandleImpl > in its test version it does not create an error. > > I guess an alternative would be to have static methods on > WebContentsTester/RenderFrameHostTester to create a NavigationHandle and have > the methods called on it. [+nick] I see that you're mostly using the TestFoo/FooTester/etc pattern correctly (apart from the RegisterThrottleForTesting method on NavigationHandle), but I'm just not a fan of that pattern. It's so heavyweight to read in the code, and Nick points out how undiscoverable it is for people wanting to write tests, as well as how likely it is to get out of date as the impl class changes. One straw man solution (if we just need to expose WillStartRequest and WillRedirectRequest to tests outside content/) is to put WillStartRequestForTesting and WillRedirectRequestForTesting on the public NavigationHandle. (Maybe a creation method as well?) In that approach, we don't need any of the extra test classes and it's obvious to test writers what functionality is available to them in NavigationHandle. It also makes the tests themselves easier to read (e.g., wouldn't have to wonder whether SimulateWillStartRequest does other things than just wrapping WillStartRequest). If that doesn't work, Nick has some ideas for an alternative approach that doesn't have some of the drawbacks of the TestFoo/FooTester pattern. I can be overruled here, but the current approach seems verbose and cumbersome to me.
I think the core of the CL looks good, but I concur with Charlie that a simpler way to do unit tests will be great. https://codereview.chromium.org/1269813002/diff/200001/content/test/test_navi... File content/test/test_navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/200001/content/test/test_navi... content/test/test_navigation_handle.h:17: class TestNavigationHandle : public NavigationHandleImpl, On 2015/09/16 17:39:27, Charlie Reis wrote: > On 2015/09/16 01:03:22, clamy wrote: > > On 2015/09/16 00:09:25, Charlie Reis (slow till 9-14) wrote: > > > I'm probably missing something here, but why do we need this class? It > looks > > > like it's just wrapping methods of NavigationHandleImpl which are already > > > public. > > > > > > I'm lamenting all the overhead with this test-specific class and the > > > NavigationHandleFactory. It would be nice to avoid TestNavigationHandle and > > > NavigationHandleFactory, if NavigationHandleImpl itself is primarily a data > > > structure. > > > > Well the goal here is to have embedders of content be able to write unit tests > > for NavigationThrottles, which is considerably simpler if you have a > > NavigationHandleTester class that allows you to have these methods explicitly > > called on the NavigationHandleImpl. Since NavigationHandleTester is a test > > interface, it can't be implemented by NavigationHandleImpl. And I added the > > factories to make sure that if people try to static_cast a > NavigationHandleImpl > > in its test version it does not create an error. > > > > I guess an alternative would be to have static methods on > > WebContentsTester/RenderFrameHostTester to create a NavigationHandle and have > > the methods called on it. > > [+nick] > > I see that you're mostly using the TestFoo/FooTester/etc pattern correctly > (apart from the RegisterThrottleForTesting method on NavigationHandle), but I'm > just not a fan of that pattern. It's so heavyweight to read in the code, and > Nick points out how undiscoverable it is for people wanting to write tests, as > well as how likely it is to get out of date as the impl class changes. > > One straw man solution (if we just need to expose WillStartRequest and > WillRedirectRequest to tests outside content/) is to put > WillStartRequestForTesting and WillRedirectRequestForTesting on the public > NavigationHandle. (Maybe a creation method as well?) In that approach, we > don't need any of the extra test classes and it's obvious to test writers what > functionality is available to them in NavigationHandle. It also makes the tests > themselves easier to read (e.g., wouldn't have to wonder whether > SimulateWillStartRequest does other things than just wrapping WillStartRequest). > > If that doesn't work, Nick has some ideas for an alternative approach that > doesn't have some of the drawbacks of the TestFoo/FooTester pattern. > > I can be overruled here, but the current approach seems verbose and cumbersome > to me. I'd second that a simpler way of testing is going to be better. It took me quite some time to even understand how it all works. https://codereview.chromium.org/1269813002/diff/220001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1269813002/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:161: // methods). It feels like a bit of a layering violation, but I think it is better than passing the two always together. Charlie, what is your take? https://codereview.chromium.org/1269813002/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1269813002/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1362: // Filter the url. nit: a redundant comment.
https://codereview.chromium.org/1269813002/diff/220001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1269813002/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:161: // methods). On 2015/09/16 20:33:39, nasko (slow to review) wrote: > It feels like a bit of a layering violation, but I think it is better than > passing the two always together. > > Charlie, what is your take? It's definitely a layering violation. :) That said, Camille made a good point that NavigationHandle and WebContentsObserver are tightly coupled, and I haven't yet been able to come up with a case where the NavigationHandle doesn't have a WebContents. I'm somewhat ok with allowing it, and hence my request for this comment. Just to explore the alternative-- it would be better from a layering perspective for frame_host classes not to depend on WebContents. If we went that route, NavigationHandle wouldn't know its WebContents, and callers would have to make that association on their own. This is doable but a bit clumsy and presents a risk of bugs if they get it wrong. Neither approach seems great, so perhaps the minor layering violation is better to avoid the potential bugs. Worst case, some NavigationHandles won't have a WebContents.
I am wondering if we could avoid that by exposing a GetDelegate method in the NavigationHandleImpl, and have the GetWebContents method not be virtual on NavigationHandle. Its implementation would static_cast the NavigatorDelegate returned by NavigationHandleImpl into a WebContents. This way we don't store a pointer to the WebContents in NavigationHandleImpl.
On 2015/09/16 20:51:41, clamy wrote: > I am wondering if we could avoid that by exposing a GetDelegate method in the > NavigationHandleImpl, and have the GetWebContents method not be virtual on > NavigationHandle. Its implementation would static_cast the NavigatorDelegate > returned by NavigationHandleImpl into a WebContents. This way we don't store a > pointer to the WebContents in NavigationHandleImpl. That makes the assumption that no other class in content/ will ever implement NavigatorDelegate. That could be a reasonable assumption, though it's a bit different than assuming FooImpl is the only implementation of Foo (which is common and explicitly supported). I'd be willing to see what that looks like. Nasko?
On 2015/09/16 21:15:37, Charlie Reis wrote: > On 2015/09/16 20:51:41, clamy wrote: > > I am wondering if we could avoid that by exposing a GetDelegate method in the > > NavigationHandleImpl, and have the GetWebContents method not be virtual on > > NavigationHandle. Its implementation would static_cast the NavigatorDelegate > > returned by NavigationHandleImpl into a WebContents. This way we don't store a > > pointer to the WebContents in NavigationHandleImpl. > > That makes the assumption that no other class in content/ will ever implement > NavigatorDelegate. That could be a reasonable assumption, though it's a bit > different than assuming FooImpl is the only implementation of Foo (which is > common and explicitly supported). > > I'd be willing to see what that looks like. Nasko? We already have InterstitialPageImpl as a NavigatorDelegate. Would that be a problem? I'm a bit torn, as I also don't like WebContentsImpl explicitly calling set_web_contents on the NavigationHandle. If we are careful and interstitials aren't a problem, I think giving that a try is worthwhile.
Thanks! I have added specific testing methods to NavigationHandle and removed the pointer to the WebContents from NavigationHandleImpl. https://codereview.chromium.org/1269813002/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1269813002/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1362: // Filter the url. On 2015/09/16 20:33:39, nasko (slow to review) wrote: > nit: a redundant comment. Done.
I think it looks good. I'd like to get confirmation from Charlie and Nick though. https://codereview.chromium.org/1269813002/diff/240001/content/content_browse... File content/content_browser.gypi (right): https://codereview.chromium.org/1269813002/diff/240001/content/content_browse... content/content_browser.gypi:186: 'public/browser/navigation_throttle.h', .h file goes below .cc https://codereview.chromium.org/1269813002/diff/240001/content/public/browser... File content/public/browser/navigation_handle.cc (right): https://codereview.chromium.org/1269813002/diff/240001/content/public/browser... content/public/browser/navigation_handle.cc:30: ->GetDelegate()); Uh, this is a fairly long chain. Why not use the RFH as input parameter? Also it will work nicely if we wanted to do navigations in subframes. https://codereview.chromium.org/1269813002/diff/240001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/240001/content/public/browser... content/public/browser/navigation_handle.h:81: nit: No empty line above. The statement below clarifies the testing methods heading.
Great. Mostly nits at this point, but two concerns I noticed about races and repeated thread hops. https://codereview.chromium.org/1269813002/diff/240001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1269813002/diff/240001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:53: << "This accessor should not be called before the request is started."; This message is confusing because DID_START sounds like it could refer to whether the request started. I had to go back and re-read to see that it's about the navigation start and not the network request start. Can we find a way to clarify this, either in the message or the enum value name? https://codereview.chromium.org/1269813002/diff/240001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1269813002/diff/240001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:129: friend class TestNavigationHandle; Don't forget to remove these. https://codereview.chromium.org/1269813002/diff/240001/content/browser/loader... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1269813002/diff/240001/content/browser/loader... content/browser/loader/navigation_resource_throttle.cc:64: render_frame_host->navigation_handle(); Are there races possible here? Suppose the UI thread has recently replaced the NavigationHandle with a different one, and a redirect comes in for the old one. Is that a problem? https://codereview.chromium.org/1269813002/diff/240001/content/browser/loader... content/browser/loader/navigation_resource_throttle.cc:100: BrowserThread::PostTask( Sanity check: This is doing a hop to the UI thread for every navigation, right? Are there performance concerns about this, and do we need to prevent other code from making repeated hops? For example, CrossSiteResourceHandler hops to the UI thread for policy checks in --site-per-process. Seems like we should avoid doing repeated hops. https://codereview.chromium.org/1269813002/diff/240001/content/public/browser... File content/public/browser/navigation_handle.cc (right): https://codereview.chromium.org/1269813002/diff/240001/content/public/browser... content/public/browser/navigation_handle.cc:14: // The NavigationHandleImpl cannot access the WebContentsImpl a it would be a nit: s/a/as/ https://codereview.chromium.org/1269813002/diff/240001/content/public/browser... content/public/browser/navigation_handle.cc:30: ->GetDelegate()); On 2015/09/17 21:10:25, nasko (slow to review) wrote: > Uh, this is a fairly long chain. Why not use the RFH as input parameter? Also it > will work nicely if we wanted to do navigations in subframes. I'm not sure we know the RFH at the time (e.g., in PlzNavigate). That said, won't this expression always evaluate to |web_contents|, just like the cast we're doing above? Seems like we could just use that.
Thanks! https://codereview.chromium.org/1269813002/diff/240001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1269813002/diff/240001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:53: << "This accessor should not be called before the request is started."; On 2015/09/18 05:40:02, Charlie Reis wrote: > This message is confusing because DID_START sounds like it could refer to > whether the request started. I had to go back and re-read to see that it's > about the navigation start and not the network request start. > > Can we find a way to clarify this, either in the message or the enum value name? I changed the state name to INITIAL. https://codereview.chromium.org/1269813002/diff/240001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1269813002/diff/240001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:129: friend class TestNavigationHandle; On 2015/09/18 05:40:02, Charlie Reis wrote: > Don't forget to remove these. Done. https://codereview.chromium.org/1269813002/diff/240001/content/browser/loader... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1269813002/diff/240001/content/browser/loader... content/browser/loader/navigation_resource_throttle.cc:64: render_frame_host->navigation_handle(); On 2015/09/18 05:40:02, Charlie Reis wrote: > Are there races possible here? Suppose the UI thread has recently replaced the > NavigationHandle with a different one, and a redirect comes in for the old one. > Is that a problem? Due to all IPCs being processed on the IO thread it is not. The issue could have come up if we started navigating to a page foo with redirects and then start navigating to bar. In blink::FrameLoader::StartLoad, a call to stop the provisional document loader is made (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). This will result in a ResourceHostMsg_CancelRequest being sent. The ResourceHostMsg_CancelRequest will cancel the request, leading to this being deleted. The DidStartProvisionalLoad IPC is sent after in blink::FrameLoader::StartLoad (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...), therefore it will arrive on the IO thread after the ResourceHostMsg_CancelRequest has been processed. In particular, any thread hops made by the NavigationResourceThrottle before its deletion will be processed on the UI thread before the DidStartProvisionalLoad, so before we create a new handler for the navigation. https://codereview.chromium.org/1269813002/diff/240001/content/browser/loader... content/browser/loader/navigation_resource_throttle.cc:100: BrowserThread::PostTask( On 2015/09/18 05:40:02, Charlie Reis wrote: > Sanity check: This is doing a hop to the UI thread for every navigation, right? > Are there performance concerns about this, and do we need to prevent other code > from making repeated hops? For example, CrossSiteResourceHandler hops to the UI > thread for policy checks in --site-per-process. Seems like we should avoid > doing repeated hops. On Android we are replacing a ResourceThrottle that made thread hops n every request, so this is similar performance wise. This is not the case on desktop, but I think thread hops are less of an issue in that case. The goal in the end is to remove all ResourceThrottles for navigations, which would ensure that only one thread hop is ever made for all navigations. For the CrossSiteResourceHandler, depending on when PlzNavigate and OOPIF ship, we could rewrite it as a NavigationThrottle, meaning we only also do one thread hop there. https://codereview.chromium.org/1269813002/diff/240001/content/content_browse... File content/content_browser.gypi (right): https://codereview.chromium.org/1269813002/diff/240001/content/content_browse... content/content_browser.gypi:186: 'public/browser/navigation_throttle.h', On 2015/09/17 21:10:25, nasko (slow to review) wrote: > .h file goes below .cc Done. https://codereview.chromium.org/1269813002/diff/240001/content/public/browser... File content/public/browser/navigation_handle.cc (right): https://codereview.chromium.org/1269813002/diff/240001/content/public/browser... content/public/browser/navigation_handle.cc:14: // The NavigationHandleImpl cannot access the WebContentsImpl a it would be a On 2015/09/18 05:40:02, Charlie Reis wrote: > nit: s/a/as/ Done. https://codereview.chromium.org/1269813002/diff/240001/content/public/browser... content/public/browser/navigation_handle.cc:30: ->GetDelegate()); On 2015/09/18 05:40:02, Charlie Reis wrote: > On 2015/09/17 21:10:25, nasko (slow to review) wrote: > > Uh, this is a fairly long chain. Why not use the RFH as input parameter? Also > it > > will work nicely if we wanted to do navigations in subframes. > > I'm not sure we know the RFH at the time (e.g., in PlzNavigate). > > That said, won't this expression always evaluate to |web_contents|, just like > the cast we're doing above? Seems like we could just use that. Done. https://codereview.chromium.org/1269813002/diff/240001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/240001/content/public/browser... content/public/browser/navigation_handle.h:81: On 2015/09/17 21:10:25, nasko (slow to review) wrote: > nit: No empty line above. The statement below clarifies the testing methods > heading. Done.
https://codereview.chromium.org/1269813002/diff/240001/content/browser/loader... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1269813002/diff/240001/content/browser/loader... content/browser/loader/navigation_resource_throttle.cc:100: BrowserThread::PostTask( On 2015/09/18 17:35:39, clamy wrote: > On 2015/09/18 05:40:02, Charlie Reis wrote: > > Sanity check: This is doing a hop to the UI thread for every navigation, > right? > > Are there performance concerns about this, and do we need to prevent other > code > > from making repeated hops? For example, CrossSiteResourceHandler hops to the > UI > > thread for policy checks in --site-per-process. Seems like we should avoid > > doing repeated hops. > > On Android we are replacing a ResourceThrottle that made thread hops n every > request, so this is similar performance wise. This is not the case on desktop, > but I think thread hops are less of an issue in that case. The goal in the end > is to remove all ResourceThrottles for navigations, which would ensure that only > one thread hop is ever made for all navigations. For the > CrossSiteResourceHandler, depending on when PlzNavigate and OOPIF ship, we could > rewrite it as a NavigationThrottle, meaning we only also do one thread hop > there. I don't think we can ensure only one thread hop per navigation. Each of the events (will send request, will redirect, ...) will require a separate hop on its own. I do agree that for each event, only one thread hop must be made, but we can't avoid multiple hops per navigation if we want to support the full NavigationHandle API.
Thanks! LGTM. https://codereview.chromium.org/1269813002/diff/240001/content/browser/loader... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1269813002/diff/240001/content/browser/loader... content/browser/loader/navigation_resource_throttle.cc:64: render_frame_host->navigation_handle(); On 2015/09/18 17:35:39, clamy wrote: > On 2015/09/18 05:40:02, Charlie Reis wrote: > > Are there races possible here? Suppose the UI thread has recently replaced > the > > NavigationHandle with a different one, and a redirect comes in for the old > one. > > Is that a problem? > > Due to all IPCs being processed on the IO thread it is not. The issue could have > come up if we started navigating to a page foo with redirects and then start > navigating to bar. In blink::FrameLoader::StartLoad, a call to stop the > provisional document loader is made > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > This will result in a ResourceHostMsg_CancelRequest being sent. The > ResourceHostMsg_CancelRequest will cancel the request, leading to this being > deleted. The DidStartProvisionalLoad IPC is sent after in > blink::FrameLoader::StartLoad > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...), > therefore it will arrive on the IO thread after the > ResourceHostMsg_CancelRequest has been processed. In particular, any thread hops > made by the NavigationResourceThrottle before its deletion will be processed on > the UI thread before the DidStartProvisionalLoad, so before we create a new > handler for the navigation. Acknowledged. https://codereview.chromium.org/1269813002/diff/240001/content/browser/loader... content/browser/loader/navigation_resource_throttle.cc:100: BrowserThread::PostTask( On 2015/09/18 17:35:39, clamy wrote: > On 2015/09/18 05:40:02, Charlie Reis wrote: > > Sanity check: This is doing a hop to the UI thread for every navigation, > right? > > Are there performance concerns about this, and do we need to prevent other > code > > from making repeated hops? For example, CrossSiteResourceHandler hops to the > UI > > thread for policy checks in --site-per-process. Seems like we should avoid > > doing repeated hops. > > On Android we are replacing a ResourceThrottle that made thread hops n every > request, so this is similar performance wise. This is not the case on desktop, > but I think thread hops are less of an issue in that case. The goal in the end > is to remove all ResourceThrottles for navigations, which would ensure that only > one thread hop is ever made for all navigations. For the > CrossSiteResourceHandler, depending on when PlzNavigate and OOPIF ship, we could > rewrite it as a NavigationThrottle, meaning we only also do one thread hop > there. Acknowledged. https://codereview.chromium.org/1269813002/diff/240001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/240001/content/public/browser... content/public/browser/navigation_handle.h:81: On 2015/09/18 17:35:39, clamy wrote: > On 2015/09/17 21:10:25, nasko (slow to review) wrote: > > nit: No empty line above. The statement below clarifies the testing methods > > heading. > > Done. nit: We should be consistent with the sections above (i.e., lines 26 and 42). I'm fine with either no blank line or having a blank comment line starting with "//".
clamy@chromium.org changed reviewers: + asargent@chromium.org, mnaganov@chromium.org, sky@chromium.org
Thanks! @asargent: PTAL at the changes in chrome/browser/apps. @sky: PTAL at the changes in chrome/browser/renderer_host @mnaganov: PTAL at the changes in components/navigation_interception https://codereview.chromium.org/1269813002/diff/240001/content/browser/loader... File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1269813002/diff/240001/content/browser/loader... content/browser/loader/navigation_resource_throttle.cc:100: BrowserThread::PostTask( On 2015/09/18 17:58:19, nasko (slow to review) wrote: > On 2015/09/18 17:35:39, clamy wrote: > > On 2015/09/18 05:40:02, Charlie Reis wrote: > > > Sanity check: This is doing a hop to the UI thread for every navigation, > > right? > > > Are there performance concerns about this, and do we need to prevent other > > code > > > from making repeated hops? For example, CrossSiteResourceHandler hops to > the > > UI > > > thread for policy checks in --site-per-process. Seems like we should avoid > > > doing repeated hops. > > > > On Android we are replacing a ResourceThrottle that made thread hops n every > > request, so this is similar performance wise. This is not the case on desktop, > > but I think thread hops are less of an issue in that case. The goal in the end > > is to remove all ResourceThrottles for navigations, which would ensure that > only > > one thread hop is ever made for all navigations. For the > > CrossSiteResourceHandler, depending on when PlzNavigate and OOPIF ship, we > could > > rewrite it as a NavigationThrottle, meaning we only also do one thread hop > > there. > > I don't think we can ensure only one thread hop per navigation. Each of the > events (will send request, will redirect, ...) will require a separate hop on > its own. I do agree that for each event, only one thread hop must be made, but > we can't avoid multiple hops per navigation if we want to support the full > NavigationHandle API. Yes that's what I meant (but my wording was not good :).
components/navigation_interception/ LGTM
chrome/browser/apps lgtm with one question mostly for my own education https://codereview.chromium.org/1269813002/diff/260001/chrome/browser/apps/ap... File chrome/browser/apps/app_url_redirector.cc (right): https://codereview.chromium.org/1269813002/diff/260001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.cc:82: if (handle->IsPost()) { You've likely already thought about this, but is it safe to assume that NavigationHandle's will only ever concern themselves with GET and POST requests, and not other request methods like HEAD, PUT, DELETE, etc.?
https://codereview.chromium.org/1269813002/diff/260001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (left): https://codereview.chromium.org/1269813002/diff/260001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:357: AppUrlRedirector::MaybeCreateThrottleFor(request, io_data); Where is the non-android code moving to?
https://codereview.chromium.org/1269813002/diff/260001/chrome/browser/apps/ap... File chrome/browser/apps/app_url_redirector.cc (right): https://codereview.chromium.org/1269813002/diff/260001/chrome/browser/apps/ap... chrome/browser/apps/app_url_redirector.cc:82: if (handle->IsPost()) { On 2015/09/18 19:47:38, Antony Sargent wrote: > You've likely already thought about this, but is it safe to assume that > NavigationHandle's will only ever concern themselves with GET and POST requests, > and not other request methods like HEAD, PUT, DELETE, etc.? > We have DCHECKs in place for that. Otherwise the code looks the same as what is currently being done in the InterceptNavigationResourceThrottle (https://code.google.com/p/chromium/codesearch#chromium/src/components/navigat...). https://codereview.chromium.org/1269813002/diff/260001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (left): https://codereview.chromium.org/1269813002/diff/260001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:357: AppUrlRedirector::MaybeCreateThrottleFor(request, io_data); On 2015/09/18 20:24:57, sky wrote: > Where is the non-android code moving to? It's moving to chrome/browser/chrome_content_browser_client.cc.
LGTM
Patchset #15 (id:280001) has been deleted
Thanks! https://codereview.chromium.org/1269813002/diff/240001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1269813002/diff/240001/content/public/browser... content/public/browser/navigation_handle.h:81: On 2015/09/18 17:58:39, Charlie Reis wrote: > On 2015/09/18 17:35:39, clamy wrote: > > On 2015/09/17 21:10:25, nasko (slow to review) wrote: > > > nit: No empty line above. The statement below clarifies the testing methods > > > heading. > > > > Done. > > nit: We should be consistent with the sections above (i.e., lines 26 and 42). > I'm fine with either no blank line or having a blank comment line starting with > "//". Done. I went with having a line starting with // as I find it more readable.
The CQ bit was checked by clamy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mnaganov@chromium.org, sky@chromium.org, asargent@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/1269813002/#ps300001 (title: "Rebase + addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1269813002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1269813002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by clamy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mnaganov@chromium.org, sky@chromium.org, asargent@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/1269813002/#ps320001 (title: "Fixed compilation error on Android")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1269813002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1269813002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...) linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
@mnaganov: I had to modify some Android webview code. Is it still good for you?
On 2015/09/21 18:49:36, clamy wrote: > @mnaganov: I had to modify some Android webview code. Is it still good for you? Yeah, I was wandering how could you do that without touching it :) LGTM, let's see what the tests will show.
The CQ bit was checked by clamy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, asargent@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/1269813002/#ps340001 (title: "Rebase + android_webview + Linux compilation error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1269813002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1269813002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1269813002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1269813002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by clamy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mnaganov@chromium.org, sky@chromium.org, asargent@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/1269813002/#ps350001 (title: "Rebase on https://codereview.chromium.org/1312213010/")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1269813002/350001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1269813002/350001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by clamy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1269813002/350001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1269813002/350001
Message was sent while issue was closed.
Committed patchset #18 (id:350001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/394057986ff5d00a841a976f3bd8d599603acaa1 Cr-Commit-Position: refs/heads/master@{#350092}
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:350001) has been created in https://codereview.chromium.org/1350913008/ by huangs@chromium.org. The reason for reverting is: linux_chromiium_chromeos_rel_ng => browser_test => SelectFileDialogExtensionBrowserTest seems to faile a lot, testing..
Message was sent while issue was closed.
Confirmed: Recent browser tests on linux_chromium_chromeos_rel_ng consistently suffer from FATAL:process_manager.cc(749)] Check failed: count > 0 || !extension_registry_->enabled_extensions().Contains(extension_id). in various manifestations (even if linux_chromium_chromeos_rel_ng is green): SelectFileDialogExtensionBrowserTest.CreateAndDestroy SelectFileDialogExtensionBrowserTest.DestroyListener PlatformAppIncognitoBrowserTest.IncognitoComponentApp
Message was sent while issue was closed.
Reverting. |
