Add UMA metrics to measure the effectiveness of views fuzzing

Add UMA metrics to measure the effectiveness of views fuzzing

Add new buckets to the Ash.GestureTarget histogram which
will be useful in measuring the effectiveness of views 
fuzzing when it is implemented. The idea is to count taps 
made on the different element types located on/near the 
tab strip. A description of the new types added to
GestureActionType in touch_uma.cc:

GESTURE_TABSWITCH_TAP: A tap on a currently unselected
tab (i.e., the tap was responsible for selecting the tab).

GESTURE_TABNOSWITCH_TAP: A tap on a currently selected
tab (i.e, the tap had no visible effect).

GESTURE_TABCLOSE_TAP: A tap on a tab's close button.

GESTURE_NEWTAB_TAP: A tap on the new tab button.

GESTURE_FRAMEMAXIMIZE_TAP: A tap on the frame maximize
button.

GESTURE_FRAMEVIEW_TAP: A tap on the space to the right of
the tabstrip, left of the tabstrip, or in between tabs.
Such a tap has no visible effect.

GESTURE_ROOTVIEWTOP_TAP: A tap on the top edge of a browser
window. Such a tap has no visible effect.

GESTURE_MAXIMIZE_DOUBLETAP: A double-tap on the browser
window resulting in a maximize or restore action.

BUG=248230
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209263

[tdanderson, 2013-06-12 20:04:23]

@sadrul, would you mind having a first look over this WIP patch and my comments?

https://codereview.chromium.org/16832003/diff/1/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/16832003/diff/1/ash/touch/touch_uma.cc#newcode10
ash/touch/touch_uma.cc:10: //#include "chrome/browser/ui/views/tabs/tab.h"
Presubmit tells me that this is an illegal include. So I am changing this so
that I instead have UMA_HISTOGRAM_ENUMERATION(...) in tab_strip.cc in order to
track GESTURE_TABSWITCH_TAP and GESTURE_TABNOSWITCH_TAP.

https://codereview.chromium.org/16832003/diff/1/ash/touch/touch_uma.cc#newcode48
ash/touch/touch_uma.cc:48: // the enum list in tools/histogram/histograms.xml is
updated with any change in
There is no mention of the Ash.GestureTarget histogram in histograms.xml, so I
am unsure where I should be adding the names and descriptions of these new
types.

[tdanderson, 2013-06-13 22:46:25]

@sadrul, Patch Set 2 fixes the illegal include issue.

https://codereview.chromium.org/16832003/diff/1/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/16832003/diff/1/ash/touch/touch_uma.cc#newcode10
ash/touch/touch_uma.cc:10: //#include "chrome/browser/ui/views/tabs/tab.h"
On 2013/06/12 20:04:24, tdanderson wrote:
> Presubmit tells me that this is an illegal include. So I am changing this so
> that I instead have UMA_HISTOGRAM_ENUMERATION(...) in tab_strip.cc in order to
> track GESTURE_TABSWITCH_TAP and GESTURE_TABNOSWITCH_TAP.

Done.

https://codereview.chromium.org/16832003/diff/1/ash/touch/touch_uma.cc#newcode48
ash/touch/touch_uma.cc:48: // the enum list in tools/histogram/histograms.xml is
updated with any change in
On 2013/06/12 20:04:24, tdanderson wrote:
> There is no mention of the Ash.GestureTarget histogram in histograms.xml, so I
> am unsure where I should be adding the names and descriptions of these new
> types.

Done.

[sadrul, 2013-06-14 00:20:25]

https://codereview.chromium.org/16832003/diff/4001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/16832003/diff/4001/ash/touch/touch_uma.cc#new...
ash/touch/touch_uma.cc:417: const char kRootView[] =
"browser/ui/views/frame/BrowserRootView";
Rename this to BrowserRootView

https://codereview.chromium.org/16832003/diff/4001/ash/touch/touch_uma.cc#new...
ash/touch/touch_uma.cc:428: return GESTURE_ROOTVIEWTOP_TAP;
I think this should just be determined by the BrowserRootView, i.e. the
BRV::OnGestureEvent should call into TouchUMA::RecordGestureAction when
appropriate.

https://codereview.chromium.org/16832003/diff/4001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/16832003/diff/4001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab.cc:1042: if (!IsSelected())
Should you just record the UMA here? Even if the gesture doesn't end up being a
TAP (e.g. long-tap, or drag out the tab), it will still have selected the tab.

https://codereview.chromium.org/16832003/diff/4001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/16832003/diff/4001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_strip.cc:311: virtual const char*
GetClassName() const OVERRIDE;
Move this with the overriden method block below.

https://codereview.chromium.org/16832003/diff/4001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_strip.cc:1605: TouchUMA::GESTURE_ACTION_COUNT);
Hm. Instead of this, how about:
 * Make TouchUMA a singleton.
 * Remove the SystemGestureEventFilter::touch_uma_. Instead, call into the
singleton from the OnTouch/GestureEvent handlers.
 * Introduce TouchUMA::RecordGestureAction(GestureAction) to the public api for
TouchUMA.
 * Call into TouchUMA::RecordGestureAction(GESTURE_TABSWITCH_TAP) from here.

[tdanderson, 2013-06-14 19:56:52]

@sadrul, can you please have a look at my latest patch set and questions when
you have a moment?

https://codereview.chromium.org/16832003/diff/4001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/16832003/diff/4001/ash/touch/touch_uma.cc#new...
ash/touch/touch_uma.cc:417: const char kRootView[] =
"browser/ui/views/frame/BrowserRootView";
On 2013/06/14 00:20:25, sadrul wrote:
> Rename this to BrowserRootView

Done.

https://codereview.chromium.org/16832003/diff/4001/ash/touch/touch_uma.cc#new...
ash/touch/touch_uma.cc:428: return GESTURE_ROOTVIEWTOP_TAP;
On 2013/06/14 00:20:25, sadrul wrote:
> I think this should just be determined by the BrowserRootView, i.e. the
> BRV::OnGestureEvent should call into TouchUMA::RecordGestureAction when
> appropriate.

There is no BrowserRootView::OnGestureEvent(). I tried adding one as an
override, but it never gets called. The only place I can find to do this is in
ToplevelWindowEventHandler::OnGestureEvent(). Does that sound right?

https://codereview.chromium.org/16832003/diff/4001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/16832003/diff/4001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab.cc:1042: if (!IsSelected())
On 2013/06/14 00:20:25, sadrul wrote:
> Should you just record the UMA here? Even if the gesture doesn't end up being
a
> TAP (e.g. long-tap, or drag out the tab), it will still have selected the tab.

I don't want to record the UMA here, since I don't care about every time a tab
was selected (i.e, if the tabs are being reordered using touch). I only care
about whether or not a tab was selected using a tap to be able to distinguish
between "taps that did something" and "taps that didn't do anything."

https://codereview.chromium.org/16832003/diff/4001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/16832003/diff/4001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_strip.cc:311: virtual const char*
GetClassName() const OVERRIDE;
On 2013/06/14 00:20:25, sadrul wrote:
> Move this with the overriden method block below.

Done.

https://codereview.chromium.org/16832003/diff/4001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab_strip.cc:1605: TouchUMA::GESTURE_ACTION_COUNT);
On 2013/06/14 00:20:25, sadrul wrote:
> Hm. Instead of this, how about:
>  * Make TouchUMA a singleton.
>  * Remove the SystemGestureEventFilter::touch_uma_. Instead, call into the
> singleton from the OnTouch/GestureEvent handlers.
>  * Introduce TouchUMA::RecordGestureAction(GestureAction) to the public api
for
> TouchUMA.
>  * Call into TouchUMA::RecordGestureAction(GESTURE_TABSWITCH_TAP) from here.

Done.

https://codereview.chromium.org/16832003/diff/14001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/16832003/diff/14001/ash/touch/touch_uma.cc#ne...
ash/touch/touch_uma.cc:202: void TouchUMA::DeleteInstance() {
I am not sure where and when this should be called. Can you please advise?

https://codereview.chromium.org/16832003/diff/14001/ash/touch/touch_uma.cc#ne...
ash/touch/touch_uma.cc:436: const char kRootView[] = "BrowserRootView";
So to clarify, is your vision for me to be moving as many of these cases out of
here and instead tracking them by calling TouchUMA::RecordGestureAction() at the
appropriate locations elsewhere in the code?

https://codereview.chromium.org/16832003/diff/14001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_event_handler.cc (right):

https://codereview.chromium.org/16832003/diff/14001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_event_handler.cc:129:
TouchUMA::GESTURE_MAXIMIZE_DOUBLETAP);
I forgot to add this earlier, but I'd also like to track double-tap to
maximize/restore. It looks like this is already getting recorded in another
histogram, though (line 126). Is this sort of duplication frowned upon?
Basically, I'd like to have all fuzzing-related metrics in one histogram if at
all possible.

https://codereview.chromium.org/16832003/diff/14001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/16832003/diff/14001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab_strip.cc:1603:
TouchUMA::GetInstance()->RecordGestureAction(action);
Please ignore the trybot compilation problems for the moment, I am in the
process of fixing.

[sadrul, 2013-06-16 20:56:17]

Responded to comments in patchsets 2, 3, 4.

https://codereview.chromium.org/16832003/diff/4001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/16832003/diff/4001/ash/touch/touch_uma.cc#new...
ash/touch/touch_uma.cc:428: return GESTURE_ROOTVIEWTOP_TAP;
On 2013/06/14 19:56:53, tdanderson wrote:
> On 2013/06/14 00:20:25, sadrul wrote:
> > I think this should just be determined by the BrowserRootView, i.e. the
> > BRV::OnGestureEvent should call into TouchUMA::RecordGestureAction when
> > appropriate.
> 
> There is no BrowserRootView::OnGestureEvent(). I tried adding one as an
> override, but it never gets called. The only place I can find to do this is in
> ToplevelWindowEventHandler::OnGestureEvent(). Does that sound right?
> 

Try overriding RootView::DispatchGestureEvent() in BrowserView for this.

https://codereview.chromium.org/16832003/diff/4001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/16832003/diff/4001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab.cc:1042: if (!IsSelected())
On 2013/06/14 19:56:53, tdanderson wrote:
> On 2013/06/14 00:20:25, sadrul wrote:
> > Should you just record the UMA here? Even if the gesture doesn't end up
being
> a
> > TAP (e.g. long-tap, or drag out the tab), it will still have selected the
tab.
> 
> I don't want to record the UMA here, since I don't care about every time a tab
> was selected (i.e, if the tabs are being reordered using touch). I only care
> about whether or not a tab was selected using a tap to be able to distinguish
> between "taps that did something" and "taps that didn't do anything."

In that case, can you count this from here when the Tab receives a
ET_GESTURE_TAP (instead of exposing this internal state
|tab_selected_with_last_gesture_begin_| via a public api)?

https://codereview.chromium.org/16832003/diff/14001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/16832003/diff/14001/ash/touch/touch_uma.cc#ne...
ash/touch/touch_uma.cc:202: void TouchUMA::DeleteInstance() {
On 2013/06/14 19:56:53, tdanderson wrote:
> I am not sure where and when this should be called. Can you please advise?

You don't need this. Just use base::Singleton<>

https://codereview.chromium.org/16832003/diff/14001/ash/touch/touch_uma.cc#ne...
ash/touch/touch_uma.cc:436: const char kRootView[] = "BrowserRootView";
On 2013/06/14 19:56:53, tdanderson wrote:
> So to clarify, is your vision for me to be moving as many of these cases out
of
> here and instead tracking them by calling TouchUMA::RecordGestureAction() at
the
> appropriate locations elsewhere in the code?

My plan was: if you can make the decision of what the action is here (e.g. by
matching the name of the view), then do this here, from RecordGestureEvent.

If it needs more work, e.g. 'did the tap actually switch the tab', 'did the tap
happen on the frame-view above the tabstrip, instead of on its side', then make
use RecordGestureAction from those places.

To make it clear, an option is to keep the enums used in RecordGestureEvent()
private (i.e. define them inside here), and expose the rest of the enums in the
header for the public API.

https://codereview.chromium.org/16832003/diff/14001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_event_handler.cc (right):

https://codereview.chromium.org/16832003/diff/14001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_event_handler.cc:129:
TouchUMA::GESTURE_MAXIMIZE_DOUBLETAP);
On 2013/06/14 19:56:53, tdanderson wrote:
> I forgot to add this earlier, but I'd also like to track double-tap to
> maximize/restore. It looks like this is already getting recorded in another
> histogram, though (line 126). Is this sort of duplication frowned upon?
> Basically, I'd like to have all fuzzing-related metrics in one histogram if at
> all possible.

I think adding this to the histogram is OK.

https://codereview.chromium.org/16832003/diff/12002/ash/touch/touch_uma.h
File ash/touch/touch_uma.h (right):

https://codereview.chromium.org/16832003/diff/12002/ash/touch/touch_uma.h#new...
ash/touch/touch_uma.h:70: static TouchUMA* instance_;
Use base::Singleton<>

[tdanderson, 2013-06-17 21:40:56]

@sadrul, thanks for the feedback. I believe I have addressed all of your
comments in my latest patch set.

Also, what sort of testing do you expect for this CL?

https://codereview.chromium.org/16832003/diff/4001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/16832003/diff/4001/ash/touch/touch_uma.cc#new...
ash/touch/touch_uma.cc:428: return GESTURE_ROOTVIEWTOP_TAP;
On 2013/06/16 20:56:17, sadrul wrote:
> On 2013/06/14 19:56:53, tdanderson wrote:
> > On 2013/06/14 00:20:25, sadrul wrote:
> > > I think this should just be determined by the BrowserRootView, i.e. the
> > > BRV::OnGestureEvent should call into TouchUMA::RecordGestureAction when
> > > appropriate.
> > 
> > There is no BrowserRootView::OnGestureEvent(). I tried adding one as an
> > override, but it never gets called. The only place I can find to do this is
in
> > ToplevelWindowEventHandler::OnGestureEvent(). Does that sound right?
> > 
> 
> Try overriding RootView::DispatchGestureEvent() in BrowserView for this.

Done.

https://codereview.chromium.org/16832003/diff/4001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/16832003/diff/4001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab.cc:1042: if (!IsSelected())
On 2013/06/16 20:56:17, sadrul wrote:
> On 2013/06/14 19:56:53, tdanderson wrote:
> > On 2013/06/14 00:20:25, sadrul wrote:
> > > Should you just record the UMA here? Even if the gesture doesn't end up
> being
> > a
> > > TAP (e.g. long-tap, or drag out the tab), it will still have selected the
> tab.
> > 
> > I don't want to record the UMA here, since I don't care about every time a
tab
> > was selected (i.e, if the tabs are being reordered using touch). I only care
> > about whether or not a tab was selected using a tap to be able to
distinguish
> > between "taps that did something" and "taps that didn't do anything."
> 
> In that case, can you count this from here when the Tab receives a
> ET_GESTURE_TAP (instead of exposing this internal state
> |tab_selected_with_last_gesture_begin_| via a public api)?

Unfortunately I don't think so. The reason I introduced |tab_selected_...| is
because the sequence of events when a tab is tapped is:

1) Tab::OnGestureEvent() with ui::ET_GESTURE_BEGIN
2) TabStrip::OnGestureEvent() with ui::ET_GESTURE_TAP_DOWN
3) TabStrip::OnGestureEvent() with ui::ET_GESTURE_TAP
4) TabStrip::OnGestureEvent() with ui::ET_GESTURE_END

https://codereview.chromium.org/16832003/diff/14001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/16832003/diff/14001/ash/touch/touch_uma.cc#ne...
ash/touch/touch_uma.cc:202: void TouchUMA::DeleteInstance() {
On 2013/06/16 20:56:17, sadrul wrote:
> On 2013/06/14 19:56:53, tdanderson wrote:
> > I am not sure where and when this should be called. Can you please advise?
> 
> You don't need this. Just use base::Singleton<>

Done.

https://codereview.chromium.org/16832003/diff/14001/ash/touch/touch_uma.cc#ne...
ash/touch/touch_uma.cc:436: const char kRootView[] = "BrowserRootView";
On 2013/06/16 20:56:17, sadrul wrote:
> On 2013/06/14 19:56:53, tdanderson wrote:
> > So to clarify, is your vision for me to be moving as many of these cases out
> of
> > here and instead tracking them by calling TouchUMA::RecordGestureAction() at
> the
> > appropriate locations elsewhere in the code?
> 
> My plan was: if you can make the decision of what the action is here (e.g. by
> matching the name of the view), then do this here, from RecordGestureEvent.
> 
> If it needs more work, e.g. 'did the tap actually switch the tab', 'did the
tap
> happen on the frame-view above the tabstrip, instead of on its side', then
make
> use RecordGestureAction from those places.

OK, makes sense.

> To make it clear, an option is to keep the enums used in RecordGestureEvent()
> private (i.e. define them inside here), and expose the rest of the enums in
the
> header for the public API.

Wouldn't this mess up the numerical ordering for the histogram / make the
ordering more error-prone due to the need to manually define a few of the
corresponding values within each enum?

https://codereview.chromium.org/16832003/diff/14001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_event_handler.cc (right):

https://codereview.chromium.org/16832003/diff/14001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_event_handler.cc:129:
TouchUMA::GESTURE_MAXIMIZE_DOUBLETAP);
On 2013/06/16 20:56:17, sadrul wrote:
> On 2013/06/14 19:56:53, tdanderson wrote:
> > I forgot to add this earlier, but I'd also like to track double-tap to
> > maximize/restore. It looks like this is already getting recorded in another
> > histogram, though (line 126). Is this sort of duplication frowned upon?
> > Basically, I'd like to have all fuzzing-related metrics in one histogram if
at
> > all possible.
> 
> I think adding this to the histogram is OK.

OK, thanks.

https://codereview.chromium.org/16832003/diff/12002/ash/touch/touch_uma.h
File ash/touch/touch_uma.h (right):

https://codereview.chromium.org/16832003/diff/12002/ash/touch/touch_uma.h#new...
ash/touch/touch_uma.h:70: static TouchUMA* instance_;
On 2013/06/16 20:56:17, sadrul wrote:
> Use base::Singleton<>

Done.

https://codereview.chromium.org/16832003/diff/38001/ash/touch/touch_uma.h
File ash/touch/touch_uma.h (right):

https://codereview.chromium.org/16832003/diff/38001/ash/touch/touch_uma.h#new...
ash/touch/touch_uma.h:65: private:
Other singleton classes in the code base have:

private:
  friend struct DefaultSingletonTraits<X>;

I'm assuming I will need this too? (Perhaps clang would have told me, but I am
still in the process of getting that to work on my local machine.)

https://codereview.chromium.org/16832003/diff/38001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_root_view.cc (right):

https://codereview.chromium.org/16832003/diff/38001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_root_view.cc:133:
TouchUMA::GESTURE_ROOTVIEWTOP_TAP);
Why was this override necessary? Couldn't I have just done this work at the top
of RootView::DispatchGestureEvent() instead?

https://codereview.chromium.org/16832003/diff/38001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_root_view.h (right):

https://codereview.chromium.org/16832003/diff/38001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_root_view.h:42: #if defined(USE_ASH)
Which is better here: to encapsulate the whole function in #if defined(USE_ASH)
or just the relevant parts of the function body? I opted for the first way since
it is less code if USE_ASH is not defined.

[sadrul, 2013-06-18 04:02:40]

> Also, what sort of testing do you expect for this CL?

I guess the gnarly change in tab/tabstrip could use a test to make sure the bit
gets set correctly.

Note that I have responded to a couple of comments in patchsets 2 and 3.

https://codereview.chromium.org/16832003/diff/4001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/16832003/diff/4001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab.cc:1042: if (!IsSelected())
On 2013/06/17 21:40:56, tdanderson wrote:
> On 2013/06/16 20:56:17, sadrul wrote:
> > On 2013/06/14 19:56:53, tdanderson wrote:
> > > On 2013/06/14 00:20:25, sadrul wrote:
> > > > Should you just record the UMA here? Even if the gesture doesn't end up
> > being
> > > a
> > > > TAP (e.g. long-tap, or drag out the tab), it will still have selected
the
> > tab.
> > > 
> > > I don't want to record the UMA here, since I don't care about every time a
> tab
> > > was selected (i.e, if the tabs are being reordered using touch). I only
care
> > > about whether or not a tab was selected using a tap to be able to
> distinguish
> > > between "taps that did something" and "taps that didn't do anything."
> > 
> > In that case, can you count this from here when the Tab receives a
> > ET_GESTURE_TAP (instead of exposing this internal state
> > |tab_selected_with_last_gesture_begin_| via a public api)?
> 
> Unfortunately I don't think so. The reason I introduced |tab_selected_...| is
> because the sequence of events when a tab is tapped is:
> 
> 1) Tab::OnGestureEvent() with ui::ET_GESTURE_BEGIN
> 2) TabStrip::OnGestureEvent() with ui::ET_GESTURE_TAP_DOWN
> 3) TabStrip::OnGestureEvent() with ui::ET_GESTURE_TAP
> 4) TabStrip::OnGestureEvent() with ui::ET_GESTURE_END

Ahhh. Looks like on GESTURE_BEGIN on Tab, the TabStrip does a capture on itself
from MaybeStartDrag(). Gah.

GAH.

https://codereview.chromium.org/16832003/diff/14001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/16832003/diff/14001/ash/touch/touch_uma.cc#ne...
ash/touch/touch_uma.cc:436: const char kRootView[] = "BrowserRootView";
> > To make it clear, an option is to keep the enums used in
RecordGestureEvent()
> > private (i.e. define them inside here), and expose the rest of the enums in
> the
> > header for the public API.
> 
> Wouldn't this mess up the numerical ordering for the histogram / make the
> ordering more error-prone due to the need to manually define a few of the
> corresponding values within each enum?

Not necessarily. For example, in the public header:

enum GestureActionPublic {
  GESTURE_PUBLIC_START = 256,
  GESTURE_TAP_ABOVE_TABSTRIP,
  GESTURE_MAXIMIZE_DOUBLE_TAP,
  ...
  GESTURE_ACTION_COUNT
};

In the .cc file,

enum GestureAction {
  <the current ones>
};

So there's still room to grow the private list, without having to change any of
the existing public enums.

Although I am not sure if this is worth it.

https://codereview.chromium.org/16832003/diff/38001/ash/touch/touch_uma.h
File ash/touch/touch_uma.h (right):

https://codereview.chromium.org/16832003/diff/38001/ash/touch/touch_uma.h#new...
ash/touch/touch_uma.h:65: private:
On 2013/06/17 21:40:56, tdanderson wrote:
> Other singleton classes in the code base have:
> 
> private:
>   friend struct DefaultSingletonTraits<X>;
> 
> I'm assuming I will need this too? (Perhaps clang would have told me, but I am
> still in the process of getting that to work on my local machine.)

The constructor/destructors should be private for singletons (otherwise any code
can do a 'new TouchUMA()'). And that's when you will need this friend.

https://codereview.chromium.org/16832003/diff/38001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_root_view.cc (right):

https://codereview.chromium.org/16832003/diff/38001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_root_view.cc:128: if (event &&
Don't check for non-NULL |event| here. It should never be null. If it is, then
something else is broken, and we should know about it.

https://codereview.chromium.org/16832003/diff/38001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_root_view.cc:130: event->location().y() <=
0 &&
Does this happen when user taps on the shadow of the window? Don't you want to
count when the tap goes between the tabstrip's top edge and the window's top
edge? Is location().y() < 0 in those cases?

https://codereview.chromium.org/16832003/diff/38001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_root_view.cc:133:
TouchUMA::GESTURE_ROOTVIEWTOP_TAP);
On 2013/06/17 21:40:56, tdanderson wrote:
> Why was this override necessary? Couldn't I have just done this work at the
top
> of RootView::DispatchGestureEvent() instead?

All views::Widgets have a views::RootView. So you would end up counting taps
towards the top of every widget. I assume you want to count the taps that happen
above the tabstrip? You probably want to check for tabstrip()->visible() too.

https://codereview.chromium.org/16832003/diff/38001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_root_view.h (right):

https://codereview.chromium.org/16832003/diff/38001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_root_view.h:42: #if defined(USE_ASH)
On 2013/06/17 21:40:56, tdanderson wrote:
> Which is better here: to encapsulate the whole function in #if
defined(USE_ASH)
> or just the relevant parts of the function body? I opted for the first way
since
> it is less code if USE_ASH is not defined.

I think this is OK

[tdanderson, 2013-06-20 18:39:44]

@sky, can you please have a look at this for OWNERS review? I am in the process
of trying to get my test to work; I have left comments in Patch Set 6 outlining
the problem I am experiencing. Please let me know if you have suggestions for
overcoming this problem or if there is a nicer way to write the test.

https://codereview.chromium.org/16832003/diff/4001/chrome/browser/ui/views/ta...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/16832003/diff/4001/chrome/browser/ui/views/ta...
chrome/browser/ui/views/tabs/tab.cc:1042: if (!IsSelected())
On 2013/06/18 04:02:40, sadrul wrote:
> On 2013/06/17 21:40:56, tdanderson wrote:
> > On 2013/06/16 20:56:17, sadrul wrote:
> > > On 2013/06/14 19:56:53, tdanderson wrote:
> > > > On 2013/06/14 00:20:25, sadrul wrote:
> > > > > Should you just record the UMA here? Even if the gesture doesn't end
up
> > > being
> > > > a
> > > > > TAP (e.g. long-tap, or drag out the tab), it will still have selected
> the
> > > tab.
> > > > 
> > > > I don't want to record the UMA here, since I don't care about every time
a
> > tab
> > > > was selected (i.e, if the tabs are being reordered using touch). I only
> care
> > > > about whether or not a tab was selected using a tap to be able to
> > distinguish
> > > > between "taps that did something" and "taps that didn't do anything."
> > > 
> > > In that case, can you count this from here when the Tab receives a
> > > ET_GESTURE_TAP (instead of exposing this internal state
> > > |tab_selected_with_last_gesture_begin_| via a public api)?
> > 
> > Unfortunately I don't think so. The reason I introduced |tab_selected_...|
is
> > because the sequence of events when a tab is tapped is:
> > 
> > 1) Tab::OnGestureEvent() with ui::ET_GESTURE_BEGIN
> > 2) TabStrip::OnGestureEvent() with ui::ET_GESTURE_TAP_DOWN
> > 3) TabStrip::OnGestureEvent() with ui::ET_GESTURE_TAP
> > 4) TabStrip::OnGestureEvent() with ui::ET_GESTURE_END
> 
> Ahhh. Looks like on GESTURE_BEGIN on Tab, the TabStrip does a capture on
itself
> from MaybeStartDrag(). Gah.
> 
> GAH.

I know, it's a bit inconvenient. But in the future if we no longer need this UMA
stat then I can remove the workaround.

https://codereview.chromium.org/16832003/diff/14001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/16832003/diff/14001/ash/touch/touch_uma.cc#ne...
ash/touch/touch_uma.cc:436: const char kRootView[] = "BrowserRootView";
On 2013/06/18 04:02:40, sadrul wrote:
> > > To make it clear, an option is to keep the enums used in
> RecordGestureEvent()
> > > private (i.e. define them inside here), and expose the rest of the enums
in
> > the
> > > header for the public API.
> > 
> > Wouldn't this mess up the numerical ordering for the histogram / make the
> > ordering more error-prone due to the need to manually define a few of the
> > corresponding values within each enum?
> 
> Not necessarily. For example, in the public header:
> 
> enum GestureActionPublic {
>   GESTURE_PUBLIC_START = 256,
>   GESTURE_TAP_ABOVE_TABSTRIP,
>   GESTURE_MAXIMIZE_DOUBLE_TAP,
>   ...
>   GESTURE_ACTION_COUNT
> };
> 
> In the .cc file,
> 
> enum GestureAction {
>   <the current ones>
> };
> 
> So there's still room to grow the private list, without having to change any
of
> the existing public enums.
> 
> Although I am not sure if this is worth it.

I had assumed that histograms.xml required the enum values to be consecutive,
but I just checked and that is not the case. However, I still think I'd rather
just leave this as is if that is OK with you.

https://codereview.chromium.org/16832003/diff/38001/ash/touch/touch_uma.h
File ash/touch/touch_uma.h (right):

https://codereview.chromium.org/16832003/diff/38001/ash/touch/touch_uma.h#new...
ash/touch/touch_uma.h:65: private:
On 2013/06/18 04:02:40, sadrul wrote:
> On 2013/06/17 21:40:56, tdanderson wrote:
> > Other singleton classes in the code base have:
> > 
> > private:
> >   friend struct DefaultSingletonTraits<X>;
> > 
> > I'm assuming I will need this too? (Perhaps clang would have told me, but I
am
> > still in the process of getting that to work on my local machine.)
> 
> The constructor/destructors should be private for singletons (otherwise any
code
> can do a 'new TouchUMA()'). And that's when you will need this friend.

Done.

https://codereview.chromium.org/16832003/diff/38001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_root_view.cc (right):

https://codereview.chromium.org/16832003/diff/38001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_root_view.cc:128: if (event &&
On 2013/06/18 04:02:40, sadrul wrote:
> Don't check for non-NULL |event| here. It should never be null. If it is, then
> something else is broken, and we should know about it.

Done.

https://codereview.chromium.org/16832003/diff/38001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_root_view.cc:130: event->location().y() <=
0 &&
On 2013/06/18 04:02:40, sadrul wrote:
> Does this happen when user taps on the shadow of the window? Don't you want to
> count when the tap goes between the tabstrip's top edge and the window's top
> edge? Is location().y() < 0 in those cases?

GESTURE_ROOTVIEWTOP_TAP represents a tap on the top edge/shadow of the window.
location().y() is <= 0 for this. 

Taps in between the tabstrip's top edge and the window's top edge (i.e.,
directly above the tabstrip) are counted with GESTURE_FRAMEVIEW_TAP. Taps to the
right of the new tab button but to the left of the maximize button are also
counted with this.

Perhaps my naming of the enum values is not entirely accurate since I am still
not 100% clear on all of the exact terminology for everything.

https://codereview.chromium.org/16832003/diff/38001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_root_view.cc:133:
TouchUMA::GESTURE_ROOTVIEWTOP_TAP);
On 2013/06/18 04:02:40, sadrul wrote:
> On 2013/06/17 21:40:56, tdanderson wrote:
> > Why was this override necessary? Couldn't I have just done this work at the
> top
> > of RootView::DispatchGestureEvent() instead?
> 
> All views::Widgets have a views::RootView. So you would end up counting taps
> towards the top of every widget. I assume you want to count the taps that
happen
> above the tabstrip? You probably want to check for tabstrip()->visible() too.

I'm not sure why I would need to check for tabstrip visibility here. I also
don't think I would need to check for it when recording
GESTURE_TAB[NO]SWITCH_TAP in TabStrip::OnGestureEvent(). Can you please explain
what you mean?

https://codereview.chromium.org/16832003/diff/38001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_root_view.h (right):

https://codereview.chromium.org/16832003/diff/38001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_root_view.h:42: #if defined(USE_ASH)
On 2013/06/18 04:02:40, sadrul wrote:
> On 2013/06/17 21:40:56, tdanderson wrote:
> > Which is better here: to encapsulate the whole function in #if
> defined(USE_ASH)
> > or just the relevant parts of the function body? I opted for the first way
> since
> > it is less code if USE_ASH is not defined.
> 
> I think this is OK

Done.

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab_strip_unittest.cc (right):

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:88: class
TabStripTestWithDragActions : public views::ViewsTestBase {
Extending ViewsTestBase is necessary for my test because I am creating a Widget,
which needs to have params set on it (see lines 400-401) and CreateParams() has
some Aura-specific code that needs to be invoked for this test to run correctly
in Aura.

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:94:
controller_->set_has_available_drag_actions(1);
Note I had to add this mutator in order for the capture to be set on the tab
strip within Tab::OnGestureEvent(ui::ET_GESTURE_BEGIN).

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:379:
TEST_F(TabStripTestWithDragActions, TabSelectedByTap) {
The purpose of this test is to make sure that the new member
|tab_selected_with_last_gesture_begin_| in Tab is set correctly. 

The name of this test should be "TabSelectedByGestureBegin".

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:415:
root->DispatchGestureEvent(&g1);
My test crashes here with a "Received signal 11 SEGV_MAPERR". The code path is:

DispatchGestureEvent(ET_GESTURE_BEGIN) => ...
Tab::OnGestureEvent() =>
TabStrip::MaybeStartDrag() =>
TabDragController::Init() =>
TabStripModel::AddObserver()

The crash is at the line observers_.AddObserver(observer) in
TabStripModel::AddObserver() and I am not sure why/what to do about this. I have
verified that |observer| is non-null. I have also tried calling
observers_.HasObserver(observer) but that line crashes too.

[sky, 2013-06-20 20:12:51]

What files do you need me to review?

[tdanderson, 2013-06-20 20:31:00]

On 2013/06/20 20:12:51, sky wrote:
> What files do you need me to review?

I have been iterating on this with sadrul@'s guidance, but
I do not believe he is an OWNER for any of these files,
so I would need you to have a look over all of them if
possible.

[sky, 2013-06-20 21:33:32]

Meta question. It's rather unusual to add metrics before we go with an
implementation. Is this because we're considering not doing the actual
implementation?

[tdanderson, 2013-06-20 22:07:54]

On 2013/06/20 21:33:32, sky wrote:
> Meta question. It's rather unusual to add metrics before we go with an
> implementation. Is this because we're considering not doing the actual
> implementation?

I am definitely still planning on implementing fuzzing for views, but I
am going to land it behind a flag. I wanted to have all of the UMA stuff
in place first so that when fuzzing lands I can apply for a finch
experiment in order to compare these metrics with/without fuzzing.
Plus, having this in place will help me with informal testing while
I am working on the fuzzing implementation.

[sky, 2013-06-21 16:21:38]

https://codereview.chromium.org/16832003/diff/50001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/16832003/diff/50001/ash/touch/touch_uma.cc#ne...
ash/touch/touch_uma.cc:413: const char kTabClose[] = "TabCloseButton";
UGH! How did we ever end up with this? This code is so fragile (who is going to
notice a name change broke this?). You need to centralize the names. Easiest way
I can think of is to move this code into chrome so that it can directly access
things like TabStrip::FooName.

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_root_view.cc (right):

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_root_view.cc:26: using
ash::internal::TouchUMA;
newline between 25/26.

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_root_view.cc:127: void
BrowserRootView::DispatchGestureEvent(ui::GestureEvent* event) {
Is there a way we could centralize this and only log for 'interesting' views?
Maybe Root could have something like this and forward it to the ViewsDelegate so
that ChromeViewsDelegate could then log for view names you care about?

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab.h (right):

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab.h:107: bool
WasTabSelectedWithLastGestureBegin() const {
style guide says trivial accessors like this should be unix_hacker_style, eg
tab_selected_with_last_gesture_begin();

Also, no point in all the ifdefs here.

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab_strip.cc:1600: gfx::Point local_point =
event->location();
const gfx::Point&, or FindTabForEvent(event->location())

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab_strip.cc:1604: if
(tab->WasTabSelectedWithLastGestureBegin())
Does a tab always have the same coordinates as the ui::ET_GESTURE_BEGIN?

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab_strip_unittest.cc (right):

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:379:
TEST_F(TabStripTestWithDragActions, TabSelectedByTap) {
On 2013/06/20 18:39:45, tdanderson wrote:
> The purpose of this test is to make sure that the new member
> |tab_selected_with_last_gesture_begin_| in Tab is set correctly. 
> 
> The name of this test should be "TabSelectedByGestureBegin".

How come you didn't name it as such then?

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:415:
root->DispatchGestureEvent(&g1);
On 2013/06/20 18:39:45, tdanderson wrote:
> My test crashes here with a "Received signal 11 SEGV_MAPERR". The code path
is:
> 
> DispatchGestureEvent(ET_GESTURE_BEGIN) => ...
> Tab::OnGestureEvent() =>
> TabStrip::MaybeStartDrag() =>
> TabDragController::Init() =>
> TabStripModel::AddObserver()
> 
> The crash is at the line observers_.AddObserver(observer) in
> TabStripModel::AddObserver() and I am not sure why/what to do about this. I
have
> verified that |observer| is non-null. I have also tried calling
> observers_.HasObserver(observer) but that line crashes too.

The tab dragging code needs a TabStripModel. The unit tests here do not create a
TabStripModel, hence you're getting NULL and crashing. You are likely to run
into lots of other issues trying to exercise the tab dragging code here since it
really isn't set up for unit tests.

[tdanderson, 2013-06-26 22:30:39]

@sky, I have uploaded patch set 7 to address your comments.

https://codereview.chromium.org/16832003/diff/50001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/16832003/diff/50001/ash/touch/touch_uma.cc#ne...
ash/touch/touch_uma.cc:413: const char kTabClose[] = "TabCloseButton";
On 2013/06/21 16:21:38, sky wrote:
> UGH! How did we ever end up with this? This code is so fragile (who is going
to
> notice a name change broke this?). You need to centralize the names. Easiest
way
> I can think of is to move this code into chrome so that it can directly access
> things like TabStrip::FooName.

In the next patch set I have removed all of the code and new names I added to
this function. Instead I call TouchUMA::RecordGestureAction() at the appropriate
places elsewhere in the code.

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_root_view.cc (right):

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_root_view.cc:26: using
ash::internal::TouchUMA;
On 2013/06/21 16:21:38, sky wrote:
> newline between 25/26.

Done.

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_root_view.cc:127: void
BrowserRootView::DispatchGestureEvent(ui::GestureEvent* event) {
On 2013/06/21 16:21:38, sky wrote:
> Is there a way we could centralize this and only log for 'interesting' views?
> Maybe Root could have something like this and forward it to the ViewsDelegate
so
> that ChromeViewsDelegate could then log for view names you care about?

I am not sure what you mean. The next patch set will not depend on hard-coded
views class names, but if this comment will still apply, please let me know.

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab.cc:1041: tab_selected_with_last_gesture_begin_
= !IsSelected();
Note I changed this to !IsActive().

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab.h (right):

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab.h:107: bool
WasTabSelectedWithLastGestureBegin() const {
On 2013/06/21 16:21:38, sky wrote:
> style guide says trivial accessors like this should be unix_hacker_style, eg
> tab_selected_with_last_gesture_begin();
> 
> Also, no point in all the ifdefs here.

Done. I have included the ifdefs elsewhere in the patch because I only want
these UMA stats to be recorded for ChromeOS devices.

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab_strip.cc:1600: gfx::Point local_point =
event->location();
On 2013/06/21 16:21:38, sky wrote:
> const gfx::Point&, or FindTabForEvent(event->location())

Removed.

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab_strip.cc:1604: if
(tab->WasTabSelectedWithLastGestureBegin())
On 2013/06/21 16:21:38, sky wrote:
> Does a tab always have the same coordinates as the ui::ET_GESTURE_BEGIN?

Actually no it doesn't, and thank you for bringing this up. The
ui::ET_GESTURE_BEGIN can target a different tab than the following
ui::ET_GESTURE_TAP. So in the next patch set I am instead looking at the
currently active tab.

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab_strip_unittest.cc (right):

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:379:
TEST_F(TabStripTestWithDragActions, TabSelectedByTap) {
On 2013/06/21 16:21:38, sky wrote:
> On 2013/06/20 18:39:45, tdanderson wrote:
> > The purpose of this test is to make sure that the new member
> > |tab_selected_with_last_gesture_begin_| in Tab is set correctly. 
> > 
> > The name of this test should be "TabSelectedByGestureBegin".
> 
> How come you didn't name it as such then?

I only realized the name should be changed after uploading this patch and
reviewing it.

https://codereview.chromium.org/16832003/diff/50001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:415:
root->DispatchGestureEvent(&g1);
On 2013/06/21 16:21:38, sky wrote:
> On 2013/06/20 18:39:45, tdanderson wrote:
> > My test crashes here with a "Received signal 11 SEGV_MAPERR". The code path
> is:
> > 
> > DispatchGestureEvent(ET_GESTURE_BEGIN) => ...
> > Tab::OnGestureEvent() =>
> > TabStrip::MaybeStartDrag() =>
> > TabDragController::Init() =>
> > TabStripModel::AddObserver()
> > 
> > The crash is at the line observers_.AddObserver(observer) in
> > TabStripModel::AddObserver() and I am not sure why/what to do about this. I
> have
> > verified that |observer| is non-null. I have also tried calling
> > observers_.HasObserver(observer) but that line crashes too.
> 
> The tab dragging code needs a TabStripModel. The unit tests here do not create
a
> TabStripModel, hence you're getting NULL and crashing. You are likely to run
> into lots of other issues trying to exercise the tab dragging code here since
it
> really isn't set up for unit tests.

I think it is reasonable to omit a test for this CL for the following reasons:

- All of the changes in this CL (including the new member variable in Tab) are
for the purpose of UMA data collection only. None of the changes will have an
effect on the browser.
- The changes are temporary until the usefulness of shipping with  views fuzzing
turned on has been determined.

[sky, 2013-06-26 23:42:55]

I don't like all the ifdefs this introduces. Instead could you have a file in
c/b/u/v/touch_uma that has the minimal enums you care about in chrome alone with
the single function RecordGestureAction(). touch_uma_ash would call through to
ash::TouchUMA and touch_uma.cc would have a no-op implementation.

https://codereview.chromium.org/16832003/diff/62001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/16832003/diff/62001/ash/touch/touch_uma.cc#ne...
ash/touch/touch_uma.cc:176: TouchUMA::TouchUMA()
Move this to match new position in header.

https://codereview.chromium.org/16832003/diff/62001/ash/touch/touch_uma.h
File ash/touch/touch_uma.h (right):

https://codereview.chromium.org/16832003/diff/62001/ash/touch/touch_uma.h#new...
ash/touch/touch_uma.h:20: namespace internal {
Since this class is used outside of chrome it shouldn't be in the internal
namespace.

https://codereview.chromium.org/16832003/diff/62001/ash/wm/workspace/frame_ma...
File ash/wm/workspace/frame_maximize_button.cc (right):

https://codereview.chromium.org/16832003/diff/62001/ash/wm/workspace/frame_ma...
ash/wm/workspace/frame_maximize_button.cc:286: #if defined(USE_ASH)
This is in ash, you shouldn't need the ifdef.

https://codereview.chromium.org/16832003/diff/62001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_root_view.cc (right):

https://codereview.chromium.org/16832003/diff/62001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_root_view.cc:133:
TouchUMA::GetInstance()->RecordGestureAction(
use {} since condition is multi-line.

https://codereview.chromium.org/16832003/diff/62001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_root_view.h (right):

https://codereview.chromium.org/16832003/diff/62001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_root_view.h:42: #if defined(USE_ASH)
Try to minimize ifdefs by nuking the ifdef here and instead ifdefing the parts
of the implementation that matter.

https://codereview.chromium.org/16832003/diff/62001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/16832003/diff/62001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab.cc:719: if (event.type() == ui::ET_GESTURE_TAP)
use {} since multi-line too.

[tdanderson, 2013-06-27 22:39:33]

@sky, is this what you had in mind?

https://codereview.chromium.org/16832003/diff/62001/ash/touch/touch_uma.cc
File ash/touch/touch_uma.cc (right):

https://codereview.chromium.org/16832003/diff/62001/ash/touch/touch_uma.cc#ne...
ash/touch/touch_uma.cc:176: TouchUMA::TouchUMA()
On 2013/06/26 23:42:55, sky wrote:
> Move this to match new position in header.

Done.

https://codereview.chromium.org/16832003/diff/62001/ash/touch/touch_uma.h
File ash/touch/touch_uma.h (right):

https://codereview.chromium.org/16832003/diff/62001/ash/touch/touch_uma.h#new...
ash/touch/touch_uma.h:20: namespace internal {
On 2013/06/26 23:42:55, sky wrote:
> Since this class is used outside of chrome it shouldn't be in the internal
> namespace.

Done.

https://codereview.chromium.org/16832003/diff/62001/ash/wm/workspace/frame_ma...
File ash/wm/workspace/frame_maximize_button.cc (right):

https://codereview.chromium.org/16832003/diff/62001/ash/wm/workspace/frame_ma...
ash/wm/workspace/frame_maximize_button.cc:286: #if defined(USE_ASH)
On 2013/06/26 23:42:55, sky wrote:
> This is in ash, you shouldn't need the ifdef.

Done.

https://codereview.chromium.org/16832003/diff/62001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_root_view.cc (right):

https://codereview.chromium.org/16832003/diff/62001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_root_view.cc:133:
TouchUMA::GetInstance()->RecordGestureAction(
On 2013/06/26 23:42:55, sky wrote:
> use {} since condition is multi-line.

Done.

https://codereview.chromium.org/16832003/diff/62001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_root_view.h (right):

https://codereview.chromium.org/16832003/diff/62001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_root_view.h:42: #if defined(USE_ASH)
On 2013/06/26 23:42:55, sky wrote:
> Try to minimize ifdefs by nuking the ifdef here and instead ifdefing the parts
> of the implementation that matter.

Done.

https://codereview.chromium.org/16832003/diff/62001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/16832003/diff/62001/chrome/browser/ui/views/t...
chrome/browser/ui/views/tabs/tab.cc:719: if (event.type() == ui::ET_GESTURE_TAP)
On 2013/06/26 23:42:55, sky wrote:
> use {} since multi-line too.

Done.

https://codereview.chromium.org/16832003/diff/74001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/touch_uma/touch_uma.h (right):

https://codereview.chromium.org/16832003/diff/74001/chrome/browser/ui/views/t...
chrome/browser/ui/views/touch_uma/touch_uma.h:22: };
Note I was unable to add in

private:
 DISALLOW_COPY_AND_ASSIGN(TouchUMA);

("ISO C++ forbids declaration of 'DISALLOW_COPY_AND_ASSIGN' with no type")

[sky, 2013-06-27 23:09:35]

Yes, this is what I had in mind.
Thanks!

https://codereview.chromium.org/16832003/diff/74001/ash/touch/touch_uma.h
File ash/touch/touch_uma.h (right):

https://codereview.chromium.org/16832003/diff/74001/ash/touch/touch_uma.h#new...
ash/touch/touch_uma.h:45: GESTURE_ACTION_COUNT = 512
I think it clearer if you define the types you have in views here too and in
touch_uma_ash have a switch to convert between the two. I realize that's a bit
tedious, but less error prone.

https://codereview.chromium.org/16832003/diff/74001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/touch_uma/touch_uma.h (right):

https://codereview.chromium.org/16832003/diff/74001/chrome/browser/ui/views/t...
chrome/browser/ui/views/touch_uma/touch_uma.h:22: };
On 2013/06/27 22:39:33, tdanderson wrote:
> Note I was unable to add in
> 
> private:
>  DISALLOW_COPY_AND_ASSIGN(TouchUMA);
> 
> ("ISO C++ forbids declaration of 'DISALLOW_COPY_AND_ASSIGN' with no type")

You sure you typed in right or forgot the include? You actually want
DISALLOW_IMPLICIT_CONSTRUCTORS here.

[tdanderson, 2013-06-28 18:45:16]

@sky, the enums have been changed according to your suggestion.

@sadrul, does this patch lgty overall?

https://codereview.chromium.org/16832003/diff/74001/ash/touch/touch_uma.h
File ash/touch/touch_uma.h (right):

https://codereview.chromium.org/16832003/diff/74001/ash/touch/touch_uma.h#new...
ash/touch/touch_uma.h:45: GESTURE_ACTION_COUNT = 512
On 2013/06/27 23:09:35, sky wrote:
> I think it clearer if you define the types you have in views here too and in
> touch_uma_ash have a switch to convert between the two. I realize that's a bit
> tedious, but less error prone.

Done.

https://codereview.chromium.org/16832003/diff/74001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/touch_uma/touch_uma.h (right):

https://codereview.chromium.org/16832003/diff/74001/chrome/browser/ui/views/t...
chrome/browser/ui/views/touch_uma/touch_uma.h:22: };
On 2013/06/27 23:09:35, sky wrote:
> On 2013/06/27 22:39:33, tdanderson wrote:
> > Note I was unable to add in
> > 
> > private:
> >  DISALLOW_COPY_AND_ASSIGN(TouchUMA);
> > 
> > ("ISO C++ forbids declaration of 'DISALLOW_COPY_AND_ASSIGN' with no type")
> 
> You sure you typed in right or forgot the include? You actually want
> DISALLOW_IMPLICIT_CONSTRUCTORS here.

Ah, forgot the include. Done.

[sky, 2013-06-28 19:39:38]

LGTM

https://codereview.chromium.org/16832003/diff/85001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/touch_uma/touch_uma_ash.cc (right):

https://codereview.chromium.org/16832003/diff/85001/chrome/browser/ui/views/t...
chrome/browser/ui/views/touch_uma/touch_uma_ash.cc:27: default:
You should be able to remove default so that we get a compile time error if a
new value is added.

[tdanderson, 2013-06-28 19:49:55]

https://codereview.chromium.org/16832003/diff/85001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/touch_uma/touch_uma_ash.cc (right):

https://codereview.chromium.org/16832003/diff/85001/chrome/browser/ui/views/t...
chrome/browser/ui/views/touch_uma/touch_uma_ash.cc:27: default:
On 2013/06/28 19:39:38, sky wrote:
> You should be able to remove default so that we get a compile time error if a
> new value is added.

Done.

[sadrul, 2013-06-28 20:20:53]

LGTM

[commit-bot: I haz the power, 2013-06-28 20:25:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/16832003/85002

[commit-bot: I haz the power, 2013-06-28 20:39:06]

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...

[commit-bot: I haz the power, 2013-06-28 21:22:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdanderson@chromium.org/16832003/85002

[commit-bot: I haz the power, 2013-06-29 01:44:37]

Change committed as 209263