[Sync] Updating HistoryPage.OtherDevicesMenu metric to capture presence of foreign data.

[Sync] Updating HistoryPage.OtherDevicesMenu metric to capture presence of foreign data.

BUG=568353
Committed: https://crrev.com/b38017043553980fc58aaa7e8752c35fd22b2a3e
Cr-Commit-Position: refs/heads/master@{#367030}

[skym, 2015-12-22 00:49:35]

Description was changed from

==========
[Sync] Updating HistoryPage.OtherDevicesMenu metric to capture presence of
foreign data.

BUG=568353
==========

to

==========
[Sync] Updating HistoryPage.OtherDevicesMenu metric to capture presence of
foreign data.

BUG=568353
==========

[skym, 2015-12-22 00:49:35]

skym@chromium.org changed reviewers:
+ dbeam@chromium.org, jwd@chromium.org

[jwd, 2015-12-22 16:32:09]

metrics LGTM

[skym, 2015-12-28 17:16:27]

PTAL when you have have time dbeam@

[Dan Beam, 2015-12-28 21:50:34]

can we just do this in C++ instead?

i.e. here
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[skym, 2015-12-28 22:31:28]

On 2015/12/28 21:50:34, Dan Beam wrote:
> can we just do this in C++ instead?
> 
> i.e. here
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

So why would you prefer to do this in C++ code instead? I haven't really worked
with any of the js in chrome so far so I'd love to understand more.

Reasons I see against switching:
* Histogram is already defined in javascript. Would be unfortunate to have it
defined in two places.
* All the rest of the associated metrics for this histogram are emitting in the
same js file, nice that they're all in the same local place.
* Page refresh (like F5) doesn't create a new ForeignSessionHandler object, but
the javascript is run upon refresh. It would be nice if we could compare
HAS_FOREIGN_DATA and INITIALIZED, which wouldn't work anymore unless we added
logic to signal from javascript to c++ that a page refresh happened, and then we
could reset ForeignSessionHandler's member variable tracking if the metric was
emitted yet. (There may be a more elegant solution to this that I'm missing). In
general I don't really understand the lifecycle of the ForeignSessionHandler.

[Dan Beam, 2015-12-28 22:56:33]

On 2015/12/28 22:31:28, skym wrote:
> On 2015/12/28 21:50:34, Dan Beam wrote:
> > can we just do this in C++ instead?
> > 
> > i.e. here
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> So why would you prefer to do this in C++ code instead? I haven't really
worked
> with any of the js in chrome so far so I'd love to understand more.
> 
> Reasons I see against switching:
> * Histogram is already defined in javascript. Would be unfortunate to have it
> defined in two places.

UMA histograms are overwhelmingly more often defined in C++.  This is a very
rare case where it only exists in JS (I've been working in the codebase for like
4+ years and have never found a case like this until now; I also split off
MetricsHandler as it's own class here[1]).

I should probably just remove the ability to do this from JS, because it will
inevitably result in:

  chrome.send('doSomethingInC++');
  chrome.send('metricsHandler:recordInHistogram', ...);

when the handler in the C++ could easily do this as well.

[1] https://codereview.chromium.org/8052028

> * All the rest of the associated metrics for this histogram are emitting in
the
> same js file, nice that they're all in the same local place.

Moving to the C++ shouldn't affect the way the metrics are logged, as far as I
can tell, as everything is initiated from a chrome.send() in the JS and not tied
to lifetime of the C++ handlers (i.e. in dtor or ctor).

> * Page refresh (like F5) doesn't create a new ForeignSessionHandler object,
but
> the javascript is run upon refresh. It would be nice if we could compare
> HAS_FOREIGN_DATA and INITIALIZED, which wouldn't work anymore unless we added
> logic to signal from javascript to c++ that a page refresh happened, and then
we
> could reset ForeignSessionHandler's member variable tracking if the metric was
> emitted yet. (There may be a more elegant solution to this that I'm missing).
In
> general I don't really understand the lifecycle of the ForeignSessionHandler.

Created once per renderer (i.e. re-used on refresh).

tl;dr - this code is likely being drastically rethought by folks in SYD right
now (redesigning this page).  How foreign sessions will be shown/implemented is
quite in flux.  So in that case: I'm fine with just adding 3 more lines of JS
even if it's kinda weird (and this page shouldn't probably be doing it this way
or this standalone metrics stuff shouldn't exist).

lgtm

[skym, 2015-12-28 23:17:36]

On 2015/12/28 22:56:33, Dan Beam wrote:
> On 2015/12/28 22:31:28, skym wrote:
> > On 2015/12/28 21:50:34, Dan Beam wrote:
> > > can we just do this in C++ instead?
> > > 
> > > i.e. here
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> > 
> > So why would you prefer to do this in C++ code instead? I haven't really
> worked
> > with any of the js in chrome so far so I'd love to understand more.
> > 
> > Reasons I see against switching:
> > * Histogram is already defined in javascript. Would be unfortunate to have
it
> > defined in two places.
> 
> UMA histograms are overwhelmingly more often defined in C++.  This is a very
> rare case where it only exists in JS (I've been working in the codebase for
like
> 4+ years and have never found a case like this until now; I also split off
> MetricsHandler as it's own class here[1]).
> 
> I should probably just remove the ability to do this from JS, because it will
> inevitably result in:
> 
>   chrome.send('doSomethingInC++');
>   chrome.send('metricsHandler:recordInHistogram', ...);
> 
> when the handler in the C++ could easily do this as well.
> 
> [1] https://codereview.chromium.org/8052028
> 
> > * All the rest of the associated metrics for this histogram are emitting in
> the
> > same js file, nice that they're all in the same local place.
> 
> Moving to the C++ shouldn't affect the way the metrics are logged, as far as I
> can tell, as everything is initiated from a chrome.send() in the JS and not
tied
> to lifetime of the C++ handlers (i.e. in dtor or ctor).
> 
> > * Page refresh (like F5) doesn't create a new ForeignSessionHandler object,
> but
> > the javascript is run upon refresh. It would be nice if we could compare
> > HAS_FOREIGN_DATA and INITIALIZED, which wouldn't work anymore unless we
added
> > logic to signal from javascript to c++ that a page refresh happened, and
then
> we
> > could reset ForeignSessionHandler's member variable tracking if the metric
was
> > emitted yet. (There may be a more elegant solution to this that I'm
missing).
> In
> > general I don't really understand the lifecycle of the
ForeignSessionHandler.
> 
> Created once per renderer (i.e. re-used on refresh).
> 
> tl;dr - this code is likely being drastically rethought by folks in SYD right
> now (redesigning this page).  How foreign sessions will be shown/implemented
is
> quite in flux.  So in that case: I'm fine with just adding 3 more lines of JS
> even if it's kinda weird (and this page shouldn't probably be doing it this
way
> or this standalone metrics stuff shouldn't exist).
> 
> lgtm

Thanks a ton for the background, that makes sense.

What exactly is the focus of the SYD team? The entire history page? The foreign
tabs? Is there a design doc or someone I could contact to learn more?

[skym, 2015-12-28 23:17:48]

The CQ bit was checked by skym@chromium.org

[commit-bot: I haz the power, 2015-12-28 23:18:10]

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

[commit-bot: I haz the power, 2015-12-29 00:25:06]

Description was changed from

==========
[Sync] Updating HistoryPage.OtherDevicesMenu metric to capture presence of
foreign data.

BUG=568353
==========

to

==========
[Sync] Updating HistoryPage.OtherDevicesMenu metric to capture presence of
foreign data.

BUG=568353
==========

[commit-bot: I haz the power, 2015-12-29 00:25:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-29 00:26:07]

Description was changed from

==========
[Sync] Updating HistoryPage.OtherDevicesMenu metric to capture presence of
foreign data.

BUG=568353
==========

to

==========
[Sync] Updating HistoryPage.OtherDevicesMenu metric to capture presence of
foreign data.

BUG=568353

Committed: https://crrev.com/b38017043553980fc58aaa7e8752c35fd22b2a3e
Cr-Commit-Position: refs/heads/master@{#367030}
==========

[commit-bot: I haz the power, 2015-12-29 00:26:08]

Patchset 2 (id:??) landed as
https://crrev.com/b38017043553980fc58aaa7e8752c35fd22b2a3e
Cr-Commit-Position: refs/heads/master@{#367030}