Add some integration tests for the search geolocation disclosure.

Add some integration tests for the search geolocation disclosure.

This adds some basic java tests for the new disclosure UI.

BUG=675858
Committed: https://crrev.com/0cdef7fe3c10147a42ad28e45b0f7bb5c274d973
Cr-Commit-Position: refs/heads/master@{#441309}

[benwells, 2016-12-22 05:56:25]

The CQ bit was checked by benwells@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 05:56:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 07:00:57]

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

[commit-bot: I haz the power, 2016-12-22 07:00:57]

Dry run: This issue passed the CQ dry run.

[benwells, 2016-12-28 02:53:51]

The CQ bit was checked by benwells@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-28 02:54:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-28 04:39:22]

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

[commit-bot: I haz the power, 2016-12-28 04:39:22]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[benwells, 2016-12-28 04:44:08]

The CQ bit was checked by benwells@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-28 04:44:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-28 05:42:48]

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

[commit-bot: I haz the power, 2016-12-28 05:42:49]

Dry run: This issue passed the CQ dry run.

[benwells, 2016-12-29 01:42:00]

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

[benwells, 2016-12-29 01:42:01]

Maybe with these tests we don't need the other legal-disclosure-infobar tests.

https://codereview.chromium.org/2593393002/diff/1/chrome/browser/android/sear...
File chrome/browser/android/search_geolocation_disclosure_tab_helper.cc (left):

https://codereview.chromium.org/2593393002/diff/1/chrome/browser/android/sear...
chrome/browser/android/search_geolocation_disclosure_tab_helper.cc:116: //
enough times already.
Losing this comment was a merge error; I'll put it back.

https://codereview.chromium.org/2593393002/diff/1/chrome/browser/android/sear...
File chrome/browser/android/search_geolocation_disclosure_tab_helper.cc (right):

https://codereview.chromium.org/2593393002/diff/1/chrome/browser/android/sear...
chrome/browser/android/search_geolocation_disclosure_tab_helper.cc:109: 
I'll also remove these empty lines, not sure how they crept in.

https://codereview.chromium.org/2593393002/diff/1/chrome/browser/android/sear...
chrome/browser/android/search_geolocation_disclosure_tab_helper.cc:226: bool
SearchGeolocationDisclosureTabHelper::Register(JNIEnv* env) {
I'll move this up in the file to be under RegisterProfilePrefs.

[benwells, 2016-12-29 01:50:14]

Oops, sorry I forgot I'd added those todos for myself. All fixed now.

https://codereview.chromium.org/2593393002/diff/1/chrome/browser/android/sear...
File chrome/browser/android/search_geolocation_disclosure_tab_helper.cc (left):

https://codereview.chromium.org/2593393002/diff/1/chrome/browser/android/sear...
chrome/browser/android/search_geolocation_disclosure_tab_helper.cc:116: //
enough times already.
On 2016/12/29 01:42:01, benwells wrote:
> Losing this comment was a merge error; I'll put it back.

Done.

https://codereview.chromium.org/2593393002/diff/1/chrome/browser/android/sear...
File chrome/browser/android/search_geolocation_disclosure_tab_helper.cc (right):

https://codereview.chromium.org/2593393002/diff/1/chrome/browser/android/sear...
chrome/browser/android/search_geolocation_disclosure_tab_helper.cc:109: 
On 2016/12/29 01:42:01, benwells wrote:
> I'll also remove these empty lines, not sure how they crept in.

Done.

https://codereview.chromium.org/2593393002/diff/1/chrome/browser/android/sear...
chrome/browser/android/search_geolocation_disclosure_tab_helper.cc:226: bool
SearchGeolocationDisclosureTabHelper::Register(JNIEnv* env) {
On 2016/12/29 01:42:01, benwells wrote:
> I'll move this up in the file to be under RegisterProfilePrefs.

Done.

[gone, 2017-01-03 18:28:31]

https://codereview.chromium.org/2593393002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/SearchGeolocationDisclosureTabHelper.java
(right):

https://codereview.chromium.org/2593393002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/SearchGeolocationDisclosureTabHelper.java:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
nit: Welp, guess it's 2017 now.  Yay holiday breaks!

https://codereview.chromium.org/2593393002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBarTest.java
(right):

https://codereview.chromium.org/2593393002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBarTest.java:43:

nit: Maybe remove the newline?

https://codereview.chromium.org/2593393002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBarTest.java:66:
listener.addInfoBarAnimationFinished("InfoBar not added.");
Should flip the ordering so that the infobar animation finishes before you check
if the infobar count is correct.

[benwells, 2017-01-03 23:37:55]

https://codereview.chromium.org/2593393002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBarTest.java
(right):

https://codereview.chromium.org/2593393002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBarTest.java:66:
listener.addInfoBarAnimationFinished("InfoBar not added.");
On 2017/01/03 18:28:31, dfalcantara (check queue) wrote:
> Should flip the ordering so that the infobar animation finishes before you
check
> if the infobar count is correct.

I did it in this order deliberately for a couple of reasons:
- it should work as the infobar list that is being checked is independent of the
animation. That is, the list being checked is the list of all infobars, not just
those that are laid out on screen. For this particular infobar it should be
added as part of the URL navigation process so it should be there immediately.
- if the above is wrong for some reason, then the other tests for an infobar not
being shown are invalid. E.g. the test on line 70 might pass even though
infobars have been added.

Does that make sense?

[benwells, 2017-01-03 23:38:53]

On 2017/01/03 23:37:55, benwells wrote:
>
https://codereview.chromium.org/2593393002/diff/80001/chrome/android/javatest...
> File
>
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBarTest.java
> (right):
> 
>
https://codereview.chromium.org/2593393002/diff/80001/chrome/android/javatest...
>
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBarTest.java:66:
> listener.addInfoBarAnimationFinished("InfoBar not added.");
> On 2017/01/03 18:28:31, dfalcantara (check queue) wrote:
> > Should flip the ordering so that the infobar animation finishes before you
> check
> > if the infobar count is correct.
> 
> I did it in this order deliberately for a couple of reasons:
> - it should work as the infobar list that is being checked is independent of
the
> animation. That is, the list being checked is the list of all infobars, not
just
> those that are laid out on screen. For this particular infobar it should be
> added as part of the URL navigation process so it should be there immediately.
> - if the above is wrong for some reason, then the other tests for an infobar
not
> being shown are invalid. E.g. the test on line 70 might pass even though
> infobars have been added.
> 
> Does that make sense?

Just to add to that, I did check that the loadURL function will wait until the
URL loading is complete, so any of these search geo infobars that will be shown
should be visible by the time it returns.

[gone, 2017-01-03 23:54:59]

On 2017/01/03 23:38:53, benwells wrote:
> On 2017/01/03 23:37:55, benwells wrote:
> >
>
https://codereview.chromium.org/2593393002/diff/80001/chrome/android/javatest...
> > File
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBarTest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2593393002/diff/80001/chrome/android/javatest...
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBarTest.java:66:
> > listener.addInfoBarAnimationFinished("InfoBar not added.");
> > On 2017/01/03 18:28:31, dfalcantara (check queue) wrote:
> > > Should flip the ordering so that the infobar animation finishes before you
> > check
> > > if the infobar count is correct.
> > 
> > I did it in this order deliberately for a couple of reasons:
> > - it should work as the infobar list that is being checked is independent of
> the
> > animation. That is, the list being checked is the list of all infobars, not
> just
> > those that are laid out on screen. For this particular infobar it should be
> > added as part of the URL navigation process so it should be there
immediately.
> > - if the above is wrong for some reason, then the other tests for an infobar
> not
> > being shown are invalid. E.g. the test on line 70 might pass even though
> > infobars have been added.
> > 
> > Does that make sense?
> 
> Just to add to that, I did check that the loadURL function will wait until the
> URL loading is complete, so any of these search geo infobars that will be
shown
> should be visible by the time it returns.

Well the load might be done, but the animation might be dog slow.  I'm coming
from the
angle that tests that have this line combo normally have to wait until the
infobar is
done animating so the user can click on one of its controls, which is what the
infobar
tests normally do.  Given that your infobar doesn't really check for any
interaction,
you're probably fine.

lgtm, but keep that in mind if you ever need to test dismissing the thing.

[gone, 2017-01-03 23:55:42]

(er lgtm % nits)

[benwells, 2017-01-03 23:57:44]

On 2017/01/03 23:54:59, dfalcantara (check queue) wrote:
> On 2017/01/03 23:38:53, benwells wrote:
> > On 2017/01/03 23:37:55, benwells wrote:
> > >
> >
>
https://codereview.chromium.org/2593393002/diff/80001/chrome/android/javatest...
> > > File
> > >
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBarTest.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2593393002/diff/80001/chrome/android/javatest...
> > >
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/infobar/SearchGeolocationDisclosureInfoBarTest.java:66:
> > > listener.addInfoBarAnimationFinished("InfoBar not added.");
> > > On 2017/01/03 18:28:31, dfalcantara (check queue) wrote:
> > > > Should flip the ordering so that the infobar animation finishes before
you
> > > check
> > > > if the infobar count is correct.
> > > 
> > > I did it in this order deliberately for a couple of reasons:
> > > - it should work as the infobar list that is being checked is independent
of
> > the
> > > animation. That is, the list being checked is the list of all infobars,
not
> > just
> > > those that are laid out on screen. For this particular infobar it should
be
> > > added as part of the URL navigation process so it should be there
> immediately.
> > > - if the above is wrong for some reason, then the other tests for an
infobar
> > not
> > > being shown are invalid. E.g. the test on line 70 might pass even though
> > > infobars have been added.
> > > 
> > > Does that make sense?
> > 
> > Just to add to that, I did check that the loadURL function will wait until
the
> > URL loading is complete, so any of these search geo infobars that will be
> shown
> > should be visible by the time it returns.
> 
> Well the load might be done, but the animation might be dog slow.  I'm coming
> from the
> angle that tests that have this line combo normally have to wait until the
> infobar is
> done animating so the user can click on one of its controls, which is what the
> infobar
> tests normally do.  Given that your infobar doesn't really check for any
> interaction,
> you're probably fine.
> 
> lgtm, but keep that in mind if you ever need to test dismissing the thing.

Cool, thanks. Good points about how these tests normally work, I'll add a
comment explaining why the ordering in this test is different to other tests.

[benwells, 2017-01-04 02:24:50]

The CQ bit was checked by benwells@chromium.org

[benwells, 2017-01-04 02:24:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2593393002/#ps120001
(title: "Nits and a comment")

[commit-bot: I haz the power, 2017-01-04 02:25:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 03:28:07]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1483496690482990,
"parent_rev": "047ba6496b5298e533ff1621e292ef47d8e605c3", "commit_rev":
"c6dd81ec85b2fb0bfaa3769926f38094f24b7fa0"}

[commit-bot: I haz the power, 2017-01-04 03:28:35]

Description was changed from

==========
Add some integration tests for the search geolocation disclosure.

This adds some basic java tests for the new disclosure UI.

BUG=675858
==========

to

==========
Add some integration tests for the search geolocation disclosure.

This adds some basic java tests for the new disclosure UI.

BUG=675858

Review-Url: https://codereview.chromium.org/2593393002
==========

[commit-bot: I haz the power, 2017-01-04 03:28:36]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2017-01-04 03:30:24]

Description was changed from

==========
Add some integration tests for the search geolocation disclosure.

This adds some basic java tests for the new disclosure UI.

BUG=675858

Review-Url: https://codereview.chromium.org/2593393002
==========

to

==========
Add some integration tests for the search geolocation disclosure.

This adds some basic java tests for the new disclosure UI.

BUG=675858

Committed: https://crrev.com/0cdef7fe3c10147a42ad28e45b0f7bb5c274d973
Cr-Commit-Position: refs/heads/master@{#441309}
==========

[commit-bot: I haz the power, 2017-01-04 03:30:25]

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

[dvadym, 2017-01-04 11:50:33]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2615563004/ by dvadym@chromium.org.

The reason for reverting is: Test 
org.chromium.chrome.browser.infobar.SearchGeolocationDisclosureInfoBarTest#testInfoBarAppears
fails,
https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test....