Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
110893004:
[SiteChip] Add UMA, finish wiring site chip experiment up to the omnibox. (Closed)
|
Created:
7 years ago by Greg Billock Modified:
7 years ago CC:
chromium-reviews, tfarina, Justin Donnelly, Peter Kasting, Ilya Sherman Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[SiteChip] Add UMA, finish wiring site chip experiment up to the omnibox.
BUG=315944
R=msw@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240652
Patch Set 1 #Patch Set 2 : . #
Total comments: 8
Patch Set 3 : Rebase #
Total comments: 1
Patch Set 4 : Fix layout math mistake #Patch Set 5 : Repair bad 16x16 spacing value merge. #
Total comments: 5
Patch Set 6 : Add chromeactions.txt #Patch Set 7 : Add background color #Patch Set 8 : rebase #
Total comments: 5
Messages
Total messages: 23 (0 generated)
This finishes by hooking up some UMAs and finalizing the connection to the omnibox on button press.
https://codereview.chromium.org/110893004/diff/10001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/110893004/diff/10001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:445: UMA_HISTOGRAM_COUNTS("SiteChip.Pressed", 1); What are you trying to record? This will do a decent job of recording the number of times a single user has clicked the SiteChip, and presenting that locally at about:histograms. But, you'll need to modify tools/metrics/histograms/histograms.xml to get this shown correctly on the UMA dashboard and aggregated for respondant Chrome users, and this format isn't good for that type of data collection anyway (you'd just see the aggregate number of times users have clicked the site chip, not a distribution of how many times each user clicks the button in a given session or anything). Also, this number alone doesn't give much context, you might want to record the number of SiteChips made visible and/or updated, how many times any are hovered, or something else that would give this a frame of reference. https://codereview.chromium.org/110893004/diff/10001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:447: toolbar_view_->location_bar()->GetOmniboxView()->SetFocus(); nit: make a local OmniboxView* omnibox_view = toolbar_view_->location_bar()->GetOmniboxView(); for convenience here. https://codereview.chromium.org/110893004/diff/10001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:451: //toolbar_view_->location_bar()->GetOmniboxView()->SelectAll(true); Remove this.
https://codereview.chromium.org/110893004/diff/10001/chrome/browser/ui/omnibo... File chrome/browser/ui/omnibox/omnibox_view.h (right): https://codereview.chromium.org/110893004/diff/10001/chrome/browser/ui/omnibo... chrome/browser/ui/omnibox/omnibox_view.h:252: void ShowURL(); Also, make sure you update the definition order to match this new declaration order, and don't conflict with https://codereview.chromium.org/111873004
https://codereview.chromium.org/110893004/diff/30001/chrome/browser/ui/omnibo... File chrome/browser/ui/omnibox/omnibox_view.h (right): https://codereview.chromium.org/110893004/diff/30001/chrome/browser/ui/omnibo... chrome/browser/ui/omnibox/omnibox_view.h:252: void ShowURL(); Note: this replicates Justin's change. That's in the CQ and so this should merge and go away once it is in.
Please address my other comments.
https://codereview.chromium.org/110893004/diff/10001/chrome/browser/ui/omnibo... File chrome/browser/ui/omnibox/omnibox_view.h (right): https://codereview.chromium.org/110893004/diff/10001/chrome/browser/ui/omnibo... chrome/browser/ui/omnibox/omnibox_view.h:252: void ShowURL(); On 2013/12/12 00:22:20, msw wrote: > Also, make sure you update the definition order to match this new declaration > order, and don't conflict with https://codereview.chromium.org/111873004 Yeah, that change should go in first -- this will merge out at that point before this commits. https://codereview.chromium.org/110893004/diff/10001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/110893004/diff/10001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:445: UMA_HISTOGRAM_COUNTS("SiteChip.Pressed", 1); On 2013/12/12 00:17:49, msw wrote: > What are you trying to record? This will do a decent job of recording the number > of times a single user has clicked the SiteChip, and presenting that locally at > about:histograms. But, you'll need to modify > tools/metrics/histograms/histograms.xml to get this shown correctly on the UMA > dashboard and aggregated for respondant Chrome users, and this format isn't good > for that type of data collection anyway (you'd just see the aggregate number of > times users have clicked the site chip, not a distribution of how many times > each user clicks the button in a given session or anything). Also, this number > alone doesn't give much context, you might want to record the number of > SiteChips made visible and/or updated, how many times any are hovered, or > something else that would give this a frame of reference. Yeah, I'll add the histograms.xml descriptor separately. Measuring hover is probably a good plan. I don't know that we really have a plan for what we'd do with measurements like per-user per-session clicks or something. We want to measure usage, but we don't expect much interaction, so it's not clear what we'd do with more information. https://codereview.chromium.org/110893004/diff/10001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:447: toolbar_view_->location_bar()->GetOmniboxView()->SetFocus(); On 2013/12/12 00:17:49, msw wrote: > nit: make a local OmniboxView* omnibox_view = > toolbar_view_->location_bar()->GetOmniboxView(); for convenience here. After Justin's change, we won't need SetFocus, and probably not SetCaretVisibility either. I'm planning to delete those lines at some point. https://codereview.chromium.org/110893004/diff/10001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:451: //toolbar_view_->location_bar()->GetOmniboxView()->SelectAll(true); On 2013/12/12 00:17:49, msw wrote: > Remove this. Done. https://codereview.chromium.org/110893004/diff/70001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/110893004/diff/70001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:107: const int k16x16IconTrailingSpacing = 2; This repairs a bad merge I must have made at some point. the leading and trailing were supposed to slit the previous k16x16IconSpacing value, which was 3. (19-16)
On 2013/12/12 20:28:58, msw wrote: > Please address my other comments. done (was working separately and hadn't seen them yet)
https://codereview.chromium.org/110893004/diff/70001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/110893004/diff/70001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:445: UMA_HISTOGRAM_COUNTS("SiteChip.Pressed", 1); Is this worthwhile if you'll have the SiteChipPress user action? https://codereview.chromium.org/110893004/diff/70001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:446: content::RecordAction(content::UserMetricsAction("SiteChipPress")); According to the guide: https://wiki.corp.google.com/twiki/bin/view/Main/ChromeUserExperienceMetrics "When adding a new action, run the script tools/metrics/actions/extract_actions.py --hash (from your Chromium checkout) which updates the list of actions in tools/metrics/actions/chromeactions.txt (which you should add to your CL)."
+Mark. Can you take a look and see if this looks like a good set of metrics to collect for site chip use? https://codereview.chromium.org/110893004/diff/70001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/110893004/diff/70001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:445: UMA_HISTOGRAM_COUNTS("SiteChip.Pressed", 1); On 2013/12/12 21:09:54, msw wrote: > Is this worthwhile if you'll have the SiteChipPress user action? I'm not sure. I thought the user actions just go into timing metrics. https://codereview.chromium.org/110893004/diff/70001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:446: content::RecordAction(content::UserMetricsAction("SiteChipPress")); On 2013/12/12 21:09:54, msw wrote: > According to the guide: > https://wiki.corp.google.com/twiki/bin/view/Main/ChromeUserExperienceMetrics > "When adding a new action, run the script > tools/metrics/actions/extract_actions.py --hash (from your Chromium checkout) > which updates the list of actions in tools/metrics/actions/chromeactions.txt > (which you should add to your CL)." Thanks. I haven't used these user actions before.
LGTM modulo Mark's comments regarding the necessity of both metrics.
I can foresee the UserAction as being useful. As for the histogram, msw@ is strictly right that it's not necessary if you have the user action. After all, one can simply sum all user actions to get the total--what would be in the histogram--for that user. However, our internal UMA dashboards do not currently show user actions split by field trial group. Hence, for some quick high-level metrics (how often is the button clicked in a certain field trial group) it would be worthwhile to have a histogram for the data rather than having to write a logs processing script to do the same thing. The histogram has little memory impact so I think there's no harm in having it. --mark
On 2013/12/12 22:36:08, Mark P wrote: > I can foresee the UserAction as being useful. > > As for the histogram, msw@ is strictly right that it's not necessary if you have > the user action. After all, one can simply sum all user actions to get the > total--what would be in the histogram--for that user. However, our internal UMA > dashboards do not currently show user actions split by field trial group. > Hence, for some quick high-level metrics (how often is the button clicked in a > certain field trial group) it would be worthwhile to have a histogram for the > data rather than having to write a logs processing script to do the same thing. > The histogram has little memory impact so I think there's no harm in having it. > > --mark Thanks. I'll leave it in then. msw, I merged to Justin's change, so ShowURL is merged and disappeared. I put in your plan to use Label::SetBackgroundColor, which works great, so that's in. Want to take another once-over?
LGTM!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/110893004/110001
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/gbillock@chromium.org/110893004/110001
The failure looked like bot flakiness to me. I put this back in the CQ.
On 2013/12/13 06:04:53, Justin Donnelly wrote: > The failure looked like bot flakiness to me. I put this back in the CQ. thanks, Justin.
Failed to apply patch for tools/metrics/actions/chromeactions.txt: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file tools/metrics/actions/chromeactions.txt Hunk #1 FAILED at 247. Hunk #2 succeeded at 1696 with fuzz 1 (offset 9 lines). 1 out of 2 hunks FAILED -- saving rejects to file tools/metrics/actions/chromeactions.txt.rej Patch: tools/metrics/actions/chromeactions.txt Index: tools/metrics/actions/chromeactions.txt diff --git a/tools/metrics/actions/chromeactions.txt b/tools/metrics/actions/chromeactions.txt index a927c53dab3166cafa73218114073076e8be8371..3c7b4a5340e2744c4219468983917dbeb1339498 100644 --- a/tools/metrics/actions/chromeactions.txt +++ b/tools/metrics/actions/chromeactions.txt @@ -247,9 +247,11 @@ 0x95a93ad59fa23f78 BookmarkBar_ContextMenu_OpenAllIncognito 0x798f8467fbe49e1e BookmarkBar_ContextMenu_OpenInNewTab 0x14c4432cb3aca04b BookmarkBar_ContextMenu_OpenInNewWindow +0x933a912dd22eacb3 BookmarkBar_ContextMenu_Redo 0xd8a39c089af645f1 BookmarkBar_ContextMenu_Remove 0xa90364ba80763db0 BookmarkBar_ContextMenu_RemoveFromBookmarkBar 0xa905159308ac6adc BookmarkBar_ContextMenu_ShowInFolder +0xb985dcc55f9d73a4 BookmarkBar_ContextMenu_Undo 0x07801d0423d26682 BookmarkBar_CtxMenu 0x8623e5f54147dbe6 BookmarkBar_DragButton 0x4b37738130b32b3e BookmarkBar_DragEnd @@ -1687,6 +1689,7 @@ 0x3b86156dcf560cdb ShowSections_RecentSitesEnabled 0x5118181c3ece5e84 ShowSessions 0x1a4ebb180ba59b06 Shutdown +0x09d79ad36a385dc7 SiteChipPress 0x26f93e6e68e28a69 Star 0x2fbe99005588ef01 StartupTick 0x11a755d598c0c417 Stop
Message was sent while issue was closed.
Committed patchset #8 manually as r240652 (presubmit successful).
Message was sent while issue was closed.
Post-vacation comments: https://codereview.chromium.org/110893004/diff/130001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/110893004/diff/130001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/site_chip_view.cc:113: const SkColor kBrokenSSLBackgroundColor = SkColorSetRGB(253, 196, 36); Do we want these colors on every possible toolbar background color? (For reference, the bubble colors in the omnibox are semitransparent; you could copy the colors + alpha values from those.) https://codereview.chromium.org/110893004/diff/130001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/site_chip_view.cc:360: host_label_->SetBackgroundColor(label_background); Hmm, maybe we should be using GetReadableColor() somewhere in all this mix to ensure the text is always readable over the background, and perhaps also that the background colors here always stick out against the toolbar color.
Message was sent while issue was closed.
https://codereview.chromium.org/110893004/diff/130001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/110893004/diff/130001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/site_chip_view.cc:113: const SkColor kBrokenSSLBackgroundColor = SkColorSetRGB(253, 196, 36); On 2013/12/18 00:33:35, Peter Kasting wrote: > Do we want these colors on every possible toolbar background color? (For > reference, the bubble colors in the omnibox are semitransparent; you could copy > the colors + alpha values from those.) That's a good question. The images have a bit of transparency, so I could be using SetRGBA here. I wasn't sure how that would interact with the label background and picking the high-contrast color, is why I didn't use it. The transparency probably isn't enough that it'll make a difference in practice. https://codereview.chromium.org/110893004/diff/130001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/site_chip_view.cc:360: host_label_->SetBackgroundColor(label_background); On 2013/12/18 00:33:35, Peter Kasting wrote: > Hmm, maybe we should be using GetReadableColor() somewhere in all this mix to > ensure the text is always readable over the background, and perhaps also that > the background colors here always stick out against the toolbar color. Michael helped me work this out -- that's what Label does internally to ensure high contrast.
Message was sent while issue was closed.
https://codereview.chromium.org/110893004/diff/130001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/110893004/diff/130001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/site_chip_view.cc:113: const SkColor kBrokenSSLBackgroundColor = SkColorSetRGB(253, 196, 36); On 2013/12/18 16:43:07, Greg Billock wrote: > On 2013/12/18 00:33:35, Peter Kasting wrote: > > Do we want these colors on every possible toolbar background color? (For > > reference, the bubble colors in the omnibox are semitransparent; you could > copy > > the colors + alpha values from those.) > > That's a good question. The images have a bit of transparency, so I could be > using SetRGBA here. I wasn't sure how that would interact with the label > background and picking the high-contrast color, is why I didn't use it. I think rather than hardcode these values (which makes for subtle maintenance issues if we change the images, since we won't automatically notice these are out of date), you should look in the code for e.g. the EV bubble and find how it samples the correct color from the image grid and then blends it against the background. Then the code below should do that and these constants can disappear entirely. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
