log the click activate/deactive of translate icon on location bar

log the click activate/deactive of translate icon on location bar

BUG=607170
Committed: https://crrev.com/883a6d1220cde7daf15ef9b3e3f96acc5315f682
Cr-Commit-Position: refs/heads/master@{#393359}

[ftang, 2016-05-10 23:14:49]

The CQ bit was checked by ftang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-10 23:15:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962393003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962393003/1

[ftang, 2016-05-10 23:25:00]

changed by 'git cl format --full'

[ftang, 2016-05-10 23:25:16]

The CQ bit was checked by ftang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-10 23:25:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962393003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962393003/20001

[commit-bot: I haz the power, 2016-05-11 00:28:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-11 00:28:17]

Dry run: This issue passed the CQ dry run.

[ftang, 2016-05-11 00:31:59]

ftang@chromium.org changed reviewers:
+ msw@chromium.org

[ftang, 2016-05-11 00:31:59]

To log the click on the Translate icon on Omnibar

[msw, 2016-05-11 18:51:35]

https://codereview.chromium.org/1962393003/diff/20001/chrome/browser/ui/trans...
File chrome/browser/ui/translate/translate_bubble_view_state_transition.h
(right):

https://codereview.chromium.org/1962393003/diff/20001/chrome/browser/ui/trans...
chrome/browser/ui/translate/translate_bubble_view_state_transition.h:65: // The
user click to activate the Omnibar Translate icon.
'Omnibar' is not the correct colloquial term; say "the translate page action
icon" here and below, and rename the values PAGE_ACTION_ICON_ACTIVATED,
PAGE_ACTION_ICON_DEATIVATED or similar. I'm removing the term 'click', because I
presume you'd also like to track similar touch presses, and perhaps even
keyboard-triggered activation.

https://codereview.chromium.org/1962393003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right):

https://codereview.chromium.org/1962393003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/bubble_icon_view.h:54: // Invoked after the
icon is clicked.
nit: "icon is pressed."

https://codereview.chromium.org/1962393003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/bubble_icon_view.h:55: virtual void
OnClicked(bool activated) {}
nit: rename OnPressed to cover touch/keyboard invocation.

https://codereview.chromium.org/1962393003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/translate/translate_icon_view.cc (right):

https://codereview.chromium.org/1962393003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/translate/translate_icon_view.cc:22: const char
kTranslateBubbleUIEvent[] = "Translate.BubbleUiEvent";
nit: define this string constant elsewhere for use here and in
translate_bubble_view.cc and translate_bubble_view_state_transition.cc

[ftang, 2016-05-11 21:45:41]

adjust for review comments

[ftang, 2016-05-11 21:47:01]

The CQ bit was checked by ftang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-11 21:47:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962393003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962393003/40001

[commit-bot: I haz the power, 2016-05-11 23:20:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-11 23:20:05]

Dry run: This issue passed the CQ dry run.

[ftang, 2016-05-11 23:20:23]

PTAL

https://codereview.chromium.org/1962393003/diff/20001/chrome/browser/ui/trans...
File chrome/browser/ui/translate/translate_bubble_view_state_transition.h
(right):

https://codereview.chromium.org/1962393003/diff/20001/chrome/browser/ui/trans...
chrome/browser/ui/translate/translate_bubble_view_state_transition.h:65: // The
user click to activate the Omnibar Translate icon.
On 2016/05/11 18:51:35, msw wrote:
> 'Omnibar' is not the correct colloquial term; say "the translate page action
> icon" here and below, and rename the values PAGE_ACTION_ICON_ACTIVATED,
> PAGE_ACTION_ICON_DEATIVATED or similar. I'm removing the term 'click', because
I
> presume you'd also like to track similar touch presses, and perhaps even
> keyboard-triggered activation.

Done.

https://codereview.chromium.org/1962393003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right):

https://codereview.chromium.org/1962393003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/bubble_icon_view.h:54: // Invoked after the
icon is clicked.
On 2016/05/11 18:51:35, msw wrote:
> nit: "icon is pressed."

Done.

https://codereview.chromium.org/1962393003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/bubble_icon_view.h:55: virtual void
OnClicked(bool activated) {}
On 2016/05/11 18:51:35, msw wrote:
> nit: rename OnPressed to cover touch/keyboard invocation.

Done.

https://codereview.chromium.org/1962393003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/translate/translate_icon_view.cc (right):

https://codereview.chromium.org/1962393003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/translate/translate_icon_view.cc:22: const char
kTranslateBubbleUIEvent[] = "Translate.BubbleUiEvent";
On 2016/05/11 18:51:35, msw wrote:
> nit: define this string constant elsewhere for use here and in
> translate_bubble_view.cc and translate_bubble_view_state_transition.cc

Done.

[msw, 2016-05-12 17:29:41]

https://codereview.chromium.org/1962393003/diff/40001/chrome/browser/ui/trans...
File chrome/browser/ui/translate/translate_bubble_view_state_transition.h
(right):

https://codereview.chromium.org/1962393003/diff/40001/chrome/browser/ui/trans...
chrome/browser/ui/translate/translate_bubble_view_state_transition.h:65: // The
user activate the translate page action icon.
nit: activated

https://codereview.chromium.org/1962393003/diff/40001/chrome/browser/ui/trans...
chrome/browser/ui/translate/translate_bubble_view_state_transition.h:68: // The
user deactivate the translate page action icon.
nit: deactivated

https://codereview.chromium.org/1962393003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right):

https://codereview.chromium.org/1962393003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/bubble_icon_view.cc:88: OnPressed(false);
I think this might be double counting the deactivation event. It seems like line
99 will count a deactivation when the mouse is pressed, and then this line will
count a deactivation when the mouse is released (both on the same 'click' to
close the bubble). Please make sure you don't count two deactivation UMA events
for clicking the icon to close the bubble.

https://codereview.chromium.org/1962393003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/translate/translate_icon_view.cc (right):

https://codereview.chromium.org/1962393003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/translate/translate_icon_view.cc:22: const char
kTranslateBubbleUIEvent[] = "Translate.BubbleUiEvent";
Remove this and use translate::kTranslateBubbleUIEvent below.

[ftang, 2016-05-12 18:37:35]

address review comments

[ftang, 2016-05-12 18:37:58]

The CQ bit was checked by ftang@chromium.org to run a CQ dry run

[ftang, 2016-05-12 18:38:08]

PTAL

https://codereview.chromium.org/1962393003/diff/40001/chrome/browser/ui/trans...
File chrome/browser/ui/translate/translate_bubble_view_state_transition.h
(right):

https://codereview.chromium.org/1962393003/diff/40001/chrome/browser/ui/trans...
chrome/browser/ui/translate/translate_bubble_view_state_transition.h:65: // The
user activate the translate page action icon.
On 2016/05/12 17:29:41, msw wrote:
> nit: activated

Done.

https://codereview.chromium.org/1962393003/diff/40001/chrome/browser/ui/trans...
chrome/browser/ui/translate/translate_bubble_view_state_transition.h:68: // The
user deactivate the translate page action icon.
On 2016/05/12 17:29:41, msw wrote:
> nit: deactivated

Done.

https://codereview.chromium.org/1962393003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right):

https://codereview.chromium.org/1962393003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/bubble_icon_view.cc:88: OnPressed(false);
On 2016/05/12 17:29:41, msw wrote:
> I think this might be double counting the deactivation event. It seems like
line
> 99 will count a deactivation when the mouse is pressed, and then this line
will
> count a deactivation when the mouse is released (both on the same 'click' to
> close the bubble). Please make sure you don't count two deactivation UMA
events
> for clicking the icon to close the bubble.

line 99 will be hit only on mouse released but not pressed (since it is still
inside the "OnMousePress" but not "OnMousePressed" handler. I put VLOG inside
OnPressed to print the value and it work correctly.

https://codereview.chromium.org/1962393003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/translate/translate_icon_view.cc (right):

https://codereview.chromium.org/1962393003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/translate/translate_icon_view.cc:22: const char
kTranslateBubbleUIEvent[] = "Translate.BubbleUiEvent";
On 2016/05/12 17:29:41, msw wrote:
> Remove this and use translate::kTranslateBubbleUIEvent below.

Done.

[commit-bot: I haz the power, 2016-05-12 18:38:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962393003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962393003/60001

[msw, 2016-05-12 19:02:53]

lgtm

https://codereview.chromium.org/1962393003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/bubble_icon_view.cc (right):

https://codereview.chromium.org/1962393003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/bubble_icon_view.cc:88: OnPressed(false);
On 2016/05/12 18:38:08, ftang wrote:
> On 2016/05/12 17:29:41, msw wrote:
> > I think this might be double counting the deactivation event. It seems like
> line
> > 99 will count a deactivation when the mouse is pressed, and then this line
> will
> > count a deactivation when the mouse is released (both on the same 'click' to
> > close the bubble). Please make sure you don't count two deactivation UMA
> events
> > for clicking the icon to close the bubble.
> 
> line 99 will be hit only on mouse released but not pressed (since it is still
> inside the "OnMousePress" but not "OnMousePressed" handler. I put VLOG inside
> OnPressed to print the value and it work correctly.

Acknowledged.

[commit-bot: I haz the power, 2016-05-12 19:21:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-12 19:21:51]

Dry run: This issue passed the CQ dry run.

[ftang, 2016-05-12 21:25:04]

The CQ bit was checked by ftang@chromium.org

[commit-bot: I haz the power, 2016-05-12 21:25:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962393003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962393003/60001

[commit-bot: I haz the power, 2016-05-12 21:29:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-12 21:30:29]

Description was changed from

==========
log the click activate/deactive of translate icon on location bar

BUG=607170
==========

to

==========
log the click activate/deactive of translate icon on location bar

BUG=607170

Committed: https://crrev.com/883a6d1220cde7daf15ef9b3e3f96acc5315f682
Cr-Commit-Position: refs/heads/master@{#393359}
==========

[commit-bot: I haz the power, 2016-05-12 21:30:30]

Patchset 4 (id:??) landed as
https://crrev.com/883a6d1220cde7daf15ef9b3e3f96acc5315f682
Cr-Commit-Position: refs/heads/master@{#393359}