WebApp splashscreen: add UMA to record how a splashscreen is constructed.

WebApp splashscreen: add UMA to record how a splashscreen is constructed.

It is adding the following UMA with tests:
- Webapp.Splashcreen.Duration: how long the splashscreen is being shown.
- Webapp.Splashscreen.Hides: why the splashscreen was hidden.
- Webapp.Splashscreen.BackgroundColor: default vs custom background color.
- Webapp.Splashscreen.ThemeColor: default vs custom theme color.
- Webapp.Splashscreen.Icon.Type: no icon vs fallback (homescreen) vs custom icon.
- Webapp.Splashscreen.Icon.Size: size in dp of the icon being shown (no records if no icon).

BUG=542389
Committed: https://crrev.com/f32d003d13534cd5ca100269f93aa97f7f5e1968
Cr-Commit-Position: refs/heads/master@{#355598}

[mlamouri (slow - plz ping), 2015-10-20 16:30:25]

Description was changed from

==========
WebApp splashscreen: add UMA to record how a splashscreen is constructed.

<WIP>

BUG=542389
==========

to

==========
WebApp splashscreen: add UMA to record how a splashscreen is constructed.

It is adding the following UMA with tests:
- Webapp.Splashcreen.Duration: how long the splashscreen is being shown.
- Webapp.Splashscreen.Hides: why the splashscreen was hidden.
- Webapp.Splashscreen.BackgroundColor: default vs custom background color.
- Webapp.Splashscreen.ThemeColor: default vs custom theme color.
- Webapp.Splashscreen.Icon.Type: no icon vs fallback (homescreen) vs custom
icon.
- Webapp.Splashscreen.Icon.Size: size in dp of the icon being shown (no records
if no icon).

BUG=542389
==========

[mlamouri (slow - plz ping), 2015-10-20 16:32:07]

mlamouri@chromium.org changed reviewers:
+ asvitkine@chromium.org, dfalcantara@chromium.org, nyquist@chromium.org

[mlamouri (slow - plz ping), 2015-10-20 16:32:08]

Could you PTAL at:

nyquist:
  base/android/java/src/org/chromium/base/metrics/RecordHistogram.java

asvitkine:
  tools/metrics/histograms/histograms.xml (and its usage)

dfalcantara:
  *

Thanks!

[gone, 2015-10-20 18:24:29]

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java
(right):

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java:14: *
Centralizes UMA data collection for Webapps.
nit: can we standardize on "web app" in comments and "splash
screen"/"SplashScreen" everywhere?  The capitalization mistake has driven me
nuts for years.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java:37:
"Webapp.Splashscreen.BackgroundColor";
Does it make sense to put these under the "Launch." top-level histograms?  I
believe the UMA folks generally discourage the addition of new ones (like
Webapp).

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java:53:
private long mSplashScreenVisibleTime = 0;
nit: -1?

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java:56: 
javadocs on all public methods

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java:59:
mSplashScreenVisibleTime = SystemClock.elapsedRealtime();
should the two color methods be combined with this instead of having three
distinct calls?

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java:73:
assert mSplashScreenVisibleTime != 0;
nit: >= 0?

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java:78:
public void recordSplashscreenIconType(int type) {
can this be combined with the recordSplashscreenIconSize() method?

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java:114:
assert mSplashScreenIconSize >= 0;
Given that this has a 999 limit, shouldn't this do a
Math.max(mSplashScreenIconSize, 999)?

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:169:

nit: just remove the extra newline

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:181:
* or if it is not opaque.
"opaque" means an alpha value of 255, doesn't it?  This seems like it's just
checking for any non-zero alpha value.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:184:
if (mThemeColor == ShortcutHelper.MANIFEST_COLOR_INVALID_OR_MISSING
Can this just be:

return mThemeColor != ShortcutHelper.MANIFEST_COLOR_INVALID_OR_MISSING
        && ((mThemeColor & 0xFF000000L) == 255);

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenBackgroundColorTest.java
(right):

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenBackgroundColorTest.java:19:
public class WebappSplashScreenBackgroundColorTest extends
WebappSplashScreenTest {
You should just make a WebappSplashScreenTestBase instead of extending the other
test.

I imagine that extending WebappSplashScreenTest, makes the test runner run the
same tests for WebappSplashScreenTest again as
WebappSplashScreenBackgroundColorTests.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenBackgroundColorTest.java:24:
intent.putExtra(ShortcutHelper.EXTRA_URL, "http://localhost");
nit: about:blank?

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenBackgroundColorTest.java:29:
// TODO(mlammouri): test that the background color is actually shown.
nit: mlamouri

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenHomescreenIconTest.java
(right):

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenHomescreenIconTest.java:17:
* Tests for splashscreen with EXTRA_ICON specified in the Intent.
Does it make sense to have so many different test suites instead of having just
a WebappSplashScreenTest suite?

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenTest.java
(right):

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenTest.java:312:
assertEquals(1, getHistogramTotalCountFor(HISTOGRAM_SPLASHSCREEN_ICON_TYPE, 3));
shouldn't you be using the MAX values you defined instead of 2s and 3s?

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenTest.java:348:
}));
I think this defaults to waiting at most 3 seconds.  Is that enough time to fade
out the splash screen without being flaky?

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenTest.java:377:

nit: remove the newlines between the assertEquals, here and above

[mlamouri (slow - plz ping), 2015-10-21 14:52:34]

In addition of the comments below, I've done two changes:
- I've added a waitUntilSplashHides() helper method to reduce code duplication.
- I've merged a few UMA tests, removing 4 tests in order to speed up the
runtime.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java
(right):

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java:14: *
Centralizes UMA data collection for Webapps.
On 2015/10/20 at 18:24:28, dfalcantara wrote:
> nit: can we standardize on "web app" in comments and "splash
screen"/"SplashScreen" everywhere?  The capitalization mistake has driven me
nuts for years.

Will do my best to keep that in mind. A quick grep shows that it was the only
occurrence in this CL. I updated a few "splashscreen" to "splash scren".

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java:37:
"Webapp.Splashscreen.BackgroundColor";
On 2015/10/20 at 18:24:28, dfalcantara wrote:
> Does it make sense to put these under the "Launch." top-level histograms?  I
believe the UMA folks generally discourage the addition of new ones (like
Webapp).

I've never heard that feedback about avoiding new top level histograms. I think
we ought to have more web apps histograms and having them more visible will
help. Let see what histograms review says.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java:53:
private long mSplashScreenVisibleTime = 0;
On 2015/10/20 at 18:24:28, dfalcantara wrote:
> nit: -1?

I don't think that would change anything. 0 is commonly used as an invalid value
for time. base::Time uses it for example.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java:56: 
On 2015/10/20 at 18:24:28, dfalcantara wrote:
> javadocs on all public methods

Done.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java:59:
mSplashScreenVisibleTime = SystemClock.elapsedRealtime();
On 2015/10/20 at 18:24:28, dfalcantara wrote:
> should the two color methods be combined with this instead of having three
distinct calls?

I guess you meant to comment below. Though, I would prefer to keep the method
separated for the sack of readability. It's clearer from the caller what is
being recorded by making the calls directly instead of having a single entry
point with so many methods it is no longer clear what is really happening. Also
note that we are using Java "int flags" instead of enums so it's very easy to
end up passing the ThemeColor value to the BackgroundColor, there is really
nothing preventing that unless we have clear method names... ... it might have
happened to me while writing the code (though, tests caught the problem :))

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java:73:
assert mSplashScreenVisibleTime != 0;
On 2015/10/20 at 18:24:28, dfalcantara wrote:
> nit: >= 0?

I guess if the default value is 0, != 0 should be okay? If the value is
negative, we probably have bigger problems. I can change if you really want it.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java:78:
public void recordSplashscreenIconType(int type) {
On 2015/10/20 at 18:24:28, dfalcantara wrote:
> can this be combined with the recordSplashscreenIconSize() method?

Same as above: clarity, granularity and lack of types.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java:114:
assert mSplashScreenIconSize >= 0;
On 2015/10/20 at 18:24:28, dfalcantara wrote:
> Given that this has a 999 limit, shouldn't this do a
Math.max(mSplashScreenIconSize, 999)?

AFAIK, the 999 limit isn't a hard limit, it will just put anything above 999 to
the highest bucket which is fine.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java
(right):

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:169:

On 2015/10/20 at 18:24:28, dfalcantara wrote:
> nit: just remove the extra newline

sure

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:181:
* or if it is not opaque.
On 2015/10/20 at 18:24:28, dfalcantara wrote:
> "opaque" means an alpha value of 255, doesn't it?  This seems like it's just
checking for any non-zero alpha value.

You are very right. Changed to "transparent". I still wonder if we need to do
that check but I guess it's not really the point of this CL.

[...]

Later: I found a crash, https://crbug.com/545988 and sent a fix simplifying all
this code.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:184:
if (mThemeColor == ShortcutHelper.MANIFEST_COLOR_INVALID_OR_MISSING
On 2015/10/20 at 18:24:28, dfalcantara wrote:
> Can this just be:
> 
> return mThemeColor != ShortcutHelper.MANIFEST_COLOR_INVALID_OR_MISSING
>         && ((mThemeColor & 0xFF000000L) == 255);

Hmm, so your check is slightly different. It checks full opacity instead of not
fully transparent. Anyway, this is no longer needed.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenBackgroundColorTest.java
(right):

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenBackgroundColorTest.java:19:
public class WebappSplashScreenBackgroundColorTest extends
WebappSplashScreenTest {
On 2015/10/20 at 18:24:28, dfalcantara wrote:
> You should just make a WebappSplashScreenTestBase instead of extending the
other test.
> 
> I imagine that extending WebappSplashScreenTest, makes the test runner run the
same tests for WebappSplashScreenTest again as
WebappSplashScreenBackgroundColorTests.

It doesn't actually confuses the test runner but I moved all of the tests to
inherit from WebappActivityTestBase and moved the common stuff there.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenBackgroundColorTest.java:24:
intent.putExtra(ShortcutHelper.EXTRA_URL, "http://localhost");
On 2015/10/20 at 18:24:28, dfalcantara wrote:
> nit: about:blank?

Removed entirely. about:blank is the default.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenBackgroundColorTest.java:29:
// TODO(mlammouri): test that the background color is actually shown.
On 2015/10/20 at 18:24:28, dfalcantara wrote:
> nit: mlamouri

Are you saying I can't write my own name?

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenHomescreenIconTest.java
(right):

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenHomescreenIconTest.java:17:
* Tests for splashscreen with EXTRA_ICON specified in the Intent.
On 2015/10/20 at 18:24:28, dfalcantara wrote:
> Does it make sense to have so many different test suites instead of having
just a WebappSplashScreenTest suite?

Yes. Because the activity is created because the test unit is actually ran so if
we want to pass different information to the Activity (ie. the intent creating
it), we must have different classes. We should probably fix that in the long
run.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenTest.java
(right):

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenTest.java:312:
assertEquals(1, getHistogramTotalCountFor(HISTOGRAM_SPLASHSCREEN_ICON_TYPE, 3));
On 2015/10/20 at 18:24:28, dfalcantara wrote:
> shouldn't you be using the MAX values you defined instead of 2s and 3s?

Good catch! Done.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenTest.java:348:
}));
On 2015/10/20 at 18:24:28, dfalcantara wrote:
> I think this defaults to waiting at most 3 seconds.  Is that enough time to
fade out the splash screen without being flaky?

I would hope so. I'm testing on an Android One phone so the tests are quick.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenTest.java:377:

On 2015/10/20 at 18:24:28, dfalcantara wrote:
> nit: remove the newlines between the assertEquals, here and above

Done.

[mlamouri (slow - plz ping), 2015-10-21 14:53:46]

mlamouri@chromium.org changed reviewers:
+ rmcilroy@chromium.org
- nyquist@chromium.org

[mlamouri (slow - plz ping), 2015-10-21 14:53:47]

-nyquist@, +rmcilroy@ (local reviewer)

rmcilroy@, PTAL at base/android/ changes.

[rmcilroy, 2015-10-21 15:02:42]

base/android lgtm.

[gone, 2015-10-21 17:19:47]

lgtm

You might want to try isherman@ if asvitkine@ is unavailable.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java
(right):

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java:37:
"Webapp.Splashscreen.BackgroundColor";
On 2015/10/21 14:52:34, Mounir Lamouri wrote:
> On 2015/10/20 at 18:24:28, dfalcantara wrote:
> > Does it make sense to put these under the "Launch." top-level histograms?  I
> believe the UMA folks generally discourage the addition of new ones (like
> Webapp).
> 
> I've never heard that feedback about avoiding new top level histograms. I
think
> we ought to have more web apps histograms and having them more visible will
> help. Let see what histograms review says.

Acknowledged.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebappUma.java:59:
mSplashScreenVisibleTime = SystemClock.elapsedRealtime();
On 2015/10/21 14:52:34, Mounir Lamouri wrote:
> On 2015/10/20 at 18:24:28, dfalcantara wrote:
> > should the two color methods be combined with this instead of having three
> distinct calls?
> 
> I guess you meant to comment below. Though, I would prefer to keep the method
> separated for the sack of readability. It's clearer from the caller what is
> being recorded by making the calls directly instead of having a single entry
> point with so many methods it is no longer clear what is really happening.
Also
> note that we are using Java "int flags" instead of enums so it's very easy to
> end up passing the ThemeColor value to the BackgroundColor, there is really
> nothing preventing that unless we have clear method names... ... it might have
> happened to me while writing the code (though, tests caught the problem :))

Acknowledged.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:181:
* or if it is not opaque.
On 2015/10/21 14:52:34, Mounir Lamouri wrote:
> On 2015/10/20 at 18:24:28, dfalcantara wrote:
> > "opaque" means an alpha value of 255, doesn't it?  This seems like it's just
> checking for any non-zero alpha value.
> 
> You are very right. Changed to "transparent". I still wonder if we need to do
> that check but I guess it's not really the point of this CL.
> 
> [...]
> 
> Later: I found a crash, https://crbug.com/545988 and sent a fix simplifying
all
> this code.

Acknowledged.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:184:
if (mThemeColor == ShortcutHelper.MANIFEST_COLOR_INVALID_OR_MISSING
On 2015/10/21 14:52:34, Mounir Lamouri wrote:
> On 2015/10/20 at 18:24:28, dfalcantara wrote:
> > Can this just be:
> > 
> > return mThemeColor != ShortcutHelper.MANIFEST_COLOR_INVALID_OR_MISSING
> >         && ((mThemeColor & 0xFF000000L) == 255);
> 
> Hmm, so your check is slightly different. It checks full opacity instead of
not
> fully transparent. Anyway, this is no longer needed.

Yeah, was going with the correct definition of opaque.

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenBackgroundColorTest.java
(right):

https://codereview.chromium.org/1391993004/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenBackgroundColorTest.java:29:
// TODO(mlammouri): test that the background color is actually shown.
On 2015/10/21 14:52:34, Mounir Lamouri wrote:
> On 2015/10/20 at 18:24:28, dfalcantara wrote:
> > nit: mlamouri
> 
> Are you saying I can't write my own name?

Nah, just your LDAP ;)

[Alexei Svitkine (slow), 2015-10-21 17:28:55]

https://codereview.chromium.org/1391993004/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappActivityTestBase.java
(right):

https://codereview.chromium.org/1391993004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappActivityTestBase.java:38:
"Webapp.Splashscreen.Icon.Size";
Can you just make the constants in WebappUma public and use those instead of
re-defining the strings?

https://codereview.chromium.org/1391993004/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1391993004/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:50067: +    pixels (ie. real size
divided by device pixel density).
Is this the area of it? (i.e. width x height)

If so, recordCount1000Histogram() might not be sufficient, since 1000 is only
100x100. You could use just Counts(), which has a bigger max value, or define a
custom one.

[Alexei Svitkine (slow), 2015-10-21 17:30:44]

https://codereview.chromium.org/1391993004/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1391993004/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:50067: +    pixels (ie. real size
divided by device pixel density).
On 2015/10/21 17:28:55, Alexei Svitkine (slow) wrote:
> Is this the area of it? (i.e. width x height)
> 
> If so, recordCount1000Histogram() might not be sufficient, since 1000 is only
> 100x100. You could use just Counts(), which has a bigger max value, or define
a
> custom one.

Er, not even 100x100 ;).

[mlamouri (slow - plz ping), 2015-10-21 18:02:58]

asvitkine@, I've applied your comments. PTAL.

https://codereview.chromium.org/1391993004/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappActivityTestBase.java
(right):

https://codereview.chromium.org/1391993004/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappActivityTestBase.java:38:
"Webapp.Splashscreen.Icon.Size";
On 2015/10/21 at 17:28:55, Alexei Svitkine (slow) wrote:
> Can you just make the constants in WebappUma public and use those instead of
re-defining the strings?

Done.

https://codereview.chromium.org/1391993004/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1391993004/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:50067: +    pixels (ie. real size
divided by device pixel density).
On 2015/10/21 at 17:30:43, Alexei Svitkine (slow) wrote:
> On 2015/10/21 17:28:55, Alexei Svitkine (slow) wrote:
> > Is this the area of it? (i.e. width x height)
> > 
> > If so, recordCount1000Histogram() might not be sufficient, since 1000 is
only
> > 100x100. You could use just Counts(), which has a bigger max value, or
define a
> > custom one.
> 
> Er, not even 100x100 ;).

It's only one dimension. I've updated the CL to make this clearer.

[Alexei Svitkine (slow), 2015-10-21 18:05:32]

lgtm

[mlamouri (slow - plz ping), 2015-10-21 20:41:01]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-10-21 20:41:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1391993004/#ps80001
(title: "review comments")

[commit-bot: I haz the power, 2015-10-21 20:41:21]

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

[commit-bot: I haz the power, 2015-10-21 22:27:31]

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

[commit-bot: I haz the power, 2015-10-21 22:27:32]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[mlamouri (slow - plz ping), 2015-10-22 13:18:08]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2015-10-22 13:18:19]

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

[mlamouri (slow - plz ping), 2015-10-22 13:19:03]

The CQ bit was unchecked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-10-22 13:20:20]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-10-22 13:20:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, dfalcantara@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1391993004/#ps100001
(title: "disable broken tests")

[commit-bot: I haz the power, 2015-10-22 13:20:31]

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

[commit-bot: I haz the power, 2015-10-22 14:32:33]

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

[commit-bot: I haz the power, 2015-10-22 14:32:34]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mlamouri (slow - plz ping), 2015-10-22 17:50:46]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-10-22 17:50:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, dfalcantara@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1391993004/#ps120001
(title: "don't assert")

[commit-bot: I haz the power, 2015-10-22 17:50:58]

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

[commit-bot: I haz the power, 2015-10-22 19:12:44]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-22 19:13:40]

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