Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
874353003:
New TimeToFirstURLJob* navigation metrics now work with PlzNavigate. (Closed)
|
Created:
5 years, 10 months ago by carlosk Modified:
5 years, 10 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNew TimeToURLJobStart* navigation metrics now work with PlzNavigate.
They weren't being properly reported before (the report happened later than it
should) but now there's two different reporting points for current navigation
and for when PlzNavigate is enabled.
BUG=416877
Committed: https://crrev.com/947ebfb68a862ca468835c1a11e2d16b44484120
Cr-Commit-Position: refs/heads/master@{#314542}
Patch Set 1 : #
Total comments: 20
Patch Set 2 : Fixed unittest support. #Patch Set 3 : Just a couple comments #Patch Set 4 : Updated method names and comments. #
Total comments: 12
Patch Set 5 : Changes from latest round of CR comments #
Total comments: 22
Patch Set 6 : Several naming changes, updated comments and more minor stuff. #Patch Set 7 : A couple minor changes #
Total comments: 2
Patch Set 8 : Final method name change. #Messages
Total messages: 28 (4 generated)
Patchset #1 (id:1) has been deleted
carlosk@chromium.org changed reviewers: + clamy@chromium.org, davidben@chromium.org, nasko@chromium.org
davidben@, clamy@, nasko@: PTAL. This adds proper reporting of the Navigation.TimeToURLJobStart* metrics when PlzNavigate is enabled. There's a lot of comments I added to the CL itself that explains the changes. The failing tests are due to me forgetting to adapt TestNavigationURLLoaderDelegate for this changes. I will do that tomorrow and upload another CL. But I already wanted to put this up to get your initial comments. https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_request.cc:111: NOTIMPLEMENTED() << " where net_error=" << net_error; Added this little extra logging to help with debugging. https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator.h:150: virtual void LogAboutToBeginNavigation(base::TimeTicks timestamp) {}; I switched the method name here, adding the Log prefix because for Navigator the logging is the purpose of this call. For the whole loader hierarchy what matters is the more generic notification that the navigation is about to begin, hence it not having the same prefix. https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:777: navigation_data_.reset(); Added a couple metrics-reset-points (here and below) for when PlzNavigate is enabled. This one resets it when a renderer initiated navigation is started so that we don't track them because they are not (yet) properly reporting start times. https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:839: navigation_data_.reset(); Also resetting when cancelling the navigation. Note that non-PlzNavigate navigations don't have resets on cancellations because there's no proper trigger for that in NavigatorImpl. They are reset when a browser side navigation is started and when it commits). This is only a problem when starting a renderer-initiated navigation after a browser-initiated one is cancelled. But even so we do a URL check between the metric data and the navigation so the chances of US getting erroneous reports should be pretty low. WDYT? https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:865: switches::kEnableBrowserSideNavigation)); Now this method is called only for non-PlzNavigate navigations. And the next one is PlzNavigate-only. https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:877: if (navigation_data_) { Note that we don't do URL checks here because they are not actually needed... Or they won't be soon so it didn't seem to make sense to provide the URL here to then eliminate that extra parameter later. With PlzNavigate we should be able to attach the |navigation_data_| to the NavigationRequest so we'd never mix metrics from different requests. I don't think that's the highest priority right now and that's why I'm not already making that change. It's already in our backlog though.
Fixed unit test is up. I added EXPECTS only to the tests where it seemed to make sense to add them (to try and avoid just repeating checks in all tests).
Thanks! A few comments below. https://chromiumcodereview.appspot.com/874353003/diff/20001/content/browser/f... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/874353003/diff/20001/content/browser/f... content/browser/frame_host/navigation_request.h:113: void AboutToBeginNavigation(base::TimeTicks timestamp) override; How about NavigationStarted? This is called after the navigation begun so AboutToBeginNavigation seems a bit weird. https://chromiumcodereview.appspot.com/874353003/diff/20001/content/browser/f... File content/browser/frame_host/navigator.h (right): https://chromiumcodereview.appspot.com/874353003/diff/20001/content/browser/f... content/browser/frame_host/navigator.h:147: // Called when ResourceDispacherHostImpl is about to begin working on a s/is about to begin working/has started working https://chromiumcodereview.appspot.com/874353003/diff/20001/content/browser/f... content/browser/frame_host/navigator.h:150: virtual void LogAboutToBeginNavigation(base::TimeTicks timestamp) {}; On 2015/01/26 19:54:01, carlosk wrote: > I switched the method name here, adding the Log prefix because for Navigator the > logging is the purpose of this call. > > For the whole loader hierarchy what matters is the more generic notification > that the navigation is about to begin, hence it not having the same prefix. Acknowledged. https://chromiumcodereview.appspot.com/874353003/diff/20001/content/browser/f... File content/browser/frame_host/navigator_impl.cc (right): https://chromiumcodereview.appspot.com/874353003/diff/20001/content/browser/f... content/browser/frame_host/navigator_impl.cc:776: // once the renderer correctly provides its request start time. I don't think we ever want to track renderer initiated navigations in the metrics if the current implementation does not report them. We want to keep the same sample as much as possible in order to draw conclusions about the improvements that PlzNavigate brings. https://chromiumcodereview.appspot.com/874353003/diff/20001/content/browser/f... content/browser/frame_host/navigator_impl.cc:865: switches::kEnableBrowserSideNavigation)); On 2015/01/26 19:54:01, carlosk wrote: > Now this method is called only for non-PlzNavigate navigations. And the next one > is PlzNavigate-only. I think they are very similar and we should just have one. https://chromiumcodereview.appspot.com/874353003/diff/20001/content/browser/f... content/browser/frame_host/navigator_impl.cc:877: if (navigation_data_) { On 2015/01/26 19:54:01, carlosk wrote: > Note that we don't do URL checks here because they are not actually needed... Or > they won't be soon so it didn't seem to make sense to provide the URL here to > then eliminate that extra parameter later. > > With PlzNavigate we should be able to attach the |navigation_data_| to the > NavigationRequest so we'd never mix metrics from different requests. > > I don't think that's the highest priority right now and that's why I'm not > already making that change. It's already in our backlog though. Acknowledged.
Thanks! https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_request.h:113: void AboutToBeginNavigation(base::TimeTicks timestamp) override; On 2015/01/27 12:35:03, clamy wrote: > How about NavigationStarted? This is called after the navigation begun so > AboutToBeginNavigation seems a bit weird. Agreed but I don't think NavigationStarted is much better... I based my choice on the method names from ResourceDispatcherHostImpl but something closer to what we actually intend to get from it should be better. So, how about AboutToAttemptFirstNetworkRequest? https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator.h:147: // Called when ResourceDispacherHostImpl is about to begin working on a On 2015/01/27 12:35:04, clamy wrote: > s/is about to begin working/has started working Done. https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:776: // once the renderer correctly provides its request start time. On 2015/01/27 12:35:04, clamy wrote: > I don't think we ever want to track renderer initiated navigations in the > metrics if the current implementation does not report them. We want to keep the > same sample as much as possible in order to draw conclusions about the > improvements that PlzNavigate brings. Agreed and removed the comment. I didn't realize NavigateToEntry is also only called by browser initiated navigations in the current implementation. https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:865: switches::kEnableBrowserSideNavigation)); On 2015/01/27 12:35:04, clamy wrote: > On 2015/01/26 19:54:01, carlosk wrote: > > Now this method is called only for non-PlzNavigate navigations. And the next > one > > is PlzNavigate-only. > > I think they are very similar and we should just have one. I didn't think it would be so bad to have them separate for now as the final result, once PlzNavigate is the-one-navigation, is to have only the next one. If I should unify this now I'd have to create yet another method just to be able to ignore the URL requirement for PlzNavigate calls. WDYT?
Thanks! I think once we fix the naming it should be good. https://chromiumcodereview.appspot.com/874353003/diff/20001/content/browser/f... File content/browser/frame_host/navigation_request.h (right): https://chromiumcodereview.appspot.com/874353003/diff/20001/content/browser/f... content/browser/frame_host/navigation_request.h:113: void AboutToBeginNavigation(base::TimeTicks timestamp) override; On 2015/01/27 17:12:03, carlosk wrote: > On 2015/01/27 12:35:03, clamy wrote: > > How about NavigationStarted? This is called after the navigation begun so > > AboutToBeginNavigation seems a bit weird. > > Agreed but I don't think NavigationStarted is much better... > > I based my choice on the method names from ResourceDispatcherHostImpl but > something closer to what we actually intend to get from it should be better. > > So, how about AboutToAttemptFirstNetworkRequest? I don't like AboutToAttemptFirstNetworkRequest much. First the AboutTo part: this function is called asynchronously, so by the time it executes chances are that we started doing what we were about to do. Second we are sending the request to the network stack, but we may not be doing an actual network request if it is cached. And it's fairly long :). I suggested NavigationStarted because NavigationRequest goes through the following states: - NOT_STARTED - WAITING_FOR_RENDERER_RESPONSE - STARTED - RESPONSE_STARTED. It goes into state STARTED after BeginNavigation has been called (so calling BeginNavigation means the navigation started). So I think that something like NavigationStarted, NavigationBegun or BeginNavigationComplete would be better, since it is about the RDH informing NavigationRequest that it did what was asked of it (BeginNavigation). https://chromiumcodereview.appspot.com/874353003/diff/20001/content/browser/f... File content/browser/frame_host/navigator_impl.cc (right): https://chromiumcodereview.appspot.com/874353003/diff/20001/content/browser/f... content/browser/frame_host/navigator_impl.cc:865: switches::kEnableBrowserSideNavigation)); On 2015/01/27 17:12:03, carlosk wrote: > On 2015/01/27 12:35:04, clamy wrote: > > On 2015/01/26 19:54:01, carlosk wrote: > > > Now this method is called only for non-PlzNavigate navigations. And the next > > one > > > is PlzNavigate-only. > > > > I think they are very similar and we should just have one. > > I didn't think it would be so bad to have them separate for now as the final > result, once PlzNavigate is the-one-navigation, is to have only the next one. > > If I should unify this now I'd have to create yet another method just to be able > to ignore the URL requirement for PlzNavigate calls. > > WDYT? Or you can just call LogResourceRequestTime with the right url and always pass the url check. I would like us to avoid adding yet another method for metrics reporting. When PlzNavigate is enabled, you can just remove the url check, and in fact we may even want to remove the metric entirely at that point (after having confirmed that it is indeed very low).
Thanks. https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigation_request.h:113: void AboutToBeginNavigation(base::TimeTicks timestamp) override; On 2015/01/28 13:44:21, clamy wrote: > On 2015/01/27 17:12:03, carlosk wrote: > > On 2015/01/27 12:35:03, clamy wrote: > > > How about NavigationStarted? This is called after the navigation begun so > > > AboutToBeginNavigation seems a bit weird. > > > > Agreed but I don't think NavigationStarted is much better... > > > > I based my choice on the method names from ResourceDispatcherHostImpl but > > something closer to what we actually intend to get from it should be better. > > > > So, how about AboutToAttemptFirstNetworkRequest? > > I don't like AboutToAttemptFirstNetworkRequest much. First the AboutTo part: > this function is called asynchronously, so by the time it executes chances are > that we started doing what we were about to do. Second we are sending the > request to the network stack, but we may not be doing an actual network request > if it is cached. And it's fairly long :). > > I suggested NavigationStarted because NavigationRequest goes through the > following states: > - NOT_STARTED > - WAITING_FOR_RENDERER_RESPONSE > - STARTED > - RESPONSE_STARTED. > > It goes into state STARTED after BeginNavigation has been called (so calling > BeginNavigation means the navigation started). > > So I think that something like NavigationStarted, NavigationBegun or > BeginNavigationComplete would be better, since it is about the RDH informing > NavigationRequest that it did what was asked of it (BeginNavigation). I agree with your point on the AboutTo not being precise as the navigation is not waiting on this message handling so in fact it might be already under way when the message is handled. But I don't agree it should relate ti the named states of NavigationRequest (NR) as the information it conveys regards solely the NavigationURLLoader. The NR is merely observing through the NavigationURLLoaderDelegate interface and its state change to STARTED is has no relation to this notification. Having this said, BeginNavigationComplete is a good option but I also suggest BeginNavigationRequested or NavigationRequested. I'm preemptively changing to NavigationRequested that I find short and clear but please let me know WDYT. https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:865: switches::kEnableBrowserSideNavigation)); On 2015/01/28 13:44:21, clamy wrote: > On 2015/01/27 17:12:03, carlosk wrote: > > On 2015/01/27 12:35:04, clamy wrote: > > > On 2015/01/26 19:54:01, carlosk wrote: > > > > Now this method is called only for non-PlzNavigate navigations. And the > next > > > one > > > > is PlzNavigate-only. > > > > > > I think they are very similar and we should just have one. > > > > I didn't think it would be so bad to have them separate for now as the final > > result, once PlzNavigate is the-one-navigation, is to have only the next one. > > > > If I should unify this now I'd have to create yet another method just to be > able > > to ignore the URL requirement for PlzNavigate calls. > > > > WDYT? > > Or you can just call LogResourceRequestTime with the right url and always pass > the url check. I would like us to avoid adding yet another method for metrics > reporting. > When PlzNavigate is enabled, you can just remove the url check, and in fact we > may even want to remove the metric entirely at that point (after having > confirmed that it is indeed very low). Done. Also added a TODO to rename it later.
On 2015/01/28 15:21:08, carlosk wrote: > https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... > File content/browser/frame_host/navigation_request.h (right): > > https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... > content/browser/frame_host/navigation_request.h:113: void > AboutToBeginNavigation(base::TimeTicks timestamp) override; > On 2015/01/28 13:44:21, clamy wrote: > > On 2015/01/27 17:12:03, carlosk wrote: > > > On 2015/01/27 12:35:03, clamy wrote: > > > > How about NavigationStarted? This is called after the navigation begun so > > > > AboutToBeginNavigation seems a bit weird. > > > > > > Agreed but I don't think NavigationStarted is much better... > > > > > > I based my choice on the method names from ResourceDispatcherHostImpl but > > > something closer to what we actually intend to get from it should be > better. > > > > > > So, how about AboutToAttemptFirstNetworkRequest? > > > > I don't like AboutToAttemptFirstNetworkRequest much. First the AboutTo part: > > this function is called asynchronously, so by the time it executes chances are > > that we started doing what we were about to do. Second we are sending the > > request to the network stack, but we may not be doing an actual network > request > > if it is cached. And it's fairly long :). > > > > I suggested NavigationStarted because NavigationRequest goes through the > > following states: > > - NOT_STARTED > > - WAITING_FOR_RENDERER_RESPONSE > > - STARTED > > - RESPONSE_STARTED. > > > > It goes into state STARTED after BeginNavigation has been called (so calling > > BeginNavigation means the navigation started). > > > > So I think that something like NavigationStarted, NavigationBegun or > > BeginNavigationComplete would be better, since it is about the RDH informing > > NavigationRequest that it did what was asked of it (BeginNavigation). > > I agree with your point on the AboutTo not being precise as the navigation is > not waiting on this message handling so in fact it might be already under way > when the message is handled. But I don't agree it should relate ti the named > states of NavigationRequest (NR) as the information it conveys regards solely > the NavigationURLLoader. The NR is merely observing through the > NavigationURLLoaderDelegate interface and its state change to STARTED is has no > relation to this notification. > > Having this said, BeginNavigationComplete is a good option but I also suggest > BeginNavigationRequested or NavigationRequested. I'm preemptively changing to > NavigationRequested that I find short and clear but please let me know WDYT. > > https://codereview.chromium.org/874353003/diff/20001/content/browser/frame_ho... > File content/browser/frame_host/navigator_impl.cc (right): Further discussing this with clamy@ she raised the concern that *Requested is too close to *Request and could be also confusing. So my latest suggestion would be around adding "Handled": BeginNavigationHandled, HandledBeginNavigation or NavigationHandled. But I'll wait on the other reviewers' feedback before going for yet another rename. :)
https://codereview.chromium.org/874353003/diff/80001/content/browser/frame_ho... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/874353003/diff/80001/content/browser/frame_ho... content/browser/frame_host/navigation_request.cc:116: common_params_.url); This is just measuring the time to do two thread hops, FWIW. https://codereview.chromium.org/874353003/diff/80001/content/browser/frame_ho... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/874353003/diff/80001/content/browser/frame_ho... content/browser/frame_host/navigator.h:145: // TODO(carlosk): once please navigate is the only navigation implementation Nit: please navigate -> PlzNavigate is probably clearer it's referring to the same thing. https://codereview.chromium.org/874353003/diff/80001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/874353003/diff/80001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:775: navigation_data_.reset(); Should navigation_data_ be a map, or perhaps condition it all on the frame being the main frame? NavigatorImpl is shared across the entire frame tree. (Ditto in the other places this is set.) https://codereview.chromium.org/874353003/diff/80001/content/browser/loader/n... File content/browser/loader/navigation_url_loader_impl_core.cc (right): https://codereview.chromium.org/874353003/diff/80001/content/browser/loader/n... content/browser/loader/navigation_url_loader_impl_core.cc:110: timestamp)); Rather than plumb this all the way to the RDH and back, perhaps just move it up to the start of Start? It would also match OnRequestResource better in cases where the RDH fails the request immediately. https://codereview.chromium.org/874353003/diff/80001/content/browser/loader/n... File content/browser/loader/navigation_url_loader_unittest.cc (right): https://codereview.chromium.org/874353003/diff/80001/content/browser/loader/n... content/browser/loader/navigation_url_loader_unittest.cc:358: EXPECT_EQ(0, delegate.about_to_begin_navigation_counter()); This doesn't quite match the other path with OnRequestResource. Even if the RDH blocks the request, LogResourceRequestTimeOnUI is still recorded.
Thanks! Do any of you find it a problem that we'll be recording metrics for requests that might be failed or canceled by RDH? Also mind that I'd still like input on the naming discussion on comment #9 (https://codereview.chromium.org/874353003/#msg9). https://codereview.chromium.org/874353003/diff/80001/content/browser/frame_ho... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/874353003/diff/80001/content/browser/frame_ho... content/browser/frame_host/navigation_request.cc:116: common_params_.url); On 2015/01/28 21:33:26, David Benjamin wrote: > This is just measuring the time to do two thread hops, FWIW. In fact I think we'll actually be measuring a single thread hop, from the UI thread to the IO thread as the timestamp is obtained before we jump back to the UI. :) But yes, we want to measure the time between the navigation request creation and the first (potential) network request is made, which is what Navigation.TimeToURLFirstJob* histograms are supposed to track. The expectation with PlzNavigate is that this will see a significant speed boost in comparison with the current navigation. I also considered moving the logging point further down into the call chain so that both logs (with or without PlzNavigate) happen at the same trigger point but that would incur too much plumbing of stuff as the way each log case happens is very different. https://codereview.chromium.org/874353003/diff/80001/content/browser/frame_ho... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/874353003/diff/80001/content/browser/frame_ho... content/browser/frame_host/navigator.h:145: // TODO(carlosk): once please navigate is the only navigation implementation On 2015/01/28 21:33:26, David Benjamin wrote: > Nit: please navigate -> PlzNavigate is probably clearer it's referring to the > same thing. Done. LoL! https://codereview.chromium.org/874353003/diff/80001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/874353003/diff/80001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:775: navigation_data_.reset(); On 2015/01/28 21:33:26, David Benjamin wrote: > Should navigation_data_ be a map, or perhaps condition it all on the frame being > the main frame? NavigatorImpl is shared across the entire frame tree. > > (Ditto in the other places this is set.) Indeed we're only interested in main-frame, browser-initiated navigations. So let me go over all points where we set |navigation_data_|. In NavigatorImpl::NavigateToEntry it seems we're doing it correctly as if I understand correctly this method is only called in the correct cases (main-frame, browser-initiated). The call chain to it beings in NavigationControllerImpl::NavigateToPendingEntry that only seems to be triggered by browser actions. Am I right? In NavigatorImpl::OnBeginNavigation I'm only making sure that if a renderer initiated navigation starts, the metrics data is cleared so that nothing will be tracked. The one in NavigatorImpl::CancelNavigation seems dangerous for it seems it might be invoked by subframes (for instance, NavigationRequest::OnResponseStarted may call it). So I'm adding a main-frame guard around it. So yes, by storing this data here we have to be very careful. But mind a previous comment where I mentioned that when PlzNavigate is done we should move it into NavigationRequest and stop worrying about this. https://codereview.chromium.org/874353003/diff/80001/content/browser/loader/n... File content/browser/loader/navigation_url_loader_impl_core.cc (right): https://codereview.chromium.org/874353003/diff/80001/content/browser/loader/n... content/browser/loader/navigation_url_loader_impl_core.cc:110: timestamp)); On 2015/01/28 21:33:27, David Benjamin wrote: > Rather than plumb this all the way to the RDH and back, perhaps just move it up > to the start of Start? It would also match OnRequestResource better in cases > where the RDH fails the request immediately. Done. I measured the time we'd not be accounting for and it's below 1ms. https://codereview.chromium.org/874353003/diff/80001/content/browser/loader/n... File content/browser/loader/navigation_url_loader_unittest.cc (right): https://codereview.chromium.org/874353003/diff/80001/content/browser/loader/n... content/browser/loader/navigation_url_loader_unittest.cc:358: EXPECT_EQ(0, delegate.about_to_begin_navigation_counter()); On 2015/01/28 21:33:27, David Benjamin wrote: > This doesn't quite match the other path with OnRequestResource. Even if the RDH > blocks the request, LogResourceRequestTimeOnUI is still recorded. Done. This behavior changed once I moved the initial call to NavigationURLLoaderImplCore::Start. Also: do you think I should add more or remove some of these EXPECTs? I'm a little lost on which tests should have it... https://codereview.chromium.org/874353003/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:950: navigation_data_->url_job_start_time_.is_null() || I realized that if for some reason NavigatorImpl::LogResourceRequestTime was not called before commit then |navigation_data_->url_job_start_time_| would not be set and we would record invalid values. Just in case, I'm updating NavigatorImpl::RecordNavigationMetrics so that it doesn't try to record anything if it was not set.
Just a few comments. https://codereview.chromium.org/874353003/diff/80001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/874353003/diff/80001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:775: navigation_data_.reset(); On 2015/01/30 14:01:47, carlosk wrote: > On 2015/01/28 21:33:26, David Benjamin wrote: > > Should navigation_data_ be a map, or perhaps condition it all on the frame > being > > the main frame? NavigatorImpl is shared across the entire frame tree. > > > > (Ditto in the other places this is set.) > > Indeed we're only interested in main-frame, browser-initiated navigations. So > let me go over all points where we set |navigation_data_|. > > In NavigatorImpl::NavigateToEntry it seems we're doing it correctly as if I > understand correctly this method is only called in the correct cases > (main-frame, browser-initiated). The call chain to it beings in > NavigationControllerImpl::NavigateToPendingEntry that only seems to be triggered > by browser actions. Am I right? In regular Chrome right now, yes. With Site Isolation enabled, this can also used for subframe navigations. I'm not sure if this CL should worry about it, but just wanted to point it out. > In NavigatorImpl::OnBeginNavigation I'm only making sure that if a renderer > initiated navigation starts, the metrics data is cleared so that nothing will be > tracked. > > The one in NavigatorImpl::CancelNavigation seems dangerous for it seems it might > be invoked by subframes (for instance, NavigationRequest::OnResponseStarted may > call it). So I'm adding a main-frame guard around it. > > So yes, by storing this data here we have to be very careful. But mind a > previous comment where I mentioned that when PlzNavigate is done we should move > it into NavigationRequest and stop worrying about this. https://codereview.chromium.org/874353003/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator.h:143: // PlzNavigate: Called when a navigation is about to make its first potential nit: Why is it "potential"? Don't we always make a request for a navigation? https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... File content/browser/loader/navigation_url_loader_delegate.h (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... content/browser/loader/navigation_url_loader_delegate.h:45: virtual void NavigationRequested(base::TimeTicks timestamp) = 0; nit: OnNavigationRequested? mainly to match the naming pattern of the methods above. https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... File content/browser/loader/navigation_url_loader_impl.h (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... content/browser/loader/navigation_url_loader_impl.h:56: void NavigationRequested(base::TimeTicks timestamp); nit: NotifyNavigationRequested? https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:982: switches::kEnableBrowserSideNavigation) && nit: Checking the resource type is a very simple comparison, which will short circuit the if statement quickly if false. Let's put the more expensive HasSwitch call as the last check.
On 2015/01/30 14:01:47, carlosk wrote: > Thanks! > > Do any of you find it a problem that we'll be recording metrics for requests > that might be failed or canceled by RDH? I think that's fine. It seems no more of a problem than if the net stack rejects the URL because it didn't recognize the URL scheme. This is the metric to measure the latency in getting the request to the net stack, right? (The one that, in the existing codepath, blocks on a process spin-up.) That latency exists in those cases too--it takes us that long to realize the request is illegal. And I wouldn't expect the latency to be inherently different in those cases since we haven't started doing anything based on the URL yet. > Also mind that I'd still like input on the naming discussion on comment #9 > (https://codereview.chromium.org/874353003/#msg9). I would probably name it something to do with network requests rather than navigations. We've already decided to start doing the navigation at this point but are instead beginning the network request, which is a step of navigation. OnRequestStarted maybe? It's a little hard to name it in the context of NavigationURLLoader because it's just measuring a thread hop. :-) It's slightly weird if the RDH decides to block the request out-right, but you can say that the "request", as far as NavigationURLLoader consumers is concerned, starts as soon as you create the object and the RDH blocking it just means that it hit an error really early.
https://codereview.chromium.org/874353003/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:775: navigation_data_.reset(); Does this also need an IsMainFrame guard? https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... File content/browser/loader/navigation_url_loader_delegate.h (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... content/browser/loader/navigation_url_loader_delegate.h:42: // Called when a begin navigation request is handled, right before the "Called when a begin navigation request is handled" seems a little weird. https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... File content/browser/loader/navigation_url_loader_unittest.cc (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... content/browser/loader/navigation_url_loader_unittest.cc:75: return about_to_begin_navigation_counter_; Nit: when we settle on a name, this should change too.
https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... File content/browser/loader/navigation_url_loader_delegate.h (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... content/browser/loader/navigation_url_loader_delegate.h:42: // Called when a begin navigation request is handled, right before the On 2015/02/02 22:33:57, David Benjamin wrote: > "Called when a begin navigation request is handled" seems a little weird. Sorry, never finished this comment. Perhaps: // Called after the network request has begun on the IO thread at time |timestamp|. This is just a thread hop but is to compare timing against the pre-PlzNavigate codepath which didn't start the network request until after the renderer was initialized.
On 2015/02/02 22:30:10, David Benjamin wrote: > On 2015/01/30 14:01:47, carlosk wrote: > > Thanks! > > > > Do any of you find it a problem that we'll be recording metrics for requests > > that might be failed or canceled by RDH? > > I think that's fine. It seems no more of a problem than if the net stack rejects > the URL because it didn't recognize the URL scheme. This is the metric to > measure the latency in getting the request to the net stack, right? (The one > that, in the existing codepath, blocks on a process spin-up.) That latency > exists in those cases too--it takes us that long to realize the request is > illegal. And I wouldn't expect the latency to be inherently different in those > cases since we haven't started doing anything based on the URL yet. > > > Also mind that I'd still like input on the naming discussion on comment #9 > > (https://codereview.chromium.org/874353003/#msg9). > > I would probably name it something to do with network requests rather than > navigations. We've already decided to start doing the navigation at this point > but are instead beginning the network request, which is a step of navigation. > > OnRequestStarted maybe? It's a little hard to name it in the context of > NavigationURLLoader because it's just measuring a thread hop. :-) It's slightly > weird if the RDH decides to block the request out-right, but you can say that > the "request", as far as NavigationURLLoader consumers is concerned, starts as > soon as you create the object and the RDH blocking it just means that it hit an > error really early. I see. To keep the existing naming patterns and to better convey the idea that the request might not be carried out I'm renaming the calls to: [Notify|On]RequestHandled.
Thanks for all comments. https://codereview.chromium.org/874353003/diff/80001/content/browser/frame_ho... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/874353003/diff/80001/content/browser/frame_ho... content/browser/frame_host/navigator_impl.cc:775: navigation_data_.reset(); On 2015/02/02 18:28:53, nasko wrote: > On 2015/01/30 14:01:47, carlosk wrote: > > On 2015/01/28 21:33:26, David Benjamin wrote: > > > Should navigation_data_ be a map, or perhaps condition it all on the frame > > being > > > the main frame? NavigatorImpl is shared across the entire frame tree. > > > > > > (Ditto in the other places this is set.) > > > > Indeed we're only interested in main-frame, browser-initiated navigations. So > > let me go over all points where we set |navigation_data_|. > > > > In NavigatorImpl::NavigateToEntry it seems we're doing it correctly as if I > > understand correctly this method is only called in the correct cases > > (main-frame, browser-initiated). The call chain to it beings in > > NavigationControllerImpl::NavigateToPendingEntry that only seems to be > triggered > > by browser actions. Am I right? > > In regular Chrome right now, yes. With Site Isolation enabled, this can also > used for subframe navigations. I'm not sure if this CL should worry about it, > but just wanted to point it out. > > > In NavigatorImpl::OnBeginNavigation I'm only making sure that if a renderer > > initiated navigation starts, the metrics data is cleared so that nothing will > be > > tracked. > > > > The one in NavigatorImpl::CancelNavigation seems dangerous for it seems it > might > > be invoked by subframes (for instance, NavigationRequest::OnResponseStarted > may > > call it). So I'm adding a main-frame guard around it. > > > > So yes, by storing this data here we have to be very careful. But mind a > > previous comment where I mentioned that when PlzNavigate is done we should > move > > it into NavigationRequest and stop worrying about this. > Ack. The future task to move it into NavigationRequest will sort that one out as well. https://codereview.chromium.org/874353003/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator.h:143: // PlzNavigate: Called when a navigation is about to make its first potential On 2015/02/02 18:28:53, nasko wrote: > nit: Why is it "potential"? Don't we always make a request for a navigation? So I just learned about that as well from clamy@: it is potential because before the network is touched there is a cache check that if successful might completely avoid the network. https://codereview.chromium.org/874353003/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:775: navigation_data_.reset(); On 2015/02/02 22:33:57, David Benjamin wrote: > Does this also need an IsMainFrame guard? No because here we actually want it to be reset. This if-block handles renderer-initiated navigations which we don't want to track. https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... File content/browser/loader/navigation_url_loader_delegate.h (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... content/browser/loader/navigation_url_loader_delegate.h:42: // Called when a begin navigation request is handled, right before the On 2015/02/02 22:37:06, David Benjamin wrote: > On 2015/02/02 22:33:57, David Benjamin wrote: > > "Called when a begin navigation request is handled" seems a little weird. > > Sorry, never finished this comment. > > Perhaps: > // Called after the network request has begun on the IO thread at time > |timestamp|. This is just a thread hop but is to compare timing against the > pre-PlzNavigate codepath which didn't start the network request until after the > renderer was initialized. Done. https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... content/browser/loader/navigation_url_loader_delegate.h:45: virtual void NavigationRequested(base::TimeTicks timestamp) = 0; On 2015/02/02 18:28:53, nasko wrote: > nit: OnNavigationRequested? mainly to match the naming pattern of the methods > above. Done. https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... File content/browser/loader/navigation_url_loader_impl.h (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... content/browser/loader/navigation_url_loader_impl.h:56: void NavigationRequested(base::TimeTicks timestamp); On 2015/02/02 18:28:53, nasko wrote: > nit: NotifyNavigationRequested? This method was removed. https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... File content/browser/loader/navigation_url_loader_unittest.cc (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... content/browser/loader/navigation_url_loader_unittest.cc:75: return about_to_begin_navigation_counter_; On 2015/02/02 22:33:58, David Benjamin wrote: > Nit: when we settle on a name, this should change too. Done. https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:982: switches::kEnableBrowserSideNavigation) && On 2015/02/02 18:28:53, nasko wrote: > nit: Checking the resource type is a very simple comparison, which will short > circuit the if statement quickly if false. Let's put the more expensive > HasSwitch call as the last check. Done. I wasn't aware it was expensive.
Thanks! It looks good, just a few more comments. https://chromiumcodereview.appspot.com/874353003/diff/100001/content/browser/... File content/browser/frame_host/navigator.h (right): https://chromiumcodereview.appspot.com/874353003/diff/100001/content/browser/... content/browser/frame_host/navigator.h:143: // PlzNavigate: Called when a navigation is about to make its first potential On 2015/02/03 11:00:04, carlosk wrote: > On 2015/02/02 18:28:53, nasko wrote: > > nit: Why is it "potential"? Don't we always make a request for a navigation? > > So I just learned about that as well from clamy@: it is potential because before > the network is touched there is a cache check that if successful might > completely avoid the network. Maybe we can rephrase that as Called when the network stack started handling the navigation request (since this is what we care about, and not whether we end serving the request from cache or the network). https://chromiumcodereview.appspot.com/874353003/diff/100001/content/browser/... File content/browser/frame_host/navigator_impl.cc (right): https://chromiumcodereview.appspot.com/874353003/diff/100001/content/browser/... content/browser/frame_host/navigator_impl.cc:775: navigation_data_.reset(); On 2015/02/03 11:00:04, carlosk wrote: > On 2015/02/02 22:33:57, David Benjamin wrote: > > Does this also need an IsMainFrame guard? > > No because here we actually want it to be reset. This if-block handles > renderer-initiated navigations which we don't want to track. I think we should have one for the case where the user starts a browser intiated navigation in the main frame and a renderer initiated navigation starts in a subframe. We don't want to discard the data coming from the main frame navigation that will eventually commit.
Thanks once more. https://codereview.chromium.org/874353003/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator.h (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator.h:143: // PlzNavigate: Called when a navigation is about to make its first potential On 2015/02/03 12:13:54, clamy wrote: > On 2015/02/03 11:00:04, carlosk wrote: > > On 2015/02/02 18:28:53, nasko wrote: > > > nit: Why is it "potential"? Don't we always make a request for a navigation? > > > > So I just learned about that as well from clamy@: it is potential because > before > > the network is touched there is a cache check that if successful might > > completely avoid the network. > > Maybe we can rephrase that as Called when the network stack started handling the > navigation request (since this is what we care about, and not whether we end > serving the request from cache or the network). Done. https://codereview.chromium.org/874353003/diff/100001/content/browser/frame_h... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/frame_h... content/browser/frame_host/navigator_impl.cc:775: navigation_data_.reset(); On 2015/02/03 12:13:54, clamy wrote: > On 2015/02/03 11:00:04, carlosk wrote: > > On 2015/02/02 22:33:57, David Benjamin wrote: > > > Does this also need an IsMainFrame guard? > > > > No because here we actually want it to be reset. This if-block handles > > renderer-initiated navigations which we don't want to track. > > I think we should have one for the case where the user starts a browser intiated > navigation in the main frame and a renderer initiated navigation starts in a > subframe. We don't want to discard the data coming from the main frame > navigation that will eventually commit. You are both right indeed, sorry. Done.
LGTM, though please wait for David's one for the loader code. https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:982: switches::kEnableBrowserSideNavigation) && On 2015/02/03 11:00:04, carlosk wrote: > On 2015/02/02 18:28:53, nasko wrote: > > nit: Checking the resource type is a very simple comparison, which will short > > circuit the if statement quickly if false. Let's put the more expensive > > HasSwitch call as the last check. > > Done. I wasn't aware it was expensive. It is "expensive" relatively speaking. It is a lookup in some structure of all flags. It will always be more expensive than a straight comparison of POD values.
lgtm https://codereview.chromium.org/874353003/diff/140001/content/browser/loader/... File content/browser/loader/navigation_url_loader_impl.h (right): https://codereview.chromium.org/874353003/diff/140001/content/browser/loader/... content/browser/loader/navigation_url_loader_impl.h:56: void NotifyRequestHandled(base::TimeTicks timestamp); Nit: I would prefer "OnRequestStart" or "OnRequestStarted" or maybe "OnRequestReady" rather than "OnRequestHandled". "Handled" to me says that the network request is done. ("I have handled the request and can move on to something else.) But this name has changed plenty enough so I'm not going to re-open the debate if everyone else is happy with the name. :-)
On 2015/02/03 23:54:40, David Benjamin wrote: > lgtm > > https://codereview.chromium.org/874353003/diff/140001/content/browser/loader/... > File content/browser/loader/navigation_url_loader_impl.h (right): > > https://codereview.chromium.org/874353003/diff/140001/content/browser/loader/... > content/browser/loader/navigation_url_loader_impl.h:56: void > NotifyRequestHandled(base::TimeTicks timestamp); > Nit: I would prefer "OnRequestStart" or "OnRequestStarted" or maybe > "OnRequestReady" rather than "OnRequestHandled". "Handled" to me says that the > network request is done. ("I have handled the request and can move on to > something else.) > > But this name has changed plenty enough so I'm not going to re-open the debate > if everyone else is happy with the name. :-) I would prefer OnRequestStarted. Handled implies different point in the lifetime of a request.
New patchsets have been uploaded after l-g-t-m from nasko@chromium.org,davidben@chromium.org
Thanks. https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/874353003/diff/100001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:982: switches::kEnableBrowserSideNavigation) && On 2015/02/03 23:51:56, nasko wrote: > On 2015/02/03 11:00:04, carlosk wrote: > > On 2015/02/02 18:28:53, nasko wrote: > > > nit: Checking the resource type is a very simple comparison, which will > short > > > circuit the if statement quickly if false. Let's put the more expensive > > > HasSwitch call as the last check. > > > > Done. I wasn't aware it was expensive. > > It is "expensive" relatively speaking. It is a lookup in some structure of all > flags. It will always be more expensive than a straight comparison of POD > values. Acknowledged. https://codereview.chromium.org/874353003/diff/140001/content/browser/loader/... File content/browser/loader/navigation_url_loader_impl.h (right): https://codereview.chromium.org/874353003/diff/140001/content/browser/loader/... content/browser/loader/navigation_url_loader_impl.h:56: void NotifyRequestHandled(base::TimeTicks timestamp); On 2015/02/03 23:54:40, David Benjamin wrote: > Nit: I would prefer "OnRequestStart" or "OnRequestStarted" or maybe > "OnRequestReady" rather than "OnRequestHandled". "Handled" to me says that the > network request is done. ("I have handled the request and can move on to > something else.) > > But this name has changed plenty enough so I'm not going to re-open the debate > if everyone else is happy with the name. :-) Given this and nasko@'s reply I'll change the names to [On|Notify]RequestStarted depending on the prefix already in use in changed class.
The CQ bit was checked by carlosk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/874353003/160001
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/947ebfb68a862ca468835c1a11e2d16b44484120 Cr-Commit-Position: refs/heads/master@{#314542} |
