Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
342053002:
Add UMA metrics for Android Chrome Google Search. (Closed)
|
Created:
6 years, 6 months ago by kmadhusu Modified:
6 years, 6 months ago CC:
chromium-reviews, tburkard+watch_chromium.org, gavinp+prer_chromium.org, cbentzel+watch_chromium.org, dominich+watch_chromium.org, davidben+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd UMA metrics for Android Chrome Google Search.
To analyze the impact of prefetching high-confidence search suggestions in Android Chrome, this CL,
- Adds a listener for counting Google searches from various search access points. No actual search query content is observed.
- Records the search count based on the prerendering settings.
(see trybot results in patchset #8).
BUG=382694
R=asvitkine@chromium.org, davidben@chromium.org, jam@chromium.org, pkasting@chromium.org, samarth@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278835
Patch Set 1 : '' #
Total comments: 27
Patch Set 2 : Addressed comments #Patch Set 3 : Removed dupe check and updated omnibox search accesspoint check. #
Total comments: 8
Patch Set 4 : Addressed nits #Patch Set 5 : Rebase #Patch Set 6 : Updated histogram name #
Total comments: 4
Patch Set 7 : Rebase + Updated histogram.xml #
Total comments: 4
Patch Set 8 : '' #Patch Set 9 : Fixed GoogleSearchCounterTest #Patch Set 10 : Rebase #Messages
Total messages: 34 (0 generated)
pkasting@: Please review. I am still working on unit tests. I would like to get the review process started. After I get your initial comments, I will add other OWNERS to the reviewer list. samarth@: FYI. Thanks.
https://codereview.chromium.org/342053002/diff/80001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_main_android.cc (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/chrome_br... chrome/browser/chrome_browser_main_android.cc:55: void ChromeBrowserMainPartsAndroid::PostProfileInit() { Check with jam@ that this is the right way to do this. https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... File chrome/browser/google/google_search_counter.cc (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_search_counter.cc:36: // receive two notifications events for the same page. Nit: notifications events -> notifications/notification events (pick one) What does this comment mean, though? Why are we getting two notifications, and what types are they? This seems strange and wrong. https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... File chrome/browser/google/google_search_counter_android.cc (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_search_counter_android.cc:52: switch (type) { Just DCHECK_EQ(content::NOTIFICATION_NAV_ENTRY_COMMITTED, type); and then handle it. Kill the switch entirely. https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... File chrome/browser/google/google_search_counter_android.h (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_search_counter_android.h:26: // content::NotificationObserver Nit: Add trailing colon https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... File chrome/browser/google/google_util.cc (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_util.cc:78: bool IsOmniboxGoogleSearchNavigation(const content::NavigationEntry& entry) { Given that these two functions are extremely short and only called once, I think it would be better to inline their contents below. This would avoid the need to check or comment about the status of IsGoogleSearchURL(). https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_util.cc:82: return stripped_transition == content::PAGE_TRANSITION_GENERATED; Don't you also need to check for PAGE_TRANSITION_FROM_ADDRESS_BAR? GENERATED alone could be from different sources. https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_util.cc:91: std::string::npos; Nit: Indent 4, not even https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_util.cc:242: // an Omnibox search. This comment goes into detail that isn't visible here. Either eliminate it or change it to only refer to externally-visible aspects of IsOmniboxGoogleSearchNavigation(). https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... File chrome/browser/google/google_util.h (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_util.h:117: GoogleSearchMetrics::AccessPoint GetGoogleSearchAccessPointForNavEntry( Nit: Comment? The name and comment of the function should note how this should only be called for Google search URLs. https://codereview.chromium.org/342053002/diff/80001/chrome/browser/prerender... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/prerender... chrome/browser/prerender/prerender_manager.h:390: // Must be called on the UI thread. Nit: No need to add the last sentence since the class comment already says this. https://codereview.chromium.org/342053002/diff/80001/components/google/core/b... File components/google/core/browser/google_search_metrics.h (right): https://codereview.chromium.org/342053002/diff/80001/components/google/core/b... components/google/core/browser/google_search_metrics.h:40: virtual void RecordAndroidGoogleSearch(AccessPoint ap, Why is this virtual?
pkasting@: Addressed comments. PTAL. Thanks. https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... File chrome/browser/google/google_search_counter.cc (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_search_counter.cc:36: // receive two notifications events for the same page. On 2014/06/19 21:20:32, Peter Kasting wrote: > Nit: notifications events -> notifications/notification events (pick one) > Done. > What does this comment mean, though? When I submit a search query from Android Chrome omnibox to the Instant search base page, I am seeing two commit entry notifications for the same search query. > Why are we getting two notifications, and > what types are they? This seems strange and wrong. I am not sure why I am getting two notifications. They are of PAGE_TRANSITION_GENERATED type. I am testing my changes against a custom google server base url which has my local changes. When I test against google.com, I am not seeing these double notifications for search URLs. I didn't have time to confirm that the problem is in my server side code. Therefore, I added this check to work around this issue. Once I land this CL, I will investigate this issue and remove this check. Added a TODO to do that. https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... File chrome/browser/google/google_search_counter_android.cc (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_search_counter_android.cc:52: switch (type) { On 2014/06/19 21:20:32, Peter Kasting wrote: > Just DCHECK_EQ(content::NOTIFICATION_NAV_ENTRY_COMMITTED, type); and then handle > it. Kill the switch entirely. Done. https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... File chrome/browser/google/google_search_counter_android.h (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_search_counter_android.h:26: // content::NotificationObserver On 2014/06/19 21:20:32, Peter Kasting wrote: > Nit: Add trailing colon Done. https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... File chrome/browser/google/google_util.cc (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_util.cc:78: bool IsOmniboxGoogleSearchNavigation(const content::NavigationEntry& entry) { On 2014/06/19 21:20:32, Peter Kasting wrote: > Given that these two functions are extremely short and only called once, I think > it would be better to inline their contents below. This would avoid the need to > check or comment about the status of IsGoogleSearchURL(). Done. https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_util.cc:82: return stripped_transition == content::PAGE_TRANSITION_GENERATED; On 2014/06/19 21:20:32, Peter Kasting wrote: > Don't you also need to check for PAGE_TRANSITION_FROM_ADDRESS_BAR? GENERATED > alone could be from different sources. I moved this code from GoogleSearchCounter to here. I retained the original code. But it seems reasonable to check for PAGE_TRANSITION_FROM_ADDRESS_BAR. Can you give some examples of other sources? So that I can test and verify that I am not causing any regression after adding this check. https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_util.cc:91: std::string::npos; On 2014/06/19 21:20:32, Peter Kasting wrote: > Nit: Indent 4, not even Fixed. https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_util.cc:242: // an Omnibox search. On 2014/06/19 21:20:32, Peter Kasting wrote: > This comment goes into detail that isn't visible here. Either eliminate it or > change it to only refer to externally-visible aspects of > IsOmniboxGoogleSearchNavigation(). Inlined IsOmniboxGoogleSearchNavigation() definition here. Keeping the comments as such. https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... File chrome/browser/google/google_util.h (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_util.h:117: GoogleSearchMetrics::AccessPoint GetGoogleSearchAccessPointForNavEntry( On 2014/06/19 21:20:33, Peter Kasting wrote: > Nit: Comment? The name and comment of the function should note how this should > only be called for Google search URLs. Done. https://codereview.chromium.org/342053002/diff/80001/chrome/browser/prerender... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/prerender... chrome/browser/prerender/prerender_manager.h:390: // Must be called on the UI thread. On 2014/06/19 21:20:33, Peter Kasting wrote: > Nit: No need to add the last sentence since the class comment already says this. Done. https://codereview.chromium.org/342053002/diff/80001/components/google/core/b... File components/google/core/browser/google_search_metrics.h (right): https://codereview.chromium.org/342053002/diff/80001/components/google/core/b... components/google/core/browser/google_search_metrics.h:40: virtual void RecordAndroidGoogleSearch(AccessPoint ap, On 2014/06/19 21:20:33, Peter Kasting wrote: > Why is this virtual? For testing. I am working on the tests.
jam@: Please review chrome/browser/chrome_browser_main_android.cc changes. Thanks.
https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... File chrome/browser/google/google_search_counter.cc (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_search_counter.cc:36: // receive two notifications events for the same page. On 2014/06/19 22:52:49, kmadhusu wrote: > On 2014/06/19 21:20:32, Peter Kasting wrote: > > What does this comment mean, though? > > When I submit a search query from Android Chrome omnibox to the Instant search > base page, I am seeing two commit entry notifications for the same search query. > > > Why are we getting two notifications, and > > what types are they? This seems strange and wrong. > > I am not sure why I am getting two notifications. They are of > PAGE_TRANSITION_GENERATED type. I am testing my changes against a custom google > server base url which has my local changes. When I test against http://google.com, I am > not seeing these double notifications for search URLs. I didn't have time to > confirm that the problem is in my server side code. Therefore, I added this > check to work around this issue. Once I land this CL, I will investigate this > issue and remove this check. Added a TODO to do that. > :( This would normally cause me to reject this patch until you determine the underlying cause of the issue, even if you don't actually have the cause fixed in this change. That way we can be sure we're not also missing some unknown other problems. Why do you not have time to figure this out? Why can't this patch land when it's ready and be merged if it's reasonable? https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... File chrome/browser/google/google_util.cc (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_util.cc:82: return stripped_transition == content::PAGE_TRANSITION_GENERATED; On 2014/06/19 22:52:49, kmadhusu wrote: > On 2014/06/19 21:20:32, Peter Kasting wrote: > > Don't you also need to check for PAGE_TRANSITION_FROM_ADDRESS_BAR? GENERATED > > alone could be from different sources. > > I moved this code from GoogleSearchCounter to here. I retained the original > code. But it seems reasonable to check for PAGE_TRANSITION_FROM_ADDRESS_BAR. Can > you give some examples of other sources? So that I can test and verify that I am > not causing any regression after adding this check. I would grep the codebase for PAGE_TRANSITION_GENERATED to see. Maybe a bookmark of a Google search?
https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... File chrome/browser/google/google_search_counter.cc (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_search_counter.cc:36: // receive two notifications events for the same page. On 2014/06/19 22:58:05, Peter Kasting wrote: > On 2014/06/19 22:52:49, kmadhusu wrote: > > On 2014/06/19 21:20:32, Peter Kasting wrote: > > > What does this comment mean, though? > > > > When I submit a search query from Android Chrome omnibox to the Instant search > > base page, I am seeing two commit entry notifications for the same search > query. > > > > > Why are we getting two notifications, and > > > what types are they? This seems strange and wrong. > > > > I am not sure why I am getting two notifications. They are of > > PAGE_TRANSITION_GENERATED type. I am testing my changes against a custom > google > > server base url which has my local changes. When I test against > http://google.com, I am > > not seeing these double notifications for search URLs. I didn't have time to > > confirm that the problem is in my server side code. Therefore, I added this > > check to work around this issue. Once I land this CL, I will investigate this > > issue and remove this check. Added a TODO to do that. > > > > :( This would normally cause me to reject this patch until you determine the > underlying cause of the issue, even if you don't actually have the cause fixed > in this change. That way we can be sure we're not also missing some unknown > other problems. > > Why do you not have time to figure this out? Why can't this patch land when > it's ready and be merged if it's reasonable? My server side changes are still in prototype stage. It will take more than a day to figure out this issue. (I already spent a whole day to confirm that it is something related to my server side code). Given the size of this CL, it will be very hard to merge the entire patch later. Until my server side changes are in, I am not planning to start any experiments. Can I can remove this check for now and check in the framework? Framework is working as expected and I don't expect any problems. Once I figure out the issue, I can merge this check if needed. https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... File chrome/browser/google/google_util.cc (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_util.cc:82: return stripped_transition == content::PAGE_TRANSITION_GENERATED; On 2014/06/19 22:58:05, Peter Kasting wrote: > On 2014/06/19 22:52:49, kmadhusu wrote: > > On 2014/06/19 21:20:32, Peter Kasting wrote: > > > Don't you also need to check for PAGE_TRANSITION_FROM_ADDRESS_BAR? > GENERATED > > > alone could be from different sources. > > > > I moved this code from GoogleSearchCounter to here. I retained the original > > code. But it seems reasonable to check for PAGE_TRANSITION_FROM_ADDRESS_BAR. > Can > > you give some examples of other sources? So that I can test and verify that I > am > > not causing any regression after adding this check. > > I would grep the codebase for PAGE_TRANSITION_GENERATED to see. Maybe a > bookmark of a Google search? Okay.
lgtm
I will submit the new tests in a follow up CL. Thanks. https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... File chrome/browser/google/google_search_counter.cc (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_search_counter.cc:36: // receive two notifications events for the same page. On 2014/06/19 23:21:45, kmadhusu wrote: > On 2014/06/19 22:58:05, Peter Kasting wrote: > > On 2014/06/19 22:52:49, kmadhusu wrote: > > > On 2014/06/19 21:20:32, Peter Kasting wrote: > > > > What does this comment mean, though? > > > > > > When I submit a search query from Android Chrome omnibox to the Instant > search > > > base page, I am seeing two commit entry notifications for the same search > > query. > > > > > > > Why are we getting two notifications, and > > > > what types are they? This seems strange and wrong. > > > > > > I am not sure why I am getting two notifications. They are of > > > PAGE_TRANSITION_GENERATED type. I am testing my changes against a custom > > google > > > server base url which has my local changes. When I test against > > http://google.com, I am > > > not seeing these double notifications for search URLs. I didn't have time to > > > confirm that the problem is in my server side code. Therefore, I added this > > > check to work around this issue. Once I land this CL, I will investigate > this > > > issue and remove this check. Added a TODO to do that. > > > > > > > :( This would normally cause me to reject this patch until you determine the > > underlying cause of the issue, even if you don't actually have the cause fixed > > in this change. That way we can be sure we're not also missing some unknown > > other problems. > > > > Why do you not have time to figure this out? Why can't this patch land when > > it's ready and be merged if it's reasonable? > > My server side changes are still in prototype stage. It will take more than a > day to figure out this issue. (I already spent a whole day to confirm that it is > something related to my server side code). > > Given the size of this CL, it will be very hard to merge the entire patch later. > Until my server side changes are in, I am not planning to start any experiments. > Can I can remove this check for now and check in the framework? Framework is > working as expected and I don't expect any problems. Once I figure out the > issue, I can merge this check if needed. As we discussed offline, I removed this dupe check. https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... File chrome/browser/google/google_util.cc (right): https://codereview.chromium.org/342053002/diff/80001/chrome/browser/google/go... chrome/browser/google/google_util.cc:82: return stripped_transition == content::PAGE_TRANSITION_GENERATED; On 2014/06/19 23:21:45, kmadhusu wrote: > On 2014/06/19 22:58:05, Peter Kasting wrote: > > On 2014/06/19 22:52:49, kmadhusu wrote: > > > On 2014/06/19 21:20:32, Peter Kasting wrote: > > > > Don't you also need to check for PAGE_TRANSITION_FROM_ADDRESS_BAR? > > GENERATED > > > > alone could be from different sources. > > > > > > I moved this code from GoogleSearchCounter to here. I retained the original > > > code. But it seems reasonable to check for PAGE_TRANSITION_FROM_ADDRESS_BAR. > > Can > > > you give some examples of other sources? So that I can test and verify that > I > > am > > > not causing any regression after adding this check. > > > > I would grep the codebase for PAGE_TRANSITION_GENERATED to see. Maybe a > > bookmark of a Google search? > > Okay. Updated the check.
cbentzel@: Please review chrome/browser/prerender/prerender_manager.h changes. asvitkine@: Please review tools/metrics/histograms/histograms.xml changes. Thanks.
LGTM https://codereview.chromium.org/342053002/diff/120001/chrome/browser/google/g... File chrome/browser/google/google_search_counter_android.cc (right): https://codereview.chromium.org/342053002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_search_counter_android.cc:39: const content::NavigationEntry& entry = *commit->entry; Nit: Could just be: const content::NavigationEntry& entry = *content::Details<content::LoadCommittedDetails>(details)->entry; https://codereview.chromium.org/342053002/diff/120001/chrome/browser/google/g... File chrome/browser/google/google_util.cc (right): https://codereview.chromium.org/342053002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_util.cc:223: // an Omnibox search. Nit: This comment isn't quite accurate to the code. How about: If the entry is FROM_ADDRESS_BAR, it comes from the omnibox; if it's GENERATED, the user was doing a search, rather than doing a navigation to a search URL (e.g. from history, or pasted in). https://codereview.chromium.org/342053002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_util.cc:234: GoogleSearchMetrics::AP_OTHER; Nit: I think this would be more readable split up: // The string "source=search_app" in the |entry| URL represents a Google // search from the Google Search App. if (entry.GetURL().query().find("source=search_app") != std::string::npos) return GoogleSearchMetrics::AP_SEARCH_APP; // For all other cases that we have not yet implemented or care to measure, we // log a generic "catch-all" metric. return GoogleSearchMetrics::AP_OTHER; https://codereview.chromium.org/342053002/diff/120001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/342053002/diff/120001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:390: bool IsEnabled() const; Nit: I suggest putting this just above AddProfileNetworkBytesIfEnabled() since both relate in some sense to whether prerendering is enabled.
Addressed nits. Thanks. https://codereview.chromium.org/342053002/diff/120001/chrome/browser/google/g... File chrome/browser/google/google_search_counter_android.cc (right): https://codereview.chromium.org/342053002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_search_counter_android.cc:39: const content::NavigationEntry& entry = *commit->entry; On 2014/06/20 01:23:18, Peter Kasting wrote: > Nit: Could just be: > > const content::NavigationEntry& entry = > *content::Details<content::LoadCommittedDetails>(details)->entry; Done. https://codereview.chromium.org/342053002/diff/120001/chrome/browser/google/g... File chrome/browser/google/google_util.cc (right): https://codereview.chromium.org/342053002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_util.cc:223: // an Omnibox search. On 2014/06/20 01:23:18, Peter Kasting wrote: > Nit: This comment isn't quite accurate to the code. How about: > > If the entry is FROM_ADDRESS_BAR, it comes from the omnibox; if it's GENERATED, > the user was doing a search, rather than doing a navigation to a search URL > (e.g. from history, or pasted in). Done. https://codereview.chromium.org/342053002/diff/120001/chrome/browser/google/g... chrome/browser/google/google_util.cc:234: GoogleSearchMetrics::AP_OTHER; On 2014/06/20 01:23:19, Peter Kasting wrote: > Nit: I think this would be more readable split up: > > // The string "source=search_app" in the |entry| URL represents a Google > // search from the Google Search App. > if (entry.GetURL().query().find("source=search_app") != std::string::npos) > return GoogleSearchMetrics::AP_SEARCH_APP; > > // For all other cases that we have not yet implemented or care to measure, we > // log a generic "catch-all" metric. > return GoogleSearchMetrics::AP_OTHER; Done. https://codereview.chromium.org/342053002/diff/120001/chrome/browser/prerende... File chrome/browser/prerender/prerender_manager.h (right): https://codereview.chromium.org/342053002/diff/120001/chrome/browser/prerende... chrome/browser/prerender/prerender_manager.h:390: bool IsEnabled() const; On 2014/06/20 01:23:19, Peter Kasting wrote: > Nit: I suggest putting this just above AddProfileNetworkBytesIfEnabled() since > both relate in some sense to whether prerendering is enabled. Done.
(FYI) Rebased my changes. r278448 componentized google_util.*. I had to move my new util function from google_util to GoogleSearchCounter because it was violating gyp dependency rules by accessing NavigationEntry.
lgtm
Instead of introducing a new top-level name "AndroidGoogleSearch", I suggest simply logging these as GoogleSearch.AccessPoint_PrerenderDisabled and GoogleSearch.AccessPoint_PrerenderEnabled. You can mention in histograms.xml that these are currently only logged on Android. In histograms.xml, you can use the histogram-suffixes feature to basically permute the existing GoogleSearch.AccessPoint histogram with these suffixes. However, I have a question for you... are you sure that there are cases where prerender is off on Android? I recently looked at this and it seems --prerender=enabled might always be passed to the Android browser process according to chrome://version. (But it's definitely possible I'm missing context or recent developments in this area.)
On 2014/06/20 15:07:31, Alexei Svitkine wrote: > Instead of introducing a new top-level name "AndroidGoogleSearch", I suggest > simply logging these as GoogleSearch.AccessPoint_PrerenderDisabled and > GoogleSearch.AccessPoint_PrerenderEnabled. You can mention in histograms.xml > that these are currently only logged on Android. > > In histograms.xml, you can use the histogram-suffixes feature to basically > permute the existing GoogleSearch.AccessPoint histogram with these suffixes. > > However, I have a question for you... are you sure that there are cases where > prerender is off on Android? I recently looked at this and it seems > --prerender=enabled might always be passed to the Android browser process > according to chrome://version. (But it's definitely possible I'm missing context > or recent developments in this area.) Or perhaps "Prerender" is just an ambiguous term and maybe in this case you're talking about something else than --prerender=enabled? In which case, I suggest to find less ambiguous terminology to use.
On 2014/06/20 15:09:39, Alexei Svitkine wrote: > On 2014/06/20 15:07:31, Alexei Svitkine wrote: > > Instead of introducing a new top-level name "AndroidGoogleSearch", I suggest > > simply logging these as GoogleSearch.AccessPoint_PrerenderDisabled and > > GoogleSearch.AccessPoint_PrerenderEnabled. You can mention in histograms.xml > > that these are currently only logged on Android. > > > > In histograms.xml, you can use the histogram-suffixes feature to basically > > permute the existing GoogleSearch.AccessPoint histogram with these suffixes. > > > > However, I have a question for you... are you sure that there are cases where > > prerender is off on Android? I recently looked at this and it seems > > --prerender=enabled might always be passed to the Android browser process > > according to chrome://version. (But it's definitely possible I'm missing > context > > or recent developments in this area.) > > Or perhaps "Prerender" is just an ambiguous term and maybe in this case you're > talking about something else than --prerender=enabled? In which case, I suggest > to find less ambiguous terminology to use. Prerender is definitely not enabled for all of Android? It's currently tied to a user setting that defaults to "Only on Wifi".
On 2014/06/20 15:22:25, samarth wrote: > On 2014/06/20 15:09:39, Alexei Svitkine wrote: > > On 2014/06/20 15:07:31, Alexei Svitkine wrote: > > > Instead of introducing a new top-level name "AndroidGoogleSearch", I suggest > > > simply logging these as GoogleSearch.AccessPoint_PrerenderDisabled and > > > GoogleSearch.AccessPoint_PrerenderEnabled. You can mention in histograms.xml > > > that these are currently only logged on Android. > > > > > > In histograms.xml, you can use the histogram-suffixes feature to basically > > > permute the existing GoogleSearch.AccessPoint histogram with these suffixes. > > > > > > However, I have a question for you... are you sure that there are cases > where > > > prerender is off on Android? I recently looked at this and it seems > > > --prerender=enabled might always be passed to the Android browser process > > > according to chrome://version. (But it's definitely possible I'm missing > > context > > > or recent developments in this area.) > > > > Or perhaps "Prerender" is just an ambiguous term and maybe in this case you're > > talking about something else than --prerender=enabled? In which case, I > suggest > > to find less ambiguous terminology to use. > > Prerender is definitely not enabled for all of Android? It's currently tied to > a user setting that defaults to "Only on Wifi". (Oops, that was not supposed to be a question :))
Does the --prerender=enabled command-line not correctly indicate whether prerender is enabled, then? On Fri, Jun 20, 2014 at 11:22 AM, <samarth@chromium.org> wrote: > On 2014/06/20 15:22:25, samarth wrote: > >> On 2014/06/20 15:09:39, Alexei Svitkine wrote: >> > On 2014/06/20 15:07:31, Alexei Svitkine wrote: >> > > Instead of introducing a new top-level name "AndroidGoogleSearch", I >> > suggest > >> > > simply logging these as GoogleSearch.AccessPoint_PrerenderDisabled >> and >> > > GoogleSearch.AccessPoint_PrerenderEnabled. You can mention in >> > histograms.xml > >> > > that these are currently only logged on Android. >> > > >> > > In histograms.xml, you can use the histogram-suffixes feature to >> basically >> > > permute the existing GoogleSearch.AccessPoint histogram with these >> > suffixes. > >> > > >> > > However, I have a question for you... are you sure that there are >> cases >> where >> > > prerender is off on Android? I recently looked at this and it seems >> > > --prerender=enabled might always be passed to the Android browser >> process >> > > according to chrome://version. (But it's definitely possible I'm >> missing >> > context >> > > or recent developments in this area.) >> > >> > Or perhaps "Prerender" is just an ambiguous term and maybe in this case >> > you're > >> > talking about something else than --prerender=enabled? In which case, I >> suggest >> > to find less ambiguous terminology to use. >> > > Prerender is definitely not enabled for all of Android? It's currently >> tied >> > to > >> a user setting that defaults to "Only on Wifi". >> > > (Oops, that was not supposed to be a question :)) > > https://codereview.chromium.org/342053002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Chris will know the details better, but I suspect the command line flag only controls whether prerender _can_ be turned on. (For example, you can end up in a prerender control group.) But in the end the user setting has the final say in turning off prerendering.
On 2014/06/20 15:30:16, samarth wrote: > Chris will know the details better, but I suspect the command line flag only > controls whether prerender _can_ be turned on. (For example, you can end up in > a prerender control group.) But in the end the user setting has the final say > in turning off prerendering. Correct. Command line flag will make it so it's forced on for finch (so you don't end up in a control group). However, the network prediction or prerender-only-on-wifi style settings for the current profile will still apply.
All right, thanks for the explanation. It is a bit confusing, when both things use "prerender enabled" terminology but mean different things. :\ On Fri, Jun 20, 2014 at 11:36 AM, <cbentzel@chromium.org> wrote: > On 2014/06/20 15:30:16, samarth wrote: > >> Chris will know the details better, but I suspect the command line flag >> only >> controls whether prerender _can_ be turned on. (For example, you can end >> up >> > in > >> a prerender control group.) But in the end the user setting has the >> final say >> in turning off prerendering. >> > > Correct. > > Command line flag will make it so it's forced on for finch (so you don't > end up > in a control group). However, the network prediction or > prerender-only-on-wifi > style settings for the current profile will still apply. > > https://codereview.chromium.org/342053002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Alexei, updated histogram name. PTAL. Thanks.
+davidben for the prerender changes, he's a better reviewer for this than me now. https://codereview.chromium.org/342053002/diff/230001/chrome/browser/google/g... File chrome/browser/google/google_search_counter_android.cc (right): https://codereview.chromium.org/342053002/diff/230001/chrome/browser/google/g... chrome/browser/google/google_search_counter_android.cc:38: prerender::PrerenderManager* prerender_manager = prerender_manager might be NULL. Maybe in this case it isn't?
+davidben for the prerender changes, he's a better reviewer for this than me now.
https://codereview.chromium.org/342053002/diff/230001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/342053002/diff/230001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:8190: +<histogram name="GoogleSearch.AccessPoint_PrerenderDisabled" Nit: Please use the histogram_suffixes feature (see other examples in this file) to define the suffixes on the existing histogram.
cbentzel@: Addressed comments. PTAL. Thanks. https://codereview.chromium.org/342053002/diff/230001/chrome/browser/google/g... File chrome/browser/google/google_search_counter_android.cc (right): https://codereview.chromium.org/342053002/diff/230001/chrome/browser/google/g... chrome/browser/google/google_search_counter_android.cc:38: prerender::PrerenderManager* prerender_manager = On 2014/06/20 16:27:11, cbentzel wrote: > prerender_manager might be NULL. Maybe in this case it isn't? Based on my testing, PrerenderManagerFactory is instantiated before we reach this code and hence prerender_manager is not NULL.
asvitkine@: Updated histogram. PTAL. Thanks. https://codereview.chromium.org/342053002/diff/230001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/342053002/diff/230001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:8190: +<histogram name="GoogleSearch.AccessPoint_PrerenderDisabled" On 2014/06/20 16:27:35, Alexei Svitkine wrote: > Nit: Please use the histogram_suffixes feature (see other examples in this file) > to define the suffixes on the existing histogram. Done.
LGTM % remaining comments https://codereview.chromium.org/342053002/diff/250001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/342053002/diff/250001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:8184: + <owner>Please list the metric's owners. Add more owner tags as needed.</owner> Perhaps pkasting@ or samarth@ should own this? If no one else volunteers, you can put me here I guess, but I'd prefer it to be someone more familiar with the implementation. https://codereview.chromium.org/342053002/diff/250001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:46358: + <suffix name="_PrerenderDisabled" Nit: Add yourself as an owner to this histogram_suffixes element.
Thanks. https://codereview.chromium.org/342053002/diff/250001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/342053002/diff/250001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:8184: + <owner>Please list the metric's owners. Add more owner tags as needed.</owner> On 2014/06/20 16:57:54, Alexei Svitkine wrote: > Perhaps pkasting@ or samarth@ should own this? If no one else volunteers, you > can put me here I guess, but I'd prefer it to be someone more familiar with the > implementation. I added my name to the owners list for now. If pkasting@ or samarth@ wants to own this, I can add their names later. https://codereview.chromium.org/342053002/diff/250001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:46358: + <suffix name="_PrerenderDisabled" On 2014/06/20 16:57:54, Alexei Svitkine wrote: > Nit: Add yourself as an owner to this histogram_suffixes element. Done.
chrome/browser/prerender LGTM
On 2014/06/20 17:19:31, David Benjamin wrote: > chrome/browser/prerender LGTM Thanks David.
The CQ bit was checked by kmadhusu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/342053002/270001
Message was sent while issue was closed.
Committed patchset #10 manually as r278835 (presubmit successful). |
