Add UMA histogram for whether page loads are mobile optimized

Add UMA histogram for whether page loads are mobile optimized

This adds a boolean histogram for whether Chrome assumes the web page is
mobile optimized. It is recorded as soon as the Tab gets the callback
for didFinishLoad(). On every call to didNavigateMainFrame(), the bit
the bit it is cleared again. This is only recorded for the main frame.

The viewport might not be correctly identified when didFinishLoad()
gets called, at which point the stat might be a false negative. This
means that this stat could be considered a lower bound for mobile
optimized pages.

BUG=None

Committed: https://crrev.com/2a5fb80d2ff6459eaa5fe52a3ca08ffad32de5d6
Cr-Commit-Position: refs/heads/master@{#317451}

[nyquist, 2015-02-19 23:15:56]

nyquist@chromium.org changed reviewers:
+ cjhopman@chromium.org, isherman@chromium.org, jdduke@chromium.org

[nyquist, 2015-02-19 23:15:56]

cjhopman: PTAL at all things
jdduke: PTAL content/android OWNERS
isherman: PTAL tools/metrics

[jdduke (slow), 2015-02-19 23:39:59]

jdduke@chromium.org changed reviewers:
+ aelias@chromium.org

[jdduke (slow), 2015-02-19 23:40:00]

aelias@: Any thoughts on the potential for racing here? Looks like the
|mIsMobileOptimizedHint| is set upon frame metadata receipt, and reset in the
didNavigateMainFrame observer callback.

https://codereview.chromium.org/943823002/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/943823002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:771:
if (isMainFrame) {
I wonder about the timing of this call relative to frame metadata receipt... I
would hope this always arrives after the frame metadata corresponding to the
initial frame, just want to make sure we're not racing.

[Ilya Sherman, 2015-02-19 23:51:26]

LGTM % nits:

https://codereview.chromium.org/943823002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/943823002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:15245: +<histogram
name="Navigation.IsMobileOptimized" enum="Boolean">
nit: Please associate a custom boolean enum, so that the labels are super
descriptive.

https://codereview.chromium.org/943823002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:15249: +    Signifies if a succesfully
finished page load for the main frame has the
Optional nit: IMO "whether" reads better than "if" in this sentence.

[jdduke (slow), 2015-02-19 23:58:33]

Just read the patch description... I guess if we're ok that this is a lower
bound, although in theory a metadata update for the *previous* page could be
received *after* the didNavigateMainFrame call. Probably not very likely.

Is there a particular motivation for logging this (I'm also interested to see
the numbers, just curious what the data might be informing)?

[aelias_OOO_until_Jul13, 2015-02-20 00:48:42]

This value really means just that the web page fits nicely in the screen size,
so it will pretty much always return true on 10-inch tablets in landscape, for
example.  In retrospect the "mobileOptimized" name is poorly chosen and
"contentWidthFitsWindow" might be better.

How about logging the histogram from Blink when a meta viewport tag is detected
there?  That's a more natural way to do this and should address all of the
problems.

[nyquist, 2015-02-20 01:43:55]

What we are trying to measure is whether the current page is optimized for the
current device, which to me means that whether viewport is set or not is just an
implementation detail.

https://codereview.chromium.org/943823002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/943823002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:15245: +<histogram
name="Navigation.IsMobileOptimized" enum="Boolean">
On 2015/02/19 23:51:26, Ilya Sherman wrote:
> nit: Please associate a custom boolean enum, so that the labels are super
> descriptive.

Done.

https://codereview.chromium.org/943823002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:15249: +    Signifies if a succesfully
finished page load for the main frame has the
On 2015/02/19 23:51:25, Ilya Sherman wrote:
> Optional nit: IMO "whether" reads better than "if" in this sentence.

Done. Agreed.

[nyquist, 2015-02-20 01:46:15]

New patchsets have been uploaded after l-g-t-m from isherman@chromium.org

[nyquist, 2015-02-20 02:01:37]

jdduke, aelias: PTAL.

I moved this to tab instead since there might be content that we are not
interested in tracking, whereas when a tab has finished loading, that is
definitely something we want to know.

[aelias_OOO_until_Jul13, 2015-02-20 02:22:53]

I don't think load-finish winning the race is very likely, but in the interest
of being confident in our data it does seem like a good idea to avoid it.  How
about noting in a boolean on didStartPageLoad, notifying Tab.java about
updateFrameInfo and logging the stat and setting the histogram value from there?

[cjhopman, 2015-02-20 02:31:43]

On 2015/02/20 02:22:53, aelias wrote:
> I don't think load-finish winning the race is very likely, but in the interest
> of being confident in our data it does seem like a good idea to avoid it.  How
> about noting in a boolean on didStartPageLoad, notifying Tab.java about
> updateFrameInfo and logging the stat and setting the histogram value from
there?

Do we know when the frame info is actually correct? The first frame info is
basically always wrong, is the second one always right?

[aelias_OOO_until_Jul13, 2015-02-20 02:42:59]

On 2015/02/20 at 02:31:43, cjhopman wrote:
> On 2015/02/20 02:22:53, aelias wrote:
> > I don't think load-finish winning the race is very likely, but in the
interest
> > of being confident in our data it does seem like a good idea to avoid it. 
How
> > about noting in a boolean on didStartPageLoad, notifying Tab.java about
> > updateFrameInfo and logging the stat and setting the histogram value from
there?
> 
> Do we know when the frame info is actually correct? The first frame info is
basically always wrong, is the second one always right?

Hmm, I was thinking contentWidth != 0 might be a proxy for it, but I can't rule
out scenarios where that briefly blips to a bogus size.

I guess the status quo is good enough.  lgtm

[nyquist, 2015-02-20 22:53:38]

New patchsets have been uploaded after l-g-t-m from aelias@chromium.org

[aelias_OOO_until_Jul13, 2015-02-20 22:55:51]

still lgtm

[nyquist, 2015-02-20 22:56:05]

The CQ bit was checked by nyquist@chromium.org

[commit-bot: I haz the power, 2015-02-20 22:56:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/943823002/80001

[commit-bot: I haz the power, 2015-02-21 00:44:46]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-21 00:45:34]

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