Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12549009:
Added finch experiment to track new menu style effect. (Closed)
|
Created:
7 years, 9 months ago by yefimt Modified:
7 years, 9 months ago CC:
chromium-reviews, tfarina, odean Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded finch experiment to track new menu style effect.
Histogram to track if anything was selected or not.
Histogram to track time to selection
Histogram to track selection index
BUG=180420
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187057
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Messages
Total messages: 27 (0 generated)
I fail to see how the index is meaningful here. Remember this code is hit for *all* menus. How is the index going to tell us anything useful? https://codereview.chromium.org/12549009/diff/2001/ui/native_theme/native_the... File ui/native_theme/native_theme.cc (right): https://codereview.chromium.org/12549009/diff/2001/ui/native_theme/native_the... ui/native_theme/native_theme.cc:35: enable_new_menu_style = Doesn't this mean we do the lookup each time? I think you need another boolean indicating if you've enable_new_menu_style has been set yet. https://codereview.chromium.org/12549009/diff/2001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_runner.cc (right): https://codereview.chromium.org/12549009/diff/2001/ui/views/controls/menu/men... ui/views/controls/menu/menu_runner.cc:26: const MenuItemView* parent = menu_item->GetParentMenuItem(); menu_item could be NULL.
New menus take more vertical space. So Idea is to see if users can find and select items that could potentially not be visible any more and require scrolling. Looking at distribution of indexes for all menus will show if it changed compare to old menus. On 2013/03/06 22:34:21, sky wrote: > I fail to see how the index is meaningful here. Remember this code is hit for > *all* menus. How is the index going to tell us anything useful? > > https://codereview.chromium.org/12549009/diff/2001/ui/native_theme/native_the... > File ui/native_theme/native_theme.cc (right): > > https://codereview.chromium.org/12549009/diff/2001/ui/native_theme/native_the... > ui/native_theme/native_theme.cc:35: enable_new_menu_style = > Doesn't this mean we do the lookup each time? I think you need another boolean > indicating if you've enable_new_menu_style has been set yet. > > https://codereview.chromium.org/12549009/diff/2001/ui/views/controls/menu/men... > File ui/views/controls/menu/menu_runner.cc (right): > > https://codereview.chromium.org/12549009/diff/2001/ui/views/controls/menu/men... > ui/views/controls/menu/menu_runner.cc:26: const MenuItemView* parent = > menu_item->GetParentMenuItem(); > menu_item could be NULL.
https://codereview.chromium.org/12549009/diff/2001/ui/native_theme/native_the... File ui/native_theme/native_theme.cc (right): https://codereview.chromium.org/12549009/diff/2001/ui/native_theme/native_the... ui/native_theme/native_theme.cc:35: enable_new_menu_style = Looking at other uses of FindFullName() I see that people are calling it every time they need this info. I think it could change during the session because Chrome is updating this information from a server periodically. On 2013/03/06 22:34:21, sky wrote: > Doesn't this mean we do the lookup each time? I think you need another boolean > indicating if you've enable_new_menu_style has been set yet. https://codereview.chromium.org/12549009/diff/2001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_runner.cc (right): https://codereview.chromium.org/12549009/diff/2001/ui/views/controls/menu/men... ui/views/controls/menu/menu_runner.cc:26: const MenuItemView* parent = menu_item->GetParentMenuItem(); This function is called only when menu_item is not NULL, but sure On 2013/03/06 22:34:21, sky wrote: > menu_item could be NULL.
I'm still not seeing how this helps to prove anything one way or the other. For one you're not logging if the index was visible or not. Additionally even if an item was not visible how can you tell whether the user intended to select it? -Scott On Wed, Mar 6, 2013 at 3:11 PM, <yefim@chromium.org> wrote: > New menus take more vertical space. So Idea is to see if users can find and > select items that could potentially not be visible any more and require > scrolling. Looking at distribution of indexes for all menus will show if it > changed compare to old menus. > > > On 2013/03/06 22:34:21, sky wrote: >> >> I fail to see how the index is meaningful here. Remember this code is hit >> for >> *all* menus. How is the index going to tell us anything useful? > > > > https://codereview.chromium.org/12549009/diff/2001/ui/native_theme/native_the... >> >> File ui/native_theme/native_theme.cc (right): > > > > https://codereview.chromium.org/12549009/diff/2001/ui/native_theme/native_the... >> >> ui/native_theme/native_theme.cc:35: enable_new_menu_style = >> Doesn't this mean we do the lookup each time? I think you need another >> boolean >> indicating if you've enable_new_menu_style has been set yet. > > > > https://codereview.chromium.org/12549009/diff/2001/ui/views/controls/menu/men... >> >> File ui/views/controls/menu/menu_runner.cc (right): > > > > https://codereview.chromium.org/12549009/diff/2001/ui/views/controls/menu/men... >> >> ui/views/controls/menu/menu_runner.cc:26: const MenuItemView* parent = >> menu_item->GetParentMenuItem(); >> menu_item could be NULL. > > > > > > https://codereview.chromium.org/12549009/
+odean@ in case he want to add something The idea is if users fail to find and select menu items that have bigger indexes then distribution of indexes will shift to the left (to lower indexes). On 2013/03/07 01:15:17, sky wrote: > I'm still not seeing how this helps to prove anything one way or the > other. For one you're not logging if the index was visible or not. > Additionally even if an item was not visible how can you tell whether > the user intended to select it? > > -Scott > > On Wed, Mar 6, 2013 at 3:11 PM, <mailto:yefim@chromium.org> wrote: > > New menus take more vertical space. So Idea is to see if users can find and > > select items that could potentially not be visible any more and require > > scrolling. Looking at distribution of indexes for all menus will show if it > > changed compare to old menus. > > > > > > On 2013/03/06 22:34:21, sky wrote: > >> > >> I fail to see how the index is meaningful here. Remember this code is hit > >> for > >> *all* menus. How is the index going to tell us anything useful? > > > > > > > > > https://codereview.chromium.org/12549009/diff/2001/ui/native_theme/native_the... > >> > >> File ui/native_theme/native_theme.cc (right): > > > > > > > > > https://codereview.chromium.org/12549009/diff/2001/ui/native_theme/native_the... > >> > >> ui/native_theme/native_theme.cc:35: enable_new_menu_style = > >> Doesn't this mean we do the lookup each time? I think you need another > >> boolean > >> indicating if you've enable_new_menu_style has been set yet. > > > > > > > > > https://codereview.chromium.org/12549009/diff/2001/ui/views/controls/menu/men... > >> > >> File ui/views/controls/menu/menu_runner.cc (right): > > > > > > > > > https://codereview.chromium.org/12549009/diff/2001/ui/views/controls/menu/men... > >> > >> ui/views/controls/menu/menu_runner.cc:26: const MenuItemView* parent = > >> menu_item->GetParentMenuItem(); > >> menu_item could be NULL. > > > > > > > > > > > > https://codereview.chromium.org/12549009/
Yefim basically summed it up. The theory is that larger menu items have made late items harder to reach / select. If users are selecting fewer late items, the median index selected would go down. We're hoping for a null-op. T On Thu, Mar 7, 2013 at 10:16 AM, <yefim@chromium.org> wrote: > +odean@ in case he want to add something > > The idea is if users fail to find and select menu items that have bigger > indexes > then distribution of indexes will shift to the left (to lower indexes). > > On 2013/03/07 01:15:17, sky wrote: > >> I'm still not seeing how this helps to prove anything one way or the >> other. For one you're not logging if the index was visible or not. >> Additionally even if an item was not visible how can you tell whether >> the user intended to select it? >> > > -Scott >> > > On Wed, Mar 6, 2013 at 3:11 PM, <mailto:yefim@chromium.org> wrote: >> > New menus take more vertical space. So Idea is to see if users can find >> and >> > select items that could potentially not be visible any more and require >> > scrolling. Looking at distribution of indexes for all menus will show >> if it >> > changed compare to old menus. >> > >> > >> > On 2013/03/06 22:34:21, sky wrote: >> >> >> >> I fail to see how the index is meaningful here. Remember this code is >> hit >> >> for >> >> *all* menus. How is the index going to tell us anything useful? >> > >> > >> > >> > >> > > https://codereview.chromium.**org/12549009/diff/2001/ui/** > native_theme/native_theme.cc<https://codereview.chromium.org/12549009/diff/2001/ui/native_theme/native_theme.cc> > >> >> >> >> File ui/native_theme/native_theme.**cc (right): >> > >> > >> > >> > >> > > https://codereview.chromium.**org/12549009/diff/2001/ui/** > native_theme/native_theme.cc#**newcode35<https://codereview.chromium.org/12549009/diff/2001/ui/native_theme/native_theme.cc#newcode35> > >> >> >> >> ui/native_theme/native_theme.**cc:35: enable_new_menu_style = >> >> Doesn't this mean we do the lookup each time? I think you need another >> >> boolean >> >> indicating if you've enable_new_menu_style has been set yet. >> > >> > >> > >> > >> > > https://codereview.chromium.**org/12549009/diff/2001/ui/** > views/controls/menu/menu_**runner.cc<https://codereview.chromium.org/12549009/diff/2001/ui/views/controls/menu/menu_runner.cc> > >> >> >> >> File ui/views/controls/menu/menu_**runner.cc (right): >> > >> > >> > >> > >> > > https://codereview.chromium.**org/12549009/diff/2001/ui/** > views/controls/menu/menu_**runner.cc#newcode26<https://codereview.chromium.org/12549009/diff/2001/ui/views/controls/menu/menu_runner.cc#newcode26> > >> >> >> >> ui/views/controls/menu/menu_**runner.cc:26: const MenuItemView* >> parent = >> >> menu_item->GetParentMenuItem()**; >> >> menu_item could be NULL. >> > >> > >> > >> > >> > >> > https://codereview.chromium.**org/12549009/<https://codereview.chromium.org/1... >> > > > > https://codereview.chromium.**org/12549009/<https://codereview.chromium.org/1... > -- Tyler Odean | Chrome | odean@google.com | 330-536-3326
I'm skeptical that the median remains the same day to day, that's why I'm dubious this data is meaningful in anyway. -Scott On Thu, Mar 7, 2013 at 11:42 AM, Tyler Odean <odean@google.com> wrote: > Yefim basically summed it up. The theory is that larger menu items have > made late items harder to reach / select. If users are selecting fewer late > items, the median index selected would go down. We're hoping for a null-op. > > T > > > On Thu, Mar 7, 2013 at 10:16 AM, <yefim@chromium.org> wrote: > >> +odean@ in case he want to add something >> >> The idea is if users fail to find and select menu items that have bigger >> indexes >> then distribution of indexes will shift to the left (to lower indexes). >> >> On 2013/03/07 01:15:17, sky wrote: >> >>> I'm still not seeing how this helps to prove anything one way or the >>> other. For one you're not logging if the index was visible or not. >>> Additionally even if an item was not visible how can you tell whether >>> the user intended to select it? >>> >> >> -Scott >>> >> >> On Wed, Mar 6, 2013 at 3:11 PM, <mailto:yefim@chromium.org> wrote: >>> > New menus take more vertical space. So Idea is to see if users can >>> find and >>> > select items that could potentially not be visible any more and require >>> > scrolling. Looking at distribution of indexes for all menus will show >>> if it >>> > changed compare to old menus. >>> > >>> > >>> > On 2013/03/06 22:34:21, sky wrote: >>> >> >>> >> I fail to see how the index is meaningful here. Remember this code is >>> hit >>> >> for >>> >> *all* menus. How is the index going to tell us anything useful? >>> > >>> > >>> > >>> > >>> >> >> https://codereview.chromium.**org/12549009/diff/2001/ui/** >> native_theme/native_theme.cc<https://codereview.chromium.org/12549009/diff/2001/ui/native_theme/native_theme.cc> >> >>> >> >>> >> File ui/native_theme/native_theme.**cc (right): >>> > >>> > >>> > >>> > >>> >> >> https://codereview.chromium.**org/12549009/diff/2001/ui/** >> native_theme/native_theme.cc#**newcode35<https://codereview.chromium.org/12549009/diff/2001/ui/native_theme/native_theme.cc#newcode35> >> >>> >> >>> >> ui/native_theme/native_theme.**cc:35: enable_new_menu_style = >>> >> Doesn't this mean we do the lookup each time? I think you need another >>> >> boolean >>> >> indicating if you've enable_new_menu_style has been set yet. >>> > >>> > >>> > >>> > >>> >> >> https://codereview.chromium.**org/12549009/diff/2001/ui/** >> views/controls/menu/menu_**runner.cc<https://codereview.chromium.org/12549009/diff/2001/ui/views/controls/menu/menu_runner.cc> >> >>> >> >>> >> File ui/views/controls/menu/menu_**runner.cc (right): >>> > >>> > >>> > >>> > >>> >> >> https://codereview.chromium.**org/12549009/diff/2001/ui/** >> views/controls/menu/menu_**runner.cc#newcode26<https://codereview.chromium.org/12549009/diff/2001/ui/views/controls/menu/menu_runner.cc#newcode26> >> >>> >> >>> >> ui/views/controls/menu/menu_**runner.cc:26: const MenuItemView* >>> parent = >>> >> menu_item->GetParentMenuItem()**; >>> >> menu_item could be NULL. >>> > >>> > >>> > >>> > >>> > >>> > https://codereview.chromium.**org/12549009/<https://codereview.chromium.org/1... >>> >> >> >> >> https://codereview.chromium.**org/12549009/<https://codereview.chromium.org/1... >> > > > > -- > Tyler Odean | Chrome | odean@google.com | 330-536-3326 >
But I suppose we can take that into account once we have some data. By that I mean presumably we'll be able to tell how the median changes for the two data sets day-to-day.
You may be right about the volatility, but we should be able to aggregate over days with Dremel as you said, so it's just a matter of gathering *enough* data to get a sense of the difference. That said, I'm totally open to any suggestions you have for less volatile ways to track this potential impact. T On Thu, Mar 7, 2013 at 1:03 PM, <sky@chromium.org> wrote: > But I suppose we can take that into account once we have some data. By > that I > mean presumably we'll be able to tell how the median changes for the two > data > sets day-to-day. > > https://codereview.chromium.**org/12549009/<https://codereview.chromium.org/1... > -- Tyler Odean | Chrome | odean@google.com | 330-536-3326
https://codereview.chromium.org/12549009/diff/5001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_runner.cc (right): https://codereview.chromium.org/12549009/diff/5001/ui/views/controls/menu/men... ui/views/controls/menu/menu_runner.cc:40: RecordSelectedIndexes(parent); Don't we only care about the index of the actual item?
The menus we design all fit on the screen. Think context menus and the wrench menu. Bookmark menus can be any size. My gutt tells me very few people actually create bookmark menus that result in clipping. Additionally just because I select a bookmark way off the screen one day does not necessarily mean I'm going to select it the next day, similarly the index I select in other menus is pretty random. I just don't see how the index is going to be very interesting. Of course I'm fine measuring it, I'm just skeptical the result is going to tell us anything one way or the other. The time open is a bit more interesting to study and I could see that changing if changed the size. -Scott On Thu, Mar 7, 2013 at 1:04 PM, Tyler Odean <odean@google.com> wrote: > You may be right about the volatility, but we should be able to aggregate > over days with Dremel as you said, so it's just a matter of gathering > *enough* data to get a sense of the difference. That said, I'm totally open > to any suggestions you have for less volatile ways to track this potential > impact. > > T > > > On Thu, Mar 7, 2013 at 1:03 PM, <sky@chromium.org> wrote: > >> But I suppose we can take that into account once we have some data. By >> that I >> mean presumably we'll be able to tell how the median changes for the two >> data >> sets day-to-day. >> >> https://codereview.chromium.**org/12549009/<https://codereview.chromium.org/1... >> > > > > -- > Tyler Odean | Chrome | odean@google.com | 330-536-3326 >
https://codereview.chromium.org/12549009/diff/5001/ui/views/controls/menu/men... File ui/views/controls/menu/menu_runner.cc (right): https://codereview.chromium.org/12549009/diff/5001/ui/views/controls/menu/men... ui/views/controls/menu/menu_runner.cc:40: RecordSelectedIndexes(parent); For multi-level menus I think we need to report index for each level. If root menu has a submenu with index 10 and selected item from that submenu has index 1, then reporting only 1 will loose information that user was able to select submenu with index 10 on a top level. On 2013/03/07 21:05:00, sky wrote: > Don't we only care about the index of the actual item?
Doing this means items deeper in the menu are weighted less than those at the top level. That doesn't seem right. Also, I don't think this counts the non-menu items, such as copy/paste and the zoom buttons. -Scott On Thu, Mar 7, 2013 at 2:17 PM, <yefim@chromium.org> wrote: > > https://codereview.chromium.**org/12549009/diff/5001/ui/** > views/controls/menu/menu_**runner.cc<https://codereview.chromium.org/12549009/diff/5001/ui/views/controls/menu/menu_runner.cc> > File ui/views/controls/menu/menu_**runner.cc (right): > > https://codereview.chromium.**org/12549009/diff/5001/ui/** > views/controls/menu/menu_**runner.cc#newcode40<https://codereview.chromium.org/12549009/diff/5001/ui/views/controls/menu/menu_runner.cc#newcode40> > ui/views/controls/menu/menu_**runner.cc:40: RecordSelectedIndexes(parent); > For multi-level menus I think we need to report index for each level. > If root menu has a submenu with index 10 and selected item from that > submenu has index 1, then reporting only 1 will loose information that > user was able to select submenu with index 10 on a top level. > > > On 2013/03/07 21:05:00, sky wrote: > >> Don't we only care about the index of the actual item? >> > > https://codereview.chromium.**org/12549009/<https://codereview.chromium.org/1... >
first of all we are going to compare numbers to a control group that uses same reporting. second to select item in a submenu user need to select submenu, so it is not much different from any other selection. And yes, copy/paste/zoom are not counted in both test and control groups. On 2013/03/08 00:33:39, sky wrote: > Doing this means items deeper in the menu are weighted less than those at > the top level. That doesn't seem right. Also, I don't think this counts the > non-menu items, such as copy/paste and the zoom buttons. > > -Scott > > > On Thu, Mar 7, 2013 at 2:17 PM, <mailto:yefim@chromium.org> wrote: > > > > > https://codereview.chromium.**org/12549009/diff/5001/ui/** > > > views/controls/menu/menu_**runner.cc<https://codereview.chromium.org/12549009/diff/5001/ui/views/controls/menu/menu_runner.cc> > > File ui/views/controls/menu/menu_**runner.cc (right): > > > > https://codereview.chromium.**org/12549009/diff/5001/ui/** > > > views/controls/menu/menu_**runner.cc#newcode40<https://codereview.chromium.org/12549009/diff/5001/ui/views/controls/menu/menu_runner.cc#newcode40> > > ui/views/controls/menu/menu_**runner.cc:40: RecordSelectedIndexes(parent); > > For multi-level menus I think we need to report index for each level. > > If root menu has a submenu with index 10 and selected item from that > > submenu has index 1, then reporting only 1 will loose information that > > user was able to select submenu with index 10 on a top level. > > > > > > On 2013/03/07 21:05:00, sky wrote: > > > >> Don't we only care about the index of the actual item? > >> > > > > > https://codereview.chromium.**org/12549009/%3Chttps://codereview.chromium.org...> > >
All this metric is showing is whether users are selecting items. It's not showing us if they can't select items, which seems to be what you are interested in knowing. Wouldn't a better metric be how often they bring up the menu and do *not* select anything? -Scott On Thu, Mar 7, 2013 at 5:56 PM, <yefim@chromium.org> wrote: > first of all we are going to compare numbers to a control group that uses > same > reporting. > second to select item in a submenu user need to select submenu, so it is > not > much different from any other selection. > And yes, copy/paste/zoom are not counted in both test and control groups. > > > On 2013/03/08 00:33:39, sky wrote: > >> Doing this means items deeper in the menu are weighted less than those at >> the top level. That doesn't seem right. Also, I don't think this counts >> the >> non-menu items, such as copy/paste and the zoom buttons. >> > > -Scott >> > > > On Thu, Mar 7, 2013 at 2:17 PM, <mailto:yefim@chromium.org> wrote: >> > > > >> > https://codereview.chromium.****org/12549009/diff/5001/ui/** >> > >> > > views/controls/menu/menu_****runner.cc<https://codereview.** > chromium.org/12549009/diff/**5001/ui/views/controls/menu/**menu_runner.cc<https://codereview.chromium.org/12549009/diff/5001/ui/views/controls/menu/menu_runner.cc> > > > >> > File ui/views/controls/menu/menu_****runner.cc (right): >> > >> > https://codereview.chromium.****org/12549009/diff/5001/ui/** >> > >> > > views/controls/menu/menu_****runner.cc#newcode40<https://** > codereview.chromium.org/**12549009/diff/5001/ui/views/** > controls/menu/menu_runner.cc#**newcode40<https://codereview.chromium.org/12549009/diff/5001/ui/views/controls/menu/menu_runner.cc#newcode40> > > > >> > ui/views/controls/menu/menu_****runner.cc:40: >> RecordSelectedIndexes(parent); >> >> > For multi-level menus I think we need to report index for each level. >> > If root menu has a submenu with index 10 and selected item from that >> > submenu has index 1, then reporting only 1 will loose information that >> > user was able to select submenu with index 10 on a top level. >> > >> > >> > On 2013/03/07 21:05:00, sky wrote: >> > >> >> Don't we only care about the index of the actual item? >> >> >> > >> > >> > > https://codereview.chromium.****org/12549009/%3Chttps://codere** > view.chromium.org/12549009/ <http://codereview.chromium.org/12549009/>> > >> > >> > > > > https://codereview.chromium.**org/12549009/<https://codereview.chromium.org/1... >
There were two proposed metrics: one tracking menus opened and one tracking index of items selected. The plan was for total(A) - total(B) to equal total menus opened with nothing clicked. Cheers, T On Fri, Mar 8, 2013 at 7:39 AM, Scott Violet <sky@chromium.org> wrote: > All this metric is showing is whether users are selecting items. It's not > showing us if they can't select items, which seems to be what you are > interested in knowing. Wouldn't a better metric be how often they bring up > the menu and do *not* select anything? > > -Scott > > > On Thu, Mar 7, 2013 at 5:56 PM, <yefim@chromium.org> wrote: > >> first of all we are going to compare numbers to a control group that uses >> same >> reporting. >> second to select item in a submenu user need to select submenu, so it is >> not >> much different from any other selection. >> And yes, copy/paste/zoom are not counted in both test and control groups. >> >> >> On 2013/03/08 00:33:39, sky wrote: >> >>> Doing this means items deeper in the menu are weighted less than those at >>> the top level. That doesn't seem right. Also, I don't think this counts >>> the >>> non-menu items, such as copy/paste and the zoom buttons. >>> >> >> -Scott >>> >> >> >> On Thu, Mar 7, 2013 at 2:17 PM, <mailto:yefim@chromium.org> wrote: >>> >> >> > >>> > https://codereview.chromium.****org/12549009/diff/5001/ui/** >>> > >>> >> >> views/controls/menu/menu_****runner.cc<https://codereview.** >> chromium.org/12549009/diff/**5001/ui/views/controls/menu/**menu_runner.cc<https://codereview.chromium.org/12549009/diff/5001/ui/views/controls/menu/menu_runner.cc> >> > >> >>> > File ui/views/controls/menu/menu_****runner.cc (right): >>> > >>> > https://codereview.chromium.****org/12549009/diff/5001/ui/** >>> > >>> >> >> views/controls/menu/menu_****runner.cc#newcode40<https://** >> codereview.chromium.org/**12549009/diff/5001/ui/views/** >> controls/menu/menu_runner.cc#**newcode40<https://codereview.chromium.org/12549009/diff/5001/ui/views/controls/menu/menu_runner.cc#newcode40> >> > >> >>> > ui/views/controls/menu/menu_****runner.cc:40: >>> RecordSelectedIndexes(parent); >>> >>> > For multi-level menus I think we need to report index for each level. >>> > If root menu has a submenu with index 10 and selected item from that >>> > submenu has index 1, then reporting only 1 will loose information that >>> > user was able to select submenu with index 10 on a top level. >>> > >>> > >>> > On 2013/03/07 21:05:00, sky wrote: >>> > >>> >> Don't we only care about the index of the actual item? >>> >> >>> > >>> > >>> >> >> https://codereview.chromium.****org/12549009/%3Chttps://codere** >> view.chromium.org/12549009/ <http://codereview.chromium.org/12549009/>> >> >>> > >>> >> >> >> >> https://codereview.chromium.**org/12549009/<https://codereview.chromium.org/1... >> > > -- Tyler Odean | Chrome | odean@google.com | 330-536-3326
We do have metric to track if user selected item or not ("MenuSelection").
On 2013/03/08 15:39:52, sky wrote:
> All this metric is showing is whether users are selecting items. It's not
> showing us if they can't select items, which seems to be what you are
> interested in knowing. Wouldn't a better metric be how often they bring up
> the menu and do *not* select anything?
>
> -Scott
>
>
> On Thu, Mar 7, 2013 at 5:56 PM, <mailto:yefim@chromium.org> wrote:
>
> > first of all we are going to compare numbers to a control group that uses
> > same
> > reporting.
> > second to select item in a submenu user need to select submenu, so it is
> > not
> > much different from any other selection.
> > And yes, copy/paste/zoom are not counted in both test and control groups.
> >
> >
> > On 2013/03/08 00:33:39, sky wrote:
> >
> >> Doing this means items deeper in the menu are weighted less than those at
> >> the top level. That doesn't seem right. Also, I don't think this counts
> >> the
> >> non-menu items, such as copy/paste and the zoom buttons.
> >>
> >
> > -Scott
> >>
> >
> >
> > On Thu, Mar 7, 2013 at 2:17 PM, <mailto:yefim@chromium.org> wrote:
> >>
> >
> > >
> >> > https://codereview.chromium.****org/12549009/diff/5001/ui/**
> >> >
> >>
> >
> > views/controls/menu/menu_****runner.cc<https://codereview.**
> >
>
chromium.org/12549009/diff/**5001/ui/views/controls/menu/**menu_runner.cc<https://codereview.chromium.org/12549009/diff/5001/ui/views/controls/menu/menu_runner.cc>
> > >
> >
> >> > File ui/views/controls/menu/menu_****runner.cc (right):
> >> >
> >> > https://codereview.chromium.****org/12549009/diff/5001/ui/**
> >> >
> >>
> >
> > views/controls/menu/menu_****runner.cc#newcode40<https://**
> > codereview.chromium.org/**12549009/diff/5001/ui/views/**
> >
>
controls/menu/menu_runner.cc#**newcode40<https://codereview.chromium.org/12549009/diff/5001/ui/views/controls/menu/menu_runner.cc#newcode40>
> > >
> >
> >> > ui/views/controls/menu/menu_****runner.cc:40:
> >> RecordSelectedIndexes(parent);
> >>
> >> > For multi-level menus I think we need to report index for each level.
> >> > If root menu has a submenu with index 10 and selected item from that
> >> > submenu has index 1, then reporting only 1 will loose information that
> >> > user was able to select submenu with index 10 on a top level.
> >> >
> >> >
> >> > On 2013/03/07 21:05:00, sky wrote:
> >> >
> >> >> Don't we only care about the index of the actual item?
> >> >>
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.****org/12549009/%253Chttps://codere**
> > view.chromium.org/12549009/ <http://codereview.chromium.org/12549009/>>
> >
> >> >
> >>
> >
> >
> >
> >
>
https://codereview.chromium.**org/12549009/%3Chttps://codereview.chromium.org...>
> >
Ok, LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yefim@chromium.org/12549009/5001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
I'm also very curious once we get some data and how we interpret it. If you can ping me when we do that I would appreciate it. Thanks, -Scott On Fri, Mar 8, 2013 at 10:29 AM, <commit-bot@chromium.org> wrote: > Step "update" is always a major failure. > Look at the try server FAQ for more details. > http://build.chromium.org/p/**tryserver.chromium/** > buildstatus?builder=linux_**clang&number=89606<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clang&number=89606> > > https://chromiumcodereview.**appspot.com/12549009/<https://chromiumcodereview... >
Absolutely. :) T On Fri, Mar 8, 2013 at 10:45 AM, Scott Violet <sky@chromium.org> wrote: > I'm also very curious once we get some data and how we interpret it. If > you can ping me when we do that I would appreciate it. > > Thanks, > > -Scott > > > On Fri, Mar 8, 2013 at 10:29 AM, <commit-bot@chromium.org> wrote: > >> Step "update" is always a major failure. >> Look at the try server FAQ for more details. >> http://build.chromium.org/p/**tryserver.chromium/** >> buildstatus?builder=linux_**clang&number=89606<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clang&number=89606> >> >> https://chromiumcodereview.**appspot.com/12549009/<https://chromiumcodereview... >> > > -- Tyler Odean | Chrome | odean@google.com | 330-536-3326
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yefim@chromium.org/12549009/33001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yefim@chromium.org/12549009/33001
Message was sent while issue was closed.
Change committed as 187057 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
