Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
111423005:
[Most Visited] Log suggestion provider to UMA (Closed)
|
Created:
7 years ago by Mathieu Modified:
7 years ago CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, dominich, jfweitz+watch_chromium.org, David Black, arv+watch_chromium.org, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Most Visited] Log suggestion provider to UMA
Will add a query param to the /log.html endpoint in order to be able to log the suggestion provider to UMA.
BUG=None
TEST=MostVisitedIframeSourceTest
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241064
Patch Set 1 #Patch Set 2 : Cleaning #
Total comments: 12
Patch Set 3 : Addressed comments #Patch Set 4 : clean #
Total comments: 8
Patch Set 5 : Histogram changes #Patch Set 6 : rebase #
Total comments: 4
Patch Set 7 : Addressed jered #Patch Set 8 : Addressed jered (bis) #
Total comments: 4
Patch Set 9 : nits #
Total comments: 2
Patch Set 10 : rebase #Patch Set 11 : re-rebase #Patch Set 12 : rebase? #
Messages
Total messages: 36 (0 generated)
Hi, This is ready for review, would like to land before branch cut in a few days. asvitkine: a look at the histogram macro in most_visited_iframe_source.h jered: review of the logic. Thanks!
Just a quick drive-by partial review. https://codereview.chromium.org/111423005/diff/20001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_util.js (right): https://codereview.chromium.org/111423005/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_util.js:170: data.provider = params.pr || ''; I suggest: params.pr || SERVER; With SERVER being a precanned string indicating a generic server provider. I'd say "server". https://codereview.chromium.org/111423005/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_util.js:178: return; I suggest adding: data.provider = CLIENT (see above) https://codereview.chromium.org/111423005/diff/20001/chrome/browser/search/mo... File chrome/browser/search/most_visited_iframe_source.h (right): https://codereview.chromium.org/111423005/diff/20001/chrome/browser/search/mo... chrome/browser/search/most_visited_iframe_source.h:13: class MostVisitedIframeSource; Do you really need to forward declare this for the macro? I would expect not. https://codereview.chromium.org/111423005/diff/20001/chrome/browser/search/mo... chrome/browser/search/most_visited_iframe_source.h:15: #define MV_CLICK_HISTOGRAM(position, provider) \ Do you have to do this in the .h? It pollutes the global namespace.
https://codereview.chromium.org/111423005/diff/20001/chrome/browser/search/mo... File chrome/browser/search/most_visited_iframe_source.h (right): https://codereview.chromium.org/111423005/diff/20001/chrome/browser/search/mo... chrome/browser/search/most_visited_iframe_source.h:24: base::Histogram::kUmaTargetedHistogramFlag); \ Missing the includes for all the histogram stuff.
I swear it's the last time I ping this thread for the next 12 minutes. :) https://codereview.chromium.org/111423005/diff/20001/chrome/browser/search/mo... File chrome/browser/search/most_visited_iframe_source.h (right): https://codereview.chromium.org/111423005/diff/20001/chrome/browser/search/mo... chrome/browser/search/most_visited_iframe_source.h:39: // Format string keeping track of MV clicks specific to a provider. Format string to generate the name of the histogram...
Thanks, please have a look! https://codereview.chromium.org/111423005/diff/20001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_util.js (right): https://codereview.chromium.org/111423005/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_util.js:170: data.provider = params.pr || ''; On 2013/12/12 19:37:21, beaudoin wrote: > I suggest: > params.pr || SERVER; > With SERVER being a precanned string indicating a generic server provider. I'd > say "server". Done. https://codereview.chromium.org/111423005/diff/20001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_util.js:178: return; On 2013/12/12 19:37:21, beaudoin wrote: > I suggest adding: > data.provider = CLIENT > (see above) Done. https://codereview.chromium.org/111423005/diff/20001/chrome/browser/search/mo... File chrome/browser/search/most_visited_iframe_source.h (right): https://codereview.chromium.org/111423005/diff/20001/chrome/browser/search/mo... chrome/browser/search/most_visited_iframe_source.h:13: class MostVisitedIframeSource; On 2013/12/12 19:37:21, beaudoin wrote: > Do you really need to forward declare this for the macro? I would expect not. Done. https://codereview.chromium.org/111423005/diff/20001/chrome/browser/search/mo... chrome/browser/search/most_visited_iframe_source.h:15: #define MV_CLICK_HISTOGRAM(position, provider) \ On 2013/12/12 19:37:21, beaudoin wrote: > Do you have to do this in the .h? It pollutes the global namespace. Done. https://codereview.chromium.org/111423005/diff/20001/chrome/browser/search/mo... chrome/browser/search/most_visited_iframe_source.h:24: base::Histogram::kUmaTargetedHistogramFlag); \ On 2013/12/12 19:39:51, beaudoin wrote: > Missing the includes for all the histogram stuff. Done. https://codereview.chromium.org/111423005/diff/20001/chrome/browser/search/mo... chrome/browser/search/most_visited_iframe_source.h:39: // Format string keeping track of MV clicks specific to a provider. On 2013/12/12 19:43:06, beaudoin wrote: > Format string to generate the name of the histogram... Done.
LGTM
Comments below. Also, please change histograms.xml to add the suffixes you're going to use. You can use a fieldtrial construct in histograms.xml to create suffixed versions of your histogram. https://codereview.chromium.org/111423005/diff/50001/chrome/browser/search/mo... File chrome/browser/search/most_visited_iframe_source.cc (right): https://codereview.chromium.org/111423005/diff/50001/chrome/browser/search/mo... chrome/browser/search/most_visited_iframe_source.cc:28: #define MV_CLICK_HISTOGRAM(position, provider) \ Add a comment that mentions why this can't use the standard histogram macros and documents params. https://codereview.chromium.org/111423005/diff/50001/chrome/browser/search/mo... chrome/browser/search/most_visited_iframe_source.cc:33: provider) : MostVisitedIframeSource::kMostVisitedHistogramName; \ Shouldn't you log the non-suffixed histogram too even when there is a provider? That can be done with a regular macro. https://codereview.chromium.org/111423005/diff/50001/chrome/browser/search/mo... chrome/browser/search/most_visited_iframe_source.cc:93: if (net::GetValueForKeyInQuery(url, "pr", &provider)) { Nit: {} not needed. https://codereview.chromium.org/111423005/diff/50001/chrome/browser/search/mo... File chrome/browser/search/most_visited_iframe_source.h (right): https://codereview.chromium.org/111423005/diff/50001/chrome/browser/search/mo... chrome/browser/search/most_visited_iframe_source.h:25: static const char kMostVisitedHistogramWithProvider[]; Instead of exposing this constant directly and having your tests use stringprintf on it, how about making a function that formats the histogram? Then, the string constant can be local in the .cc file and the test can call the function.
Please have a look! https://codereview.chromium.org/111423005/diff/50001/chrome/browser/search/mo... File chrome/browser/search/most_visited_iframe_source.cc (right): https://codereview.chromium.org/111423005/diff/50001/chrome/browser/search/mo... chrome/browser/search/most_visited_iframe_source.cc:28: #define MV_CLICK_HISTOGRAM(position, provider) \ On 2013/12/12 22:15:44, Alexei Svitkine wrote: > Add a comment that mentions why this can't use the standard histogram macros and > documents params. Done. https://codereview.chromium.org/111423005/diff/50001/chrome/browser/search/mo... chrome/browser/search/most_visited_iframe_source.cc:33: provider) : MostVisitedIframeSource::kMostVisitedHistogramName; \ On 2013/12/12 22:15:44, Alexei Svitkine wrote: > Shouldn't you log the non-suffixed histogram too even when there is a provider? > That can be done with a regular macro. I was combining both in this macro and passing NULL as provider for the other call. I've changed it so that the non-provider case is back to using the regular macro and this macro is more specific to the provider. https://codereview.chromium.org/111423005/diff/50001/chrome/browser/search/mo... chrome/browser/search/most_visited_iframe_source.cc:93: if (net::GetValueForKeyInQuery(url, "pr", &provider)) { On 2013/12/12 22:15:44, Alexei Svitkine wrote: > Nit: {} not needed. Done. https://codereview.chromium.org/111423005/diff/50001/chrome/browser/search/mo... File chrome/browser/search/most_visited_iframe_source.h (right): https://codereview.chromium.org/111423005/diff/50001/chrome/browser/search/mo... chrome/browser/search/most_visited_iframe_source.h:25: static const char kMostVisitedHistogramWithProvider[]; On 2013/12/12 22:15:44, Alexei Svitkine wrote: > Instead of exposing this constant directly and having your tests use > stringprintf on it, how about making a function that formats the histogram? > Then, the string constant can be local in the .cc file and the test can call the > function. Done.
Could you re-upload? codereview gives me "error: old chunk mismatch" errors...
Sorry, have a look now.
thanks, lgtm
https://codereview.chromium.org/111423005/diff/90001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_util.js (right): https://codereview.chromium.org/111423005/diff/90001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_util.js:191: !/^[a-z0-9]{0,8}$/i.test(data.provider)) /^[a-z0-9]{0,8}$/i.test(undefined) is false. Did you mean for the "0" to catch the case where provider is undefined? https://codereview.chromium.org/111423005/diff/90001/chrome/browser/search/mo... File chrome/browser/search/most_visited_iframe_source.cc (right): https://codereview.chromium.org/111423005/diff/90001/chrome/browser/search/mo... chrome/browser/search/most_visited_iframe_source.cc:30: // provider, and UMA_HISTOGRAM_ENUMERATION caches a given histogram object at Where are you using UMA_HISTOGRAM_ENUMERATION here? Can this not just be an inline function with the text of this macro, minus the do...while?
Please have a look :) https://codereview.chromium.org/111423005/diff/90001/chrome/browser/resources... File chrome/browser/resources/local_ntp/most_visited_util.js (right): https://codereview.chromium.org/111423005/diff/90001/chrome/browser/resources... chrome/browser/resources/local_ntp/most_visited_util.js:191: !/^[a-z0-9]{0,8}$/i.test(data.provider)) On 2013/12/13 19:39:38, Jered wrote: > /^[a-z0-9]{0,8}$/i.test(undefined) is false. Did you mean for the "0" to catch > the case where provider is undefined? From the if..else above it should never be undefined, which is why I removed my original condition of (data.provider && !/^[a-z0-9]{0,8}$/i.test(data.provider)) But let me know if you feel strongly about this. https://codereview.chromium.org/111423005/diff/90001/chrome/browser/search/mo... File chrome/browser/search/most_visited_iframe_source.cc (right): https://codereview.chromium.org/111423005/diff/90001/chrome/browser/search/mo... chrome/browser/search/most_visited_iframe_source.cc:30: // provider, and UMA_HISTOGRAM_ENUMERATION caches a given histogram object at On 2013/12/13 19:39:38, Jered wrote: > Where are you using UMA_HISTOGRAM_ENUMERATION here? Can this not just be an > inline function with the text of this macro, minus the do...while? You're right, done. Made it a private function so that it can use my other private function, GetHistogramNameForProvider.
still lgtm, some more nits https://codereview.chromium.org/111423005/diff/130001/chrome/browser/search/m... File chrome/browser/search/most_visited_iframe_source.cc (right): https://codereview.chromium.org/111423005/diff/130001/chrome/browser/search/m... chrome/browser/search/most_visited_iframe_source.cc:101: int position, const std::string& provider) { Nit: 1 param per line per the style guide. https://codereview.chromium.org/111423005/diff/130001/chrome/browser/search/m... File chrome/browser/search/most_visited_iframe_source.h (right): https://codereview.chromium.org/111423005/diff/130001/chrome/browser/search/m... chrome/browser/search/most_visited_iframe_source.h:41: // Logs the click on a Most Visited tile for a specific |provider|. Note that Nit: Add an empty line before that.
Thanks. https://codereview.chromium.org/111423005/diff/130001/chrome/browser/search/m... File chrome/browser/search/most_visited_iframe_source.cc (right): https://codereview.chromium.org/111423005/diff/130001/chrome/browser/search/m... chrome/browser/search/most_visited_iframe_source.cc:101: int position, const std::string& provider) { On 2013/12/13 20:12:49, Alexei Svitkine wrote: > Nit: 1 param per line per the style guide. Done. https://codereview.chromium.org/111423005/diff/130001/chrome/browser/search/m... File chrome/browser/search/most_visited_iframe_source.h (right): https://codereview.chromium.org/111423005/diff/130001/chrome/browser/search/m... chrome/browser/search/most_visited_iframe_source.h:41: // Logs the click on a Most Visited tile for a specific |provider|. Note that On 2013/12/13 20:12:49, Alexei Svitkine wrote: > Nit: Add an empty line before that. Done.
lgtm with nit https://chromiumcodereview.appspot.com/111423005/diff/150001/chrome/browser/s... File chrome/browser/search/most_visited_iframe_source.cc (right): https://chromiumcodereview.appspot.com/111423005/diff/150001/chrome/browser/s... chrome/browser/search/most_visited_iframe_source.cc:108: MostVisitedIframeSource::kNumMostVisited + 1, Aren't there only n and not n+1 buckets?
Thanks! https://chromiumcodereview.appspot.com/111423005/diff/150001/chrome/browser/s... File chrome/browser/search/most_visited_iframe_source.cc (right): https://chromiumcodereview.appspot.com/111423005/diff/150001/chrome/browser/s... chrome/browser/search/most_visited_iframe_source.cc:108: MostVisitedIframeSource::kNumMostVisited + 1, On 2013/12/13 21:37:59, Jered wrote: > Aren't there only n and not n+1 buckets? Spoke with Alexei, he said it's best to follow what the macro is doing (https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo..., which is what I have here), and the API over there is a bit weird (there may be an implicit 0 bucket...). To be safe, will leave as is.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/111423005/150001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/111423005/170001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/111423005/180001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/111423005/180001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/111423005/180001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/111423005/180001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/111423005/180001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/111423005/240001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/111423005/240001
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
Math: Checked it again for you. On Mon, Dec 16, 2013 at 6:21 PM, <commit-bot@chromium.org> wrote: > Commit queue rejected this change because the description was changed > between the time the change entered the commit queue and the time it > was ready to commit. You can safely check the commit box again. > > https://codereview.chromium.org/111423005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mathp@chromium.org/111423005/240001
Message was sent while issue was closed.
Change committed as 241064 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
