Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2545943003:
Accessing navigation information via webcontents (Closed)
DescriptionSummary
-----
With PlzNavigate, main frame navigations
don't have a valid (render_process_id, render_frame_id). This CL is a
preliminary refactoring to properly support PlzNavigate.
Details
-----
To be able to do browser navigations to non yet created frames, we use
webcontents that will always be available. This is preparation work
to be able to use webcontents identification instead of frame ids easily.
At the higher level we are passing a WebContentsGetter and run it on a
UI thread, leading to new dedicated functions in the observer. We then
use a wrapper to initialize the NavigationID part of the request summary.
SummarizeResponse is not responsible for filling in NavigationID any more,
and the behavior has been updated in unit tests.
BUG=631966
Committed: https://crrev.com/d38f1d7f0c8d8caf40232f071269b21288b9452e
Cr-Commit-Position: refs/heads/master@{#437524}
Patch Set 1 #Patch Set 2 : Fixed SummarizeResponse #
Total comments: 25
Patch Set 3 : Post-Review Modifications #
Total comments: 22
Patch Set 4 : Post-Review Modifications #2 #
Total comments: 11
Patch Set 5 : Post-Review Modifications #3 #Messages
Total messages: 51 (31 generated)
The CQ bit was checked by ahemery@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ahemery@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Accessing navigation information via webcontents What ? We are now accessing the frame information (render_process_id and render_frame_id) using a WebContents. The observer always receives a WebContentsGetter that grant access to a richer interface. Why ? To be able to do browser navigations to non yet created frames, we use webcontents that will always be available. This is preparation work to be able to use webcontents identification instead of frame ids easily. How ? At the higher level we are passing a WebContentsGetter and run it on a UI thread, leading to 3 new functions. We factor the WebContentsGetter to NavigationID work into the RetrieveNavigationID function. BUG=631966 ========== to ========== Accessing navigation information via webcontents What ? We are now accessing the frame information (render_process_id and render_frame_id) using a WebContents. The observer always receives a WebContentsGetter that grant access to a richer interface. Why ? To be able to do browser navigations to non yet created frames, we use webcontents that will always be available. This is preparation work to be able to use webcontents identification instead of frame ids easily. How ? At the higher level we are passing a WebContentsGetter and run it on a UI thread, leading to 3 new functions. We factor the WebContentsGetter to NavigationID work into the RetrieveNavigationID function. SummarizeResponse is not responsible for filling in NavigationID any more, and the behavior has been updated in unit tests. BUG=631966 ==========
Description was changed from ========== Accessing navigation information via webcontents What ? We are now accessing the frame information (render_process_id and render_frame_id) using a WebContents. The observer always receives a WebContentsGetter that grant access to a richer interface. Why ? To be able to do browser navigations to non yet created frames, we use webcontents that will always be available. This is preparation work to be able to use webcontents identification instead of frame ids easily. How ? At the higher level we are passing a WebContentsGetter and run it on a UI thread, leading to 3 new functions. We factor the WebContentsGetter to NavigationID work into the RetrieveNavigationID function. SummarizeResponse is not responsible for filling in NavigationID any more, and the behavior has been updated in unit tests. BUG=631966 ========== to ========== Accessing navigation information via webcontents What ? We are now accessing the frame information (render_process_id and render_frame_id) using a WebContents. The observer always receives a WebContentsGetter that grant access to a richer interface. Why ? To be able to do browser navigations to non yet created frames, we use webcontents that will always be available. This is preparation work to be able to use webcontents identification instead of frame ids easily. How ? At the higher level we are passing a WebContentsGetter and run it on a UI thread, leading to 3 new functions. We factor the WebContentsGetter to NavigationID work into the RetrieveNavigationID function. SummarizeResponse is not responsible for filling in NavigationID any more, and the behavior has been updated in unit tests. BUG=631966 ==========
Description was changed from ========== Accessing navigation information via webcontents What ? We are now accessing the frame information (render_process_id and render_frame_id) using a WebContents. The observer always receives a WebContentsGetter that grant access to a richer interface. Why ? To be able to do browser navigations to non yet created frames, we use webcontents that will always be available. This is preparation work to be able to use webcontents identification instead of frame ids easily. How ? At the higher level we are passing a WebContentsGetter and run it on a UI thread, leading to 3 new functions. We factor the WebContentsGetter to NavigationID work into the RetrieveNavigationID function. SummarizeResponse is not responsible for filling in NavigationID any more, and the behavior has been updated in unit tests. BUG=631966 ========== to ========== Accessing navigation information via webcontents What ? We are now accessing the frame information (render_process_id and render_frame_id) using a WebContents. The observer always receives a WebContentsGetter that grant access to a richer interface. Why ? To be able to do browser navigations to non yet created frames, we use webcontents that will always be available. This is preparation work to be able to use webcontents identification instead of frame ids easily. How ? At the higher level we are passing a WebContentsGetter and run it on a UI thread, leading to 3 new functions. We factor the WebContentsGetter to NavigationID work into the RetrieveNavigationID function. SummarizeResponse is not responsible for filling in NavigationID any more, and the behavior has been updated in unit tests. BUG=631966 ==========
ahemery@chromium.org changed reviewers: + alexilin@chromium.org, lizeb@chromium.org
First part of the change I'm working on. No behavior change for now, just prep work. https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:7: #include <memory> for unique_ptr https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:10: #include "base/memory/ptr_util.h" For MakeUnique and base::Passed https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:86: auto summary = base::MakeUnique<URLRequestSummary>(); Now using a pointer as we are passing it to a different thread https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:129: summary->navigation_id.main_frame_url = request->first_party_for_cookies(); main_frame_url and creation_time is not done in SummarizeResponse anymore. https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... File chrome/browser/net/resource_prefetch_predictor_observer.h (right): https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.h:29: Not sure this is okay in a header file, even if inside a namespace https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (left): https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:333: int render_process_id, render_frame_id; SummarizeResponse does not have access to a web content. NavigationID filling is now done separately. https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.cc (right): https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.cc:328: const content::ResourceRequestInfo* request_info = Just renaming https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:1334: // navigation_id elements should be unset by default Simply reflecting the changes commented in SummarizeResponse. Note: Cannot directly compare NavigationID's to avoid DCHECK(is_valid) triggering
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, here is some comments. https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:102: void ResourcePrefetchPredictorObserver::OnRequestStartedOnUIThread( nit: Could you put all definitions of private functions to the end of file? Function declaration order should match function definition order: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:208: predictors::NavigationID& navigation_id, Pass an object by pointer if you want to change its state. All parameters passed by reference must be labeled const: https://google.github.io/styleguide/cppguide.html#Reference_Arguments https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:213: DCHECK(web_contents); Based on comment to GetWebContentsGetterForRequest() function the callback WebContentsGetter can return nullptr so it shouldn't be DCHECK. I suggest to make return type bool for this function and return false if web_contents_getter returned false. Outside callers also should check return value of this function. https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:214: navigation_id.render_process_id = nit: I'm wondering if we can eliminate the mess with NavigationID constructors. For example, we could use explicit NavigationID(content::WebContents* web_contents); here and delete NavigationID(int render_process_id, int render_frame_id, const GURL& main_frame_url); at all. https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... File chrome/browser/net/resource_prefetch_predictor_observer.h (right): https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.h:27: // called on the IO thread. Can be destroyed on UI or IO thread. Probably we should update this comment. I'm not sure whether class comment describes public interface only (lizeb?). https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.h:29: On 2016/12/05 09:38:45, ahemery wrote: > Not sure this is okay in a header file, even if inside a namespace We generally avoid the use of using declaration inside header files. Please use fully qualified name here. https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:99: // Returns true for success. Note: Navigation ID has particular nit: Note: NavigationID is not initialized by this function. https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:1334: // navigation_id elements should be unset by default nit: add period to the end of the comment. https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G...
Hi, Thanks for the detailed explanations in the commit message and in the diff. A few high-level comments. https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:207: void ResourcePrefetchPredictorObserver::RetrieveNavigationID( Actually, considering the remark below, this function should return a boolean, and be named perhaps "MaybeSetNavigationIDFromWebContents()" or something along these lines. https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:208: predictors::NavigationID& navigation_id, We don't use non-const references in Chrome. See https://google.github.io/styleguide/cppguide.html#Reference_Arguments The alternative is to use a pointer. https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:213: DCHECK(web_contents); A DCHECK() is not correct here, as the returned WebContents can be nullptr. Early return with a comment is better here. There are several reasons for which the returned WebContents can be nullptr: - the WebContents has been destroyed in the meantime - the request was never associated with a WebContents (see the comment in ResourceRequestInfoImpl::GetWebContentsGetterForRequest()) https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:214: navigation_id.render_process_id = The partial initialization of NavigationID here is a bit too weird. Can you fully initialize it in one place? https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:99: // Returns true for success. Note: Navigation ID has particular Thanks for documenting this.
The CQ bit was checked by ahemery@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:100: } Private methods moved at the end of the file https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... File chrome/browser/net/resource_prefetch_predictor_observer.h (right): https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.h:30: using URLRequestSummary = Removed and full names used in header file https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:1323: GURL emptyUrl; Just using GURL() https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/net/reso... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:9: #include <utility> For std::move https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:23: using URLRequestSummary = Added in .cc file because removed from .h https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:98: web_contents_getter, request->first_party_for_cookies(), The new parameters that get passed all the way to TryToFillNavigationID. Same for two other functions below https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:127: summary->redirect_url = redirect_url; Moved from above, no modification https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:189: if (!TryToFillNavigationID(&summary->navigation_id, Now checking for failure. Returning without any further action was the initial behavior if the Summary could not get produced. https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:224: bool ResourcePrefetchPredictorObserver::TryToFillNavigationID( Modified name to make it clearer that it can fail https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:234: *navigation_id = predictors::NavigationID(web_contents, Using a direct constructor here https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/net/reso... File chrome/browser/net/resource_prefetch_predictor_observer.h (right): https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.h:8: #include <memory> For unique_ptr https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.h:52: Comment about the additional parameters. WebContents is only accessible from UI thread, and request is tightly coupled to the IO thread. Therefore we extract necessary information in the IO thread and pass it along to the UI thread to be used. We completely fill the navigation_id in a single place. https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_common.h (left): https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common.h:41: NavigationID(int render_process_id, This version was only used in test. Removed. https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_common.h (right): https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common.h:42: explicit NavigationID(content::WebContents* web_contents, New constructor, necessary to fully initialize the navigationID https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor.h (right): https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor.h:99: // Returns true for success. Note: NavigationID is NOT initialized Reduced comment following alex remark https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_test_util.cc (right): https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_test_util.cc:66: NavigationID navigation_id; Producing navigation_id manually instead of exposing a different constructor in the interface
Patchset #4 (id:60001) has been deleted
Thanks, a few more comments, only minor ones. https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/net/reso... File chrome/browser/net/resource_prefetch_predictor_observer.h (right): https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.h:8: #include <memory> On 2016/12/06 14:31:12, ahemery wrote: > For unique_ptr Thanks for adding the comments in the review, but these are not required, as we trust you to have run "git cl lint". https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.h:76: bool TryToFillNavigationID( This can be made static, right? If so, please make it a standalone function in an anonymous namespace in the .cc file. https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_common.h (right): https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common.h:42: explicit NavigationID(content::WebContents* web_contents, nit: Multiple parameters constructors don't need to be explicit. https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetcher_unittest.cc (right): https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetcher_unittest.cc:156: GURL main_frame_url(main_frame_url_string); nit: You can use main_frame_url.spec() to avoid adding the string. https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetcher_unittest.cc:234: std::string main_frame_url_string("http://www.google.com"); nit: ditto.
https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_common.cc (right): https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common.cc:19: #include "net/url_request/url_request.h" Relic from try&error https://codereview.chromium.org/2545943003/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetcher_unittest.cc (right): https://codereview.chromium.org/2545943003/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetcher_unittest.cc:155: GURL main_frame_url("http://www.google.com"); Modified this to be a bit cleaner
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/net/reso... File chrome/browser/net/resource_prefetch_predictor_observer.h (right): https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.h:76: bool TryToFillNavigationID( Now an implementation detail https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_common.h (right): https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_common.h:42: explicit NavigationID(content::WebContents* web_contents, Removed explicit https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetcher_unittest.cc (right): https://codereview.chromium.org/2545943003/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetcher_unittest.cc:156: GURL main_frame_url(main_frame_url_string); Simplified
Thanks, lgtm with nits. You also need to add OWNERS of chrome/browser/net and chrome/browser/loader to pass this review. https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://codereview.chromium.org/2545943003/diff/20001/chrome/browser/net/reso... chrome/browser/net/resource_prefetch_predictor_observer.cc:100: } On 2016/12/06 14:31:11, ahemery wrote: > Private methods moved at the end of the file Thanks! Could you reply directly under initial comment pressing "reply" button next time? Then it would be clearer that comment was answered. https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:72: *navigation_id = predictors::NavigationID(web_contents, Would you run git cl format? https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:207: creation_time)) nit: git cl format here too + There is no strict rule but we prefer to put curly braces even for single-line conditional statements if the condition takes more than one line. The same thing about when the conditional block itself is single statement but takes two or more lines. https://google.github.io/styleguide/cppguide.html#Conditionals https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:219: if (!TryToFillNavigationID(&summary->navigation_id, ditto https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:233: if (!TryToFillNavigationID(&summary->navigation_id, ditto
A few remaining minor comments as well. Also, as Alex mentioned, please run "git cl format" and "git cl lint", if this has not been done. We welcome our robots formatting overlords. https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:15: #include "content/public/browser/render_process_host.h" nit: Is this header needed? https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:17: #include "content/public/browser/web_contents.h" nit: Would a simple forward declaration be enough here? https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:186: &ResourcePrefetchPredictorObserver::OnResponseStartedOnUIThread, Question: is it necessary to fully qualify the name here?
ahemery@chromium.org changed reviewers: + rdsmith@chromium.org
ahemery@chromium.org changed reviewers: + rdsmith@chromium.org
https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:15: #include "content/public/browser/render_process_host.h" On 2016/12/06 16:09:22, Benoit L wrote: > nit: Is this header needed? Removed https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:17: #include "content/public/browser/web_contents.h" On 2016/12/06 16:09:22, Benoit L wrote: > nit: Would a simple forward declaration be enough here? Moved to forward declaration https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:72: *navigation_id = predictors::NavigationID(web_contents, On 2016/12/06 15:51:54, alexilin wrote: > Would you run git cl format? Done https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:207: creation_time)) On 2016/12/06 15:51:54, alexilin wrote: > nit: git cl format here too > + There is no strict rule but we prefer to put curly braces even for single-line > conditional statements if the condition takes more than one line. The same thing > about when the conditional block itself is single statement but takes two or > more lines. > https://google.github.io/styleguide/cppguide.html#Conditionals Modified to prefered format
https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:15: #include "content/public/browser/render_process_host.h" On 2016/12/06 16:09:22, Benoit L wrote: > nit: Is this header needed? Removed https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:17: #include "content/public/browser/web_contents.h" On 2016/12/06 16:09:22, Benoit L wrote: > nit: Would a simple forward declaration be enough here? Moved to forward declaration https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:72: *navigation_id = predictors::NavigationID(web_contents, On 2016/12/06 15:51:54, alexilin wrote: > Would you run git cl format? Done https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... chrome/browser/net/resource_prefetch_predictor_observer.cc:207: creation_time)) On 2016/12/06 15:51:54, alexilin wrote: > nit: git cl format here too > + There is no strict rule but we prefer to put curly braces even for single-line > conditional statements if the condition takes more than one line. The same thing > about when the conditional block itself is single statement but takes two or > more lines. > https://google.github.io/styleguide/cppguide.html#Conditionals Modified to prefered format
Patchset #5 (id:120001) has been deleted
The CQ bit was checked by ahemery@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/07 09:17:05, ahemery wrote: > https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... > File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): > > https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... > chrome/browser/net/resource_prefetch_predictor_observer.cc:15: #include > "content/public/browser/render_process_host.h" > On 2016/12/06 16:09:22, Benoit L wrote: > > nit: Is this header needed? > > Removed > > https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... > chrome/browser/net/resource_prefetch_predictor_observer.cc:17: #include > "content/public/browser/web_contents.h" > On 2016/12/06 16:09:22, Benoit L wrote: > > nit: Would a simple forward declaration be enough here? > > Moved to forward declaration > > https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... > chrome/browser/net/resource_prefetch_predictor_observer.cc:72: *navigation_id = > predictors::NavigationID(web_contents, > On 2016/12/06 15:51:54, alexilin wrote: > > Would you run git cl format? > > Done > > https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... > chrome/browser/net/resource_prefetch_predictor_observer.cc:207: creation_time)) > On 2016/12/06 15:51:54, alexilin wrote: > > nit: git cl format here too > > + There is no strict rule but we prefer to put curly braces even for > single-line > > conditional statements if the condition takes more than one line. The same > thing > > about when the conditional block itself is single statement but takes two or > > more lines. > > https://google.github.io/styleguide/cppguide.html#Conditionals > > Modified to prefered format About the commit message: - The proper bug is https://bugs.chromium.org/p/chromium/issues/detail?id=650246, not the one linked. - The comment is no longer up to date, as the code has changed. You can update it with "git cl description" or through the web interface. - Thanks for adding that many details in the commit message, but you could be a bit terser. The gist of the issue is "With PlzNavigate, main frame navigations don't have a valid (render_process_id, render_frame_id). This CL is a preliminary refactoring to properly support PlzNavigate". Up to you. That being said, this level of detail is definitively useful for code review.
Description was changed from ========== Accessing navigation information via webcontents What ? We are now accessing the frame information (render_process_id and render_frame_id) using a WebContents. The observer always receives a WebContentsGetter that grant access to a richer interface. Why ? To be able to do browser navigations to non yet created frames, we use webcontents that will always be available. This is preparation work to be able to use webcontents identification instead of frame ids easily. How ? At the higher level we are passing a WebContentsGetter and run it on a UI thread, leading to 3 new functions. We factor the WebContentsGetter to NavigationID work into the RetrieveNavigationID function. SummarizeResponse is not responsible for filling in NavigationID any more, and the behavior has been updated in unit tests. BUG=631966 ========== to ========== Summary ----- With PlzNavigate, main frame navigations don't have a valid (render_process_id, render_frame_id). This CL is a preliminary refactoring to properly support PlzNavigate. Details ----- To be able to do browser navigations to non yet created frames, we use webcontents that will always be available. This is preparation work to be able to use webcontents identification instead of frame ids easily. At the higher level we are passing a WebContentsGetter and run it on a UI thread, leading to new dedicated functions in the observer. We then use a wrapper to initialize the NavigationID part of the request summary. SummarizeResponse is not responsible for filling in NavigationID any more, and the behavior has been updated in unit tests. BUG=631966 ==========
On 2016/12/07 09:50:55, Benoit L wrote: > On 2016/12/07 09:17:05, ahemery wrote: > > > https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... > > File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): > > > > > https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... > > chrome/browser/net/resource_prefetch_predictor_observer.cc:15: #include > > "content/public/browser/render_process_host.h" > > On 2016/12/06 16:09:22, Benoit L wrote: > > > nit: Is this header needed? > > > > Removed > > > > > https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... > > chrome/browser/net/resource_prefetch_predictor_observer.cc:17: #include > > "content/public/browser/web_contents.h" > > On 2016/12/06 16:09:22, Benoit L wrote: > > > nit: Would a simple forward declaration be enough here? > > > > Moved to forward declaration > > > > > https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... > > chrome/browser/net/resource_prefetch_predictor_observer.cc:72: *navigation_id > = > > predictors::NavigationID(web_contents, > > On 2016/12/06 15:51:54, alexilin wrote: > > > Would you run git cl format? > > > > Done > > > > > https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... > > chrome/browser/net/resource_prefetch_predictor_observer.cc:207: > creation_time)) > > On 2016/12/06 15:51:54, alexilin wrote: > > > nit: git cl format here too > > > + There is no strict rule but we prefer to put curly braces even for > > single-line > > > conditional statements if the condition takes more than one line. The same > > thing > > > about when the conditional block itself is single statement but takes two or > > > more lines. > > > https://google.github.io/styleguide/cppguide.html#Conditionals > > > > Modified to prefered format > > About the commit message: > - The proper bug is > https://bugs.chromium.org/p/chromium/issues/detail?id=650246, not the one > linked. > - The comment is no longer up to date, as the code has changed. You can update > it with "git cl description" or through the web interface. > - Thanks for adding that many details in the commit message, but you could be a > bit terser. The gist of the issue is "With PlzNavigate, main frame navigations > don't have a valid (render_process_id, render_frame_id). This CL is a > preliminary refactoring to properly support PlzNavigate". Up to you. That being > said, this level of detail is definitively useful for code review. Updated the description
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/07 10:05:04, ahemery wrote: > On 2016/12/07 09:50:55, Benoit L wrote: > > On 2016/12/07 09:17:05, ahemery wrote: > > > > > > https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... > > > File chrome/browser/net/resource_prefetch_predictor_observer.cc (right): > > > > > > > > > https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... > > > chrome/browser/net/resource_prefetch_predictor_observer.cc:15: #include > > > "content/public/browser/render_process_host.h" > > > On 2016/12/06 16:09:22, Benoit L wrote: > > > > nit: Is this header needed? > > > > > > Removed > > > > > > > > > https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... > > > chrome/browser/net/resource_prefetch_predictor_observer.cc:17: #include > > > "content/public/browser/web_contents.h" > > > On 2016/12/06 16:09:22, Benoit L wrote: > > > > nit: Would a simple forward declaration be enough here? > > > > > > Moved to forward declaration > > > > > > > > > https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... > > > chrome/browser/net/resource_prefetch_predictor_observer.cc:72: > *navigation_id > > = > > > predictors::NavigationID(web_contents, > > > On 2016/12/06 15:51:54, alexilin wrote: > > > > Would you run git cl format? > > > > > > Done > > > > > > > > > https://codereview.chromium.org/2545943003/diff/100001/chrome/browser/net/res... > > > chrome/browser/net/resource_prefetch_predictor_observer.cc:207: > > creation_time)) > > > On 2016/12/06 15:51:54, alexilin wrote: > > > > nit: git cl format here too > > > > + There is no strict rule but we prefer to put curly braces even for > > > single-line > > > > conditional statements if the condition takes more than one line. The same > > > thing > > > > about when the conditional block itself is single statement but takes two > or > > > > more lines. > > > > https://google.github.io/styleguide/cppguide.html#Conditionals > > > > > > Modified to prefered format > > > > About the commit message: > > - The proper bug is > > https://bugs.chromium.org/p/chromium/issues/detail?id=650246, not the one > > linked. > > - The comment is no longer up to date, as the code has changed. You can update > > it with "git cl description" or through the web interface. > > - Thanks for adding that many details in the commit message, but you could be > a > > bit terser. The gist of the issue is "With PlzNavigate, main frame navigations > > don't have a valid (render_process_id, render_frame_id). This CL is a > > preliminary refactoring to properly support PlzNavigate". Up to you. That > being > > said, this level of detail is definitively useful for code review. > Updated the description LGTM, thanks.
chrome/browser/{loader,net} LGTM.
The CQ bit was checked by ahemery@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexilin@chromium.org Link to the patchset: https://codereview.chromium.org/2545943003/#ps140001 (title: "Post-Review Modifications #3")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by ahemery@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481286599736080,
"parent_rev": "d8dafe61e38d37a375205d407b8f9c6bd27c1fcc", "commit_rev":
"de4be85b319f2c32242ba54c4278270de2017a0b"}
Message was sent while issue was closed.
Description was changed from ========== Summary ----- With PlzNavigate, main frame navigations don't have a valid (render_process_id, render_frame_id). This CL is a preliminary refactoring to properly support PlzNavigate. Details ----- To be able to do browser navigations to non yet created frames, we use webcontents that will always be available. This is preparation work to be able to use webcontents identification instead of frame ids easily. At the higher level we are passing a WebContentsGetter and run it on a UI thread, leading to new dedicated functions in the observer. We then use a wrapper to initialize the NavigationID part of the request summary. SummarizeResponse is not responsible for filling in NavigationID any more, and the behavior has been updated in unit tests. BUG=631966 ========== to ========== Summary ----- With PlzNavigate, main frame navigations don't have a valid (render_process_id, render_frame_id). This CL is a preliminary refactoring to properly support PlzNavigate. Details ----- To be able to do browser navigations to non yet created frames, we use webcontents that will always be available. This is preparation work to be able to use webcontents identification instead of frame ids easily. At the higher level we are passing a WebContentsGetter and run it on a UI thread, leading to new dedicated functions in the observer. We then use a wrapper to initialize the NavigationID part of the request summary. SummarizeResponse is not responsible for filling in NavigationID any more, and the behavior has been updated in unit tests. BUG=631966 Review-Url: https://codereview.chromium.org/2545943003 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Summary ----- With PlzNavigate, main frame navigations don't have a valid (render_process_id, render_frame_id). This CL is a preliminary refactoring to properly support PlzNavigate. Details ----- To be able to do browser navigations to non yet created frames, we use webcontents that will always be available. This is preparation work to be able to use webcontents identification instead of frame ids easily. At the higher level we are passing a WebContentsGetter and run it on a UI thread, leading to new dedicated functions in the observer. We then use a wrapper to initialize the NavigationID part of the request summary. SummarizeResponse is not responsible for filling in NavigationID any more, and the behavior has been updated in unit tests. BUG=631966 Review-Url: https://codereview.chromium.org/2545943003 ========== to ========== Summary ----- With PlzNavigate, main frame navigations don't have a valid (render_process_id, render_frame_id). This CL is a preliminary refactoring to properly support PlzNavigate. Details ----- To be able to do browser navigations to non yet created frames, we use webcontents that will always be available. This is preparation work to be able to use webcontents identification instead of frame ids easily. At the higher level we are passing a WebContentsGetter and run it on a UI thread, leading to new dedicated functions in the observer. We then use a wrapper to initialize the NavigationID part of the request summary. SummarizeResponse is not responsible for filling in NavigationID any more, and the behavior has been updated in unit tests. BUG=631966 Committed: https://crrev.com/d38f1d7f0c8d8caf40232f071269b21288b9452e Cr-Commit-Position: refs/heads/master@{#437524} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d38f1d7f0c8d8caf40232f071269b21288b9452e Cr-Commit-Position: refs/heads/master@{#437524} |
