Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
150033002:
NTP: add histogram to track New Tab page URL status. (Closed)
|
Created:
6 years, 10 months ago by samarth Modified:
6 years, 10 months ago CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, jar (doing other things), dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, dominich, jfweitz+watch_chromium.org, David Black, asvitkine+watch_chromium.org, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Ilya Sherman Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionNTP: add histogram to track New Tab page URL status.
Log status of the New Tab page URL (e.g. valid URL from search provider,
insecure url, etc.) whenever a New Tab is opened.
BUG=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248573
Patch Set 1 #
Total comments: 6
Patch Set 2 : Address comments. #
Total comments: 4
Patch Set 3 : Address comments. #Patch Set 4 : Fix doc. #
Total comments: 4
Patch Set 5 : Address comments. #
Total comments: 2
Patch Set 6 : static helper #Patch Set 7 : Clean things up. #
Total comments: 9
Patch Set 8 : Fix return value + nits. #Patch Set 9 : Typo. #Patch Set 10 : Rebase. #
Messages
Total messages: 20 (0 generated)
PTAL, thanks! Samarth
https://codereview.chromium.org/150033002/diff/1/chrome/browser/search/search.cc File chrome/browser/search/search.cc (right): https://codereview.chromium.org/150033002/diff/1/chrome/browser/search/search... chrome/browser/search/search.cc:91: // provider's URL. Mention that this is used in an UMA macro, so new values should be added at the end and existing values shouldn't be re-ordered or removed. https://codereview.chromium.org/150033002/diff/1/chrome/browser/search/search... chrome/browser/search/search.cc:96: LOCAL_NTP_FALLBACK_MAX = 4 Maybe better to remove the = 4 here, so that it automatically is set to the last value + 1 (so that new values being added don't need to update it). https://codereview.chromium.org/150033002/diff/1/chrome/browser/search/search... chrome/browser/search/search.cc:477: UMA_HISTOGRAM_ENUMERATION("InstantExtended.LocalNTPFallbackReason", Can you make a small helper function in the anon namespace that wraps this macro and takes just the enum value? That way, you don't need to repeat the string and it's more efficient from code size perspective (the macro expands to a blob of code, so having it in a single place avoids the duplication.)
https://codereview.chromium.org/150033002/diff/1/chrome/browser/search/search.cc File chrome/browser/search/search.cc (right): https://codereview.chromium.org/150033002/diff/1/chrome/browser/search/search... chrome/browser/search/search.cc:91: // provider's URL. On 2014/01/30 15:19:24, Alexei Svitkine wrote: > Mention that this is used in an UMA macro, so new values should be added at the > end and existing values shouldn't be re-ordered or removed. Done. https://codereview.chromium.org/150033002/diff/1/chrome/browser/search/search... chrome/browser/search/search.cc:96: LOCAL_NTP_FALLBACK_MAX = 4 On 2014/01/30 15:19:24, Alexei Svitkine wrote: > Maybe better to remove the = 4 here, so that it automatically is set to the last > value + 1 (so that new values being added don't need to update it). Done. https://codereview.chromium.org/150033002/diff/1/chrome/browser/search/search... chrome/browser/search/search.cc:477: UMA_HISTOGRAM_ENUMERATION("InstantExtended.LocalNTPFallbackReason", On 2014/01/30 15:19:24, Alexei Svitkine wrote: > Can you make a small helper function in the anon namespace that wraps this macro > and takes just the enum value? That way, you don't need to repeat the string and > it's more efficient from code size perspective (the macro expands to a blob of > code, so having it in a single place avoids the duplication.) Done.
https://codereview.chromium.org/150033002/diff/70001/chrome/browser/search/se... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/150033002/diff/70001/chrome/browser/search/se... chrome/browser/search/search.cc:477: return GURL(chrome::kChromeSearchLocalNtpUrl); How about adding another bucket, BAD_URL, to measure these other two cases? I'm curious how much profile corruption is really a problem. https://codereview.chromium.org/150033002/diff/70001/chrome/browser/search/se... chrome/browser/search/search.cc:484: if (!url.SchemeIsSecure()) { If you add a bucket NONE, then we can normalize as a fraction of total calls to this function, and you can write LocalNTPFallbackReason fallback = NONE; if (cond1) fallback = R1; else if (cond2) fallback = R2; ... record(fallback); if (fallback != NONE) return url; return GURL(localntp);
PTAL. Thanks, Samarth https://codereview.chromium.org/150033002/diff/70001/chrome/browser/search/se... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/150033002/diff/70001/chrome/browser/search/se... chrome/browser/search/search.cc:477: return GURL(chrome::kChromeSearchLocalNtpUrl); On 2014/01/30 17:13:12, Jered wrote: > How about adding another bucket, BAD_URL, to measure these other two cases? I'm > curious how much profile corruption is really a problem. Done. https://codereview.chromium.org/150033002/diff/70001/chrome/browser/search/se... chrome/browser/search/search.cc:484: if (!url.SchemeIsSecure()) { On 2014/01/30 17:13:12, Jered wrote: > If you add a bucket NONE, then we can normalize as a fraction of total calls to > this function, and you can write > LocalNTPFallbackReason fallback = NONE; > if (cond1) > fallback = R1; > else if (cond2) > fallback = R2; > ... > record(fallback); > if (fallback != NONE) > return url; > return GURL(localntp); I ended up redoing this code to make it simpler and clearer. It also changes it so that we only log when the rewrite is happening and not whenever GetNewTabPageURL is called.
https://codereview.chromium.org/150033002/diff/120001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/150033002/diff/120001/chrome/browser/search/s... chrome/browser/search/search.cc:290: NewTabPageURLState GetNewTabPageURLState(Profile* profile, GURL* new_tab_url) { See comment below. How about adding struct NewTabURL { NewTabURL(const GURL& url, int state); }; then changing this to NewTabURL GetNewTabPageURL(Profile* profile) { if (!profile) return NewTabURL(NEW_TAB_URL_BAD, GURL()); if (profile->IsOffTheRecord()) return NewTabURL(NEW_TAB_URL_INCOGNITO, GURL()); ... } Caller just says NewTabURL new_tab_url(GetNewTabPageURL(profile)); UMA_HISTOGRAM_ENUMERATION("NewTab.URLState", new_tab_url.reason, NEW_TAB_URL_MAX); *url = new_tab_url.url; return url->is_valid(); // or something https://codereview.chromium.org/150033002/diff/120001/chrome/browser/search/s... chrome/browser/search/search.cc:509: GURL GetNewTabPageURL(Profile* profile) { It'd be nice to keep the property that this function always returns the correct URL. That way we don't have to worry about other callers not duplicating the extra logic in HandleRewrite.
https://codereview.chromium.org/150033002/diff/120001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/150033002/diff/120001/chrome/browser/search/s... chrome/browser/search/search.cc:290: NewTabPageURLState GetNewTabPageURLState(Profile* profile, GURL* new_tab_url) { On 2014/01/31 21:56:23, Jered wrote: > See comment below. How about adding > struct NewTabURL { > NewTabURL(const GURL& url, int state); > }; > then changing this to > NewTabURL GetNewTabPageURL(Profile* profile) { > if (!profile) > return NewTabURL(NEW_TAB_URL_BAD, GURL()); > if (profile->IsOffTheRecord()) > return NewTabURL(NEW_TAB_URL_INCOGNITO, GURL()); > ... > } > Caller just says > NewTabURL new_tab_url(GetNewTabPageURL(profile)); > UMA_HISTOGRAM_ENUMERATION("NewTab.URLState", new_tab_url.reason, > NEW_TAB_URL_MAX); > *url = new_tab_url.url; > return url->is_valid(); // or something OK, I did something like this. WDYT? https://codereview.chromium.org/150033002/diff/120001/chrome/browser/search/s... chrome/browser/search/search.cc:509: GURL GetNewTabPageURL(Profile* profile) { On 2014/01/31 21:56:23, Jered wrote: > It'd be nice to keep the property that this function always returns the correct > URL. That way we don't have to worry about other callers not duplicating the > extra logic in HandleRewrite. Done.
lgtm https://codereview.chromium.org/150033002/diff/170001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/150033002/diff/170001/chrome/browser/search/s... chrome/browser/search/search.cc:292: : url(url), state(state) {} nit: This could have an interface like static NewTabURLDetails ForProfile(Profile *profile) { } then callers could say NewTabURLDetails::ForProfile(profile) Might eliminate the need to pass by return ptr and collect some of the global functions. No strong preference.
https://codereview.chromium.org/150033002/diff/170001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/150033002/diff/170001/chrome/browser/search/s... chrome/browser/search/search.cc:292: : url(url), state(state) {} On 2014/02/01 00:01:40, Jered wrote: > nit: This could have an interface like > static NewTabURLDetails ForProfile(Profile *profile) { > } > then callers could say > NewTabURLDetails::ForProfile(profile) > > Might eliminate the need to pass by return ptr and collect some of the global > functions. No strong preference. Did something like this and also tried to clean things up some more. PTAL.
On 2014/02/01 01:35:59, samarth wrote: > https://codereview.chromium.org/150033002/diff/170001/chrome/browser/search/s... > File chrome/browser/search/search.cc (right): > > https://codereview.chromium.org/150033002/diff/170001/chrome/browser/search/s... > chrome/browser/search/search.cc:292: : url(url), state(state) {} > On 2014/02/01 00:01:40, Jered wrote: > > nit: This could have an interface like > > static NewTabURLDetails ForProfile(Profile *profile) { > > } > > then callers could say > > NewTabURLDetails::ForProfile(profile) > > > > Might eliminate the need to pass by return ptr and collect some of the global > > functions. No strong preference. > > Did something like this and also tried to clean things up some more. PTAL. slgtm Thanks for the cleanup
https://codereview.chromium.org/150033002/diff/240001/chrome/browser/search/s... File chrome/browser/search/search_unittest.cc (right): https://codereview.chromium.org/150033002/diff/240001/chrome/browser/search/s... chrome/browser/search/search_unittest.cc:533: EXPECT_FALSE(HandleNewTabURLRewrite(&new_tab_url, AsBrowserContext( nit: Just profile() should work.
https://codereview.chromium.org/150033002/diff/240001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/150033002/diff/240001/chrome/browser/search/s... chrome/browser/search/search.cc:306: struct NewTabURLDetails { Could probably use a comment briefly outlining what this is for. https://codereview.chromium.org/150033002/diff/240001/chrome/browser/search/s... chrome/browser/search/search.cc:328: return NewTabURLDetails(local_url, NEW_TAB_URL_BAD); Wouldn't this result in the histogram only ever logging one of: NEW_TAB_URL_VALID, NEW_TAB_URL_INCOGNITO or NEW_TAB_URL_BAD with the other enum values always mapping to NEW_TAB_URL_BAD? https://codereview.chromium.org/150033002/diff/240001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/150033002/diff/240001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28300: + <int value="0" label="Valid URL that was used"/> Nit: "Valid URL was used"
https://codereview.chromium.org/150033002/diff/240001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/150033002/diff/240001/chrome/browser/search/s... chrome/browser/search/search.cc:328: return NewTabURLDetails(local_url, NEW_TAB_URL_BAD); On 2014/02/03 18:08:38, Alexei Svitkine wrote: > Wouldn't this result in the histogram only ever logging one of: > NEW_TAB_URL_VALID, NEW_TAB_URL_INCOGNITO or NEW_TAB_URL_BAD with the other > enum values always mapping to NEW_TAB_URL_BAD? Oops, good catch. Should this be state?
https://codereview.chromium.org/150033002/diff/240001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/150033002/diff/240001/chrome/browser/search/s... chrome/browser/search/search.cc:306: struct NewTabURLDetails { On 2014/02/03 18:08:38, Alexei Svitkine wrote: > Could probably use a comment briefly outlining what this is for. Done. https://codereview.chromium.org/150033002/diff/240001/chrome/browser/search/s... chrome/browser/search/search.cc:328: return NewTabURLDetails(local_url, NEW_TAB_URL_BAD); On 2014/02/03 18:18:11, jered1 wrote: > On 2014/02/03 18:08:38, Alexei Svitkine wrote: > > Wouldn't this result in the histogram only ever logging one of: > > NEW_TAB_URL_VALID, NEW_TAB_URL_INCOGNITO or NEW_TAB_URL_BAD with the other > > enum values always mapping to NEW_TAB_URL_BAD? > > Oops, good catch. Should this be state? Yep, thanks for catching that! https://codereview.chromium.org/150033002/diff/240001/chrome/browser/search/s... File chrome/browser/search/search_unittest.cc (right): https://codereview.chromium.org/150033002/diff/240001/chrome/browser/search/s... chrome/browser/search/search_unittest.cc:533: EXPECT_FALSE(HandleNewTabURLRewrite(&new_tab_url, AsBrowserContext( On 2014/02/03 15:58:11, Jered wrote: > nit: Just profile() should work. Done. https://codereview.chromium.org/150033002/diff/240001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/150033002/diff/240001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:28300: + <int value="0" label="Valid URL that was used"/> On 2014/02/03 18:08:38, Alexei Svitkine wrote: > Nit: "Valid URL was used" Done.
lgtm
The CQ bit was checked by samarth@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/samarth@chromium.org/150033002/370001
Message was sent while issue was closed.
Change committed as 248573
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring.
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
