Move NewTabPage.LogoShownTime metrics recording into LogoBridge

Move NewTabPage.LogoShownTime metrics recording into LogoBridge

In order to record NTP.LogoShownTime for timely refreshes only, there
would be many JNI callbacks in the bridge's implementation necessary.

Therefore, this CL moves the recording into the C++ side of the bridge
and implements the originally intended behavior of the metric.

BUG=713166

[fhorschig, 2017-04-26 16:44:49]

fhorschig@chromium.org changed reviewers:
+ treib@chromium.org

[fhorschig, 2017-04-26 16:44:50]

Hi Marc, this is the alternative CL referenced in 
https://codereview.chromium.org/2833473002/.

It is not completely tested but would you please have a
look at the concept?

[tschumann, 2017-04-27 06:09:17]

https://codereview.chromium.org/2838833005/diff/20001/chrome/browser/android/...
File chrome/browser/android/logo_bridge.h (right):

https://codereview.chromium.org/2838833005/diff/20001/chrome/browser/android/...
chrome/browser/android/logo_bridge.h:40: class LoadTimeMetricRecorder {
this seems overly complicated to me. Why do we need to maintain so much state?
What are we abstracting here?

I guess the main question is why do we have a long-lived metric-recorder (as a
member) and not one per request. In the end, it's all about capturing a start
time and deciding whether to log it or not, based on conditions we already
know... (at the places where we now call the recorder).

The more state we can make request-scoped (i.e. not outlive the request), the
simpler the solution would get (e.g., one-time callbacks instead of registered
observers; per-request objects (passed through parameters) instead of members.
Happy to discuss offline.

[fhorschig, 2017-04-27 11:14:41]

Description was changed from

==========
Move NewTabPage.LogoShownTime metrics recording into LogoBridge

In order to record NTP.LogoShownTime for timely refreshes only, there
would be many JNI callbacks in the bridge's implementation necessary.

Therefore, this CL moves the recording into the C++ side of the bridge
and implements the originally intended behavior of the metric.

BUG=713166
==========

to

==========
Move NewTabPage.LogoShownTime metrics recording into LogoBridge

In order to record NTP.LogoShownTime for timely refreshes only, there
would be many JNI callbacks in the bridge's implementation necessary.

Therefore, this CL moves the recording into the C++ side of the bridge
and implements the originally intended behavior of the metric.

BUG=713166
==========

[fhorschig, 2017-04-27 11:16:14]

As discussed, we will go for the originally discussed measuring in the UI:
https://codereview.chromium.org/2833473002/

Therefore, I close this CL for now.
(I took the 2min to extract the state anyway. 
Just so we have a clean reference for this alternative approach.)

https://codereview.chromium.org/2838833005/diff/20001/chrome/browser/android/...
File chrome/browser/android/logo_bridge.h (right):

https://codereview.chromium.org/2838833005/diff/20001/chrome/browser/android/...
chrome/browser/android/logo_bridge.h:40: class LoadTimeMetricRecorder {
On 2017/04/27 06:09:17, tschumann wrote:
> this seems overly complicated to me. Why do we need to maintain so much state?
> What are we abstracting here?
>
> I guess the main question is why do we have a long-lived metric-recorder (as a
> member) and not one per request. In the end, it's all about capturing a start
> time and deciding whether to log it or not, based on conditions we already
> know... (at the places where we now call the recorder).
> 
> The more state we can make request-scoped (i.e. not outlive the request), the
> simpler the solution would get (e.g., one-time callbacks instead of registered
> observers; per-request objects (passed through parameters) instead of members.
> Happy to discuss offline.

I moved the state out of the class. 
Although this CL should be abandoned soon as we discussed to do this in the UI.