Webapps splashscreen: add moar tests.

Webapps splashscreen: add moar tests.

This is adding many tests that were required for the splashscreen feature,
checking that the theme-color behaviour works as expected, that the
splashscreen properly disappears when the page is loaded, crashes or paints.

This is testing a few recent CLs and some older ones.

BUG=542773, 531519, 542626, 543604
Committed: https://crrev.com/72504ef40ebaaed432d9bc864c482bfeaa415482
Cr-Commit-Position: refs/heads/master@{#354844}

[mlamouri (slow - plz ping), 2015-10-15 17:44:25]

mlamouri@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[mlamouri (slow - plz ping), 2015-10-15 17:44:26]

aurimas@, FYI. Pinky promise was fulfilled :)

[mlamouri (slow - plz ping), 2015-10-17 12:56:21]

mlamouri@chromium.org changed reviewers:
+ dtrainor@chromium.org

[mlamouri (slow - plz ping), 2015-10-17 12:56:21]

+dtrainor@ because dfalcantara@ said you have opinions about how to instrument
TabObservers.

[aurimas (slooooooooow), 2015-10-17 16:14:14]

aurimas@chromium.org changed reviewers:
+ aurimas@chromium.org

[aurimas (slooooooooow), 2015-10-17 16:14:14]

Calling the observers directly seems a little odd to me. Can we actually cause
the event to happen instead?

[mlamouri (slow - plz ping), 2015-10-19 11:10:48]

On 2015/10/17 at 16:14:14, aurimas wrote:
> Calling the observers directly seems a little odd to me. Can we actually cause
the event to happen instead?

In general, I think this will lead to flakyness and the tests will be way
slower. I wrote this test with the idea that we will test things down to
TabObserver and TabObserver is tested. It's not part of the webapps splashscreen
to check if TabObserver is actually firing the events.

In particular, the PageLoad/FirstPaint events will be very hard to decouple if
we test the full stack. They are two completely orthogonal concepts and one
might happen before or after the other depending on various things especially
for very small pages that load quickly. Without some instrumentation, we will
not be able to test that these events both remove the splashscreen.

[aurimas (slooooooooow), 2015-10-19 15:11:18]

lgtm

[David Trainor- moved to gerrit, 2015-10-19 15:16:36]

On 2015/10/19 15:11:18, aurimas wrote:
> lgtm

Yeah sadly I don't have a good alternative to this atm :(.  I'm fine with this,
but we should look into a better way of doing it.

Hmm actually, I'd almost prefer some kind of TabTestUtils that has a bunch of
methods that simulate this.  Ie:

TabTestUtils#crashTab()
TabTestUtils#sendDidFirstPaint()

These could go to native, hit the web contents, and poke that object/it's
exposed members itself.  I don't know if it's much better, but it would test
more of the Tab stack and not expose the observers here.  The only downside is
that this requires native, but if you're using Tab you probably have native
anyway :).

lgtm, but wait for Dan to look over this suggestion and you two can pick the one
you're happier with.  Sorry for the review delay!

[David Trainor- moved to gerrit, 2015-10-19 15:17:00]

Oh and those methods I mentioned above would obviously take a Tab argument.

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

https://codereview.chromium.org/1405023002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenTest.java:205:
observer.onPageLoadFinished(getActivity().getActivityTab());
Should this be three different for loops?  We'll never get two different calls
to the same observer before notifying all observers of the current event.

[gone, 2015-10-19 17:41:46]

lgtm, if you break it into three for loops.

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

https://codereview.chromium.org/1405023002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenTest.java:205:
observer.onPageLoadFinished(getActivity().getActivityTab());
On 2015/10/19 15:17:00, David Trainor wrote:
> Should this be three different for loops?  We'll never get two different calls
> to the same observer before notifying all observers of the current event.

Yeah, three for loops.  One of the pitfalls of exposing the observers directly,
I suppose.

https://codereview.chromium.org/1405023002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenThemeColorTest.java
(right):

https://codereview.chromium.org/1405023002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenThemeColorTest.java:48:
public void testThemeColorNotUsedIfPagesHasOne() {
Just make the test throw an exception.  It's the standard pattern for all the
other ones.

[mlamouri (slow - plz ping), 2015-10-19 19:14:24]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-10-19 19:14:24]

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

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

Comments applied and https://crbug.com/545111 to implement TabTestUtils. I don't
want to block this on that because we delayed these tests for long enough
already.

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

https://codereview.chromium.org/1405023002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenTest.java:205:
observer.onPageLoadFinished(getActivity().getActivityTab());
On 2015/10/19 at 17:41:46, dfalcantara wrote:
> On 2015/10/19 15:17:00, David Trainor wrote:
> > Should this be three different for loops?  We'll never get two different
calls
> > to the same observer before notifying all observers of the current event.
> 
> Yeah, three for loops.  One of the pitfalls of exposing the observers
directly, I suppose.

Done.

https://codereview.chromium.org/1405023002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenThemeColorTest.java
(right):

https://codereview.chromium.org/1405023002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappSplashScreenThemeColorTest.java:48:
public void testThemeColorNotUsedIfPagesHasOne() {
On 2015/10/19 at 17:41:46, dfalcantara wrote:
> Just make the test throw an exception.  It's the standard pattern for all the
other ones.

Done.

[commit-bot: I haz the power, 2015-10-19 19:16:04]

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

[commit-bot: I haz the power, 2015-10-19 20:10:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-19 20:11:28]

Patchset 3 (id:??) landed as
https://crrev.com/72504ef40ebaaed432d9bc864c482bfeaa415482
Cr-Commit-Position: refs/heads/master@{#354844}