Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1919193003:
Add FromGWS variants to the AbortTiming metrics (Closed)
|
Created:
4 years, 7 months ago by Charlie Harrison Modified:
4 years, 7 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd FromGWS variants to the AbortTiming metrics
This patch was split off from the larger CL authored here:
https://codereview.chromium.org/1901303004/
BUG=605259
Committed: https://crrev.com/6bd4ae311136de4f95b19b8f1a3e6d4d8430b560
Cr-Commit-Position: refs/heads/master@{#389903}
Patch Set 1 #
Total comments: 22
Patch Set 2 : bmcquade@ review, added some tests #
Total comments: 2
Patch Set 3 : Disallow navs from GoogleRedirector #
Messages
Total messages: 17 (7 generated)
Description was changed from ========== Add FromGWS variants to the AbortTiming metrics BUG=605259 ========== to ========== Add FromGWS variants to the AbortTiming metrics This patch was split off from the larger CL authored here: https://codereview.chromium.org/1901303004/ BUG=605259 ==========
csharrison@chromium.org changed reviewers: + bmcquade@chromium.org
Here's the first patch: just FromGWS abort metrics.
Thanks! A few more suggestions. https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:53: void LogCommittedAborts(UserAbortType abort_type, just to be explicit can we call this LogCommittedAbortsBeforePaint https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:71: "PageLoad.Clients.FromGWS2.AbortTiming.Reload.AfterCommit." let's declare all the names in the same place - even if the constants aren't in the header let's have a kHistogram for this and the one below. that way if for example we version to FromGWS3 they'll all be together and we'll be less likely to miss them. https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:82: break; can we dcheck that we don't get UNKNOWN_NAVIGATION here? are there other cases we are explicitly ignoring here? if so let's leave a comment to note which are being ignored and why. https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:108: break; same - let's leave a short comment to note which are being ignored and why. https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:314: previously_committed_url_from_gws_ = IsUrlFromGWS(url); given that half of IsUrlFromGWS calls IsGoogleSearchRedirectorUrl and we also store the result of calling that method explicitly below, it feels slightly redundant that it's also included here. given that we store the result of IsGoogleSearchRedirectorUrl explicitly let's have a separate boolean for the other half (IsGoogleSearchResultUrl). Let's also shy away from using GWS in our names since it's somewhat ambiguous what that means (do we just mean search results page? do we mean search results and redirector? what about the search homepage?) Also perhaps it's overly subtle (my fault) but there is a difference between IsGoogleRedirectorUrl and IsGoogleSearchRedirectorUrl so we should be explicit about which one we mean. so maybe previously_committed_url_is_search_results_ = IsGoogleSearchResultUrl(url); previously_committed_url_is_search_redirector_ = IsGoogleSearchRedirectorUrl(url); https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:319: provisional_url_from_gws_ = IsUrlFromGWS(url); in the interest of being clearer here let's call this provisional_url_is_search_results_or_google_redirector_ and i'm inclined to just do IsGoogleSearchResultUrl(url) || IsGoogleRedirectorUrl(url) inline. https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:360: bool aborted_before_paint = can we move this logic inside of the test for if (!extra_info.committed_url.is_empty()) { since it's only needed there? https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:379: if (IsUrlFromGWS(committed_url) || note that IsUrlFromGWS is subtly different from the previous logic, which tests IsGoogleRedirectorUrl which is slightly more general than IsGoogleSearchRedirectorUrl. We don't want to log navigations for either case so it's worth using the more broad test here (IsGoogleRedirectorUrl). Since IsUrlFromGWS is relatively simple, I think it's clearer to just inline the logic that we want at each call site. https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:380: (committed_url.is_empty() && provisional_url_from_gws_)) { small logic flow suggestion change here: // Don't log stats if the URL being navigated to (committed if available, else provisional) // is a known google redirector URL or Google search results URL. if (committed_url.is_empty()) { if (provisional_url_is_search_results_or_google_redirector_) { return false; } } else { if (IsGoogleSearchResultUrl(committed_url) || IsGoogleRedirectorUrl(committed_url)) { return false; } } https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:389: if (!previously_committed_url_from_gws_) let's invert the negations to make the logic easier to follow. how about: if (previously_committed_url_is_search_results_ && navigation_initiated_via_link_) { return true; } // If we navigated via the search redirector, then the information about whether // the navigation was from a link would have been associated with the navigation // to the redirector, and not included in the redirected navigation, so we can't // require the navigation to have been initiated via a link in this case. if (previously_committed_url_is_search_redirector_) { return true; } return false; https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:391: if (!committed_url.is_empty() && !navigation_initiated_via_link_ && is the requirement that the committed url is not empty here still part of this function? seems like that policy has moved to the actual logging function, which may want to log aborts before commit.
https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:53: void LogCommittedAborts(UserAbortType abort_type, On 2016/04/26 19:28:46, Bryan McQuade wrote: > just to be explicit can we call this LogCommittedAbortsBeforePaint Done. https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:71: "PageLoad.Clients.FromGWS2.AbortTiming.Reload.AfterCommit." On 2016/04/26 19:28:46, Bryan McQuade wrote: > let's declare all the names in the same place - even if the constants aren't in > the header let's have a kHistogram for this and the one below. that way if for > example we version to FromGWS3 they'll all be together and we'll be less likely > to miss them. Done. https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:82: break; On 2016/04/26 19:28:46, Bryan McQuade wrote: > can we dcheck that we don't get UNKNOWN_NAVIGATION here? are there other cases > we are explicitly ignoring here? if so let's leave a comment to note which are > being ignored and why. Added the unknown and other case here. https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:108: break; On 2016/04/26 19:28:46, Bryan McQuade wrote: > same - let's leave a short comment to note which are being ignored and why. No DCHECKs here (they can still trigger rarely). I've added a comment though. https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:314: previously_committed_url_from_gws_ = IsUrlFromGWS(url); On 2016/04/26 19:28:46, Bryan McQuade wrote: > given that half of IsUrlFromGWS calls IsGoogleSearchRedirectorUrl and we also > store the result of calling that method explicitly below, it feels slightly > redundant that it's also included here. > > given that we store the result of IsGoogleSearchRedirectorUrl explicitly let's > have a separate boolean for the other half (IsGoogleSearchResultUrl). > > Let's also shy away from using GWS in our names since it's somewhat ambiguous > what that means (do we just mean search results page? do we mean search results > and redirector? what about the search homepage?) > > Also perhaps it's overly subtle (my fault) but there is a difference between > IsGoogleRedirectorUrl and IsGoogleSearchRedirectorUrl so we should be explicit > about which one we mean. > > so maybe > previously_committed_url_is_search_results_ = IsGoogleSearchResultUrl(url); > previously_committed_url_is_search_redirector_ = > IsGoogleSearchRedirectorUrl(url); Done. https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:319: provisional_url_from_gws_ = IsUrlFromGWS(url); On 2016/04/26 19:28:46, Bryan McQuade wrote: > in the interest of being clearer here let's call this > provisional_url_is_search_results_or_google_redirector_ > > and i'm inclined to just do IsGoogleSearchResultUrl(url) || > IsGoogleRedirectorUrl(url) inline. Done. https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:360: bool aborted_before_paint = On 2016/04/26 19:28:46, Bryan McQuade wrote: > can we move this logic inside of the test for if > (!extra_info.committed_url.is_empty()) { since it's only needed there? Done. https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:379: if (IsUrlFromGWS(committed_url) || On 2016/04/26 19:28:46, Bryan McQuade wrote: > note that IsUrlFromGWS is subtly different from the previous logic, which tests > IsGoogleRedirectorUrl which is slightly more general than > IsGoogleSearchRedirectorUrl. We don't want to log navigations for either case so > it's worth using the more broad test here (IsGoogleRedirectorUrl). Since > IsUrlFromGWS is relatively simple, I think it's clearer to just inline the logic > that we want at each call site. Done. https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:380: (committed_url.is_empty() && provisional_url_from_gws_)) { On 2016/04/26 19:28:46, Bryan McQuade wrote: > small logic flow suggestion change here: > > // Don't log stats if the URL being navigated to (committed if available, else > provisional) > // is a known google redirector URL or Google search results URL. > if (committed_url.is_empty()) { > if (provisional_url_is_search_results_or_google_redirector_) { > return false; > } > } else { > if (IsGoogleSearchResultUrl(committed_url) || > IsGoogleRedirectorUrl(committed_url)) { > return false; > } > } Done. https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:389: if (!previously_committed_url_from_gws_) On 2016/04/26 19:28:46, Bryan McQuade wrote: > let's invert the negations to make the logic easier to follow. how about: > > if (previously_committed_url_is_search_results_ && > navigation_initiated_via_link_) { > return true; > } > // If we navigated via the search redirector, then the information about whether > // the navigation was from a link would have been associated with the navigation > // to the redirector, and not included in the redirected navigation, so we can't > // require the navigation to have been initiated via a link in this case. > if (previously_committed_url_is_search_redirector_) { > return true; > } > return false; Done. https://codereview.chromium.org/1919193003/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:391: if (!committed_url.is_empty() && !navigation_initiated_via_link_ && On 2016/04/26 19:28:46, Bryan McQuade wrote: > is the requirement that the committed url is not empty here still part of this > function? seems like that policy has moved to the actual logging function, which > may want to log aborts before commit. This follows from the fact that link navigations are only captured for committed loads.
LGTM, thanks! Just one small change request. Thanks also for the great unit tests! https://codereview.chromium.org/1919193003/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1919193003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:332: IsGoogleSearchResultUrl(url) || IsGoogleSearchRedirectorUrl(url); i think this wants to be || IsGoogleRedirectorUrl(url); (drop the search part)
csharrison@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine@, PTAL thanks! https://codereview.chromium.org/1919193003/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc (right): https://codereview.chromium.org/1919193003/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/from_gws_page_load_metrics_observer.cc:332: IsGoogleSearchResultUrl(url) || IsGoogleSearchRedirectorUrl(url); On 2016/04/26 20:46:30, Bryan McQuade wrote: > i think this wants to be || IsGoogleRedirectorUrl(url); (drop the search part) Done.
lgtm
Thanks, everyone!
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org Link to the patchset: https://codereview.chromium.org/1919193003/#ps40001 (title: "Disallow navs from GoogleRedirector")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1919193003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1919193003/40001
Message was sent while issue was closed.
Description was changed from ========== Add FromGWS variants to the AbortTiming metrics This patch was split off from the larger CL authored here: https://codereview.chromium.org/1901303004/ BUG=605259 ========== to ========== Add FromGWS variants to the AbortTiming metrics This patch was split off from the larger CL authored here: https://codereview.chromium.org/1901303004/ BUG=605259 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add FromGWS variants to the AbortTiming metrics This patch was split off from the larger CL authored here: https://codereview.chromium.org/1901303004/ BUG=605259 ========== to ========== Add FromGWS variants to the AbortTiming metrics This patch was split off from the larger CL authored here: https://codereview.chromium.org/1901303004/ BUG=605259 Committed: https://crrev.com/6bd4ae311136de4f95b19b8f1a3e6d4d8430b560 Cr-Commit-Position: refs/heads/master@{#389903} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6bd4ae311136de4f95b19b8f1a3e6d4d8430b560 Cr-Commit-Position: refs/heads/master@{#389903} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
