Add LocationBar PageAction tests

Add LocationBar PageAction tests

Page actions in the location bar have been somewhat loosely tested as a side-
effect of the page actions api testing, but (near as I can tell), we never
explicity test the ui for page actions.

This is a problem because the extensions tests shouldn't care about the UI, and
soon won't, since where the page actions are in the UI may change (i.e.,
location bar vs toolbar).

Fix this by adding a few basic UI tests for page actions in the location bar.

BUG=408261
Committed: https://crrev.com/a75ac029b21ff4162860eaba3be63a2c651a2a25
Cr-Commit-Position: refs/heads/master@{#292957}

[Devlin, 2014-08-27 23:07:35]

Patchset #1 (id:1) has been deleted

[Devlin, 2014-08-27 23:10:40]

rdevlin.cronin@chromium.org changed reviewers:
+ finnur@chromium.org, sky@chromium.org

[Devlin, 2014-08-27 23:10:40]

Finnur and Scott, please take a look when you can. :)

(Follows https://codereview.chromium.org/516633002/ for removing location bar
page actions, but shouldn't be needed for review.)

[sky, 2014-08-28 00:03:05]

https://codereview.chromium.org/512693003/diff/20001/chrome/browser/ui/locati...
File chrome/browser/ui/location_bar/location_bar_browsertest.cc (right):

https://codereview.chromium.org/512693003/diff/20001/chrome/browser/ui/locati...
chrome/browser/ui/location_bar/location_bar_browsertest.cc:17: #include
"chrome/browser/ui/location_bar/location_bar.h"
nit: this should be your first include (same order as location_bar.cc).

https://codereview.chromium.org/512693003/diff/20001/chrome/browser/ui/locati...
chrome/browser/ui/location_bar/location_bar_browsertest.cc:46: const
extensions::Extension* LoadPageActionExtension(
AFAICT this doesn't need any state from 'this'. I would turn this into a
non-member function and remove the subclass of ExtensionBrowserTest.

https://codereview.chromium.org/512693003/diff/20001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/512693003/diff/20001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:1463:
'browser/ui/location_bar/location_bar_browsertest.cc',
nit: sort.

[Devlin, 2014-08-28 05:30:17]

(Will respond to nits tomorrow morning)

https://codereview.chromium.org/512693003/diff/20001/chrome/browser/ui/locati...
File chrome/browser/ui/location_bar/location_bar_browsertest.cc (right):

https://codereview.chromium.org/512693003/diff/20001/chrome/browser/ui/locati...
chrome/browser/ui/location_bar/location_bar_browsertest.cc:46: const
extensions::Extension* LoadPageActionExtension(
On 2014/08/28 00:03:05, sky wrote:
> AFAICT this doesn't need any state from 'this'. I would turn this into a
> non-member function and remove the subclass of ExtensionBrowserTest.

Unfortunately, it needs it to do LoadExtension() on line 58 (a method in
ExtensionBrowserTest).  I originally had it as a vanilla ExtensionBrowserTest,
but the amount of setup to load an extension made this version a bit more
readable, and less prone to errors.

[Finnur, 2014-08-28 11:42:04]

LGTM

https://codereview.chromium.org/512693003/diff/20001/chrome/browser/ui/locati...
File chrome/browser/ui/location_bar/location_bar_browsertest.cc (right):

https://codereview.chromium.org/512693003/diff/20001/chrome/browser/ui/locati...
chrome/browser/ui/location_bar/location_bar_browsertest.cc:17: #include
"chrome/browser/ui/location_bar/location_bar.h"
Is that a recent rule change?

[sky, 2014-08-28 16:29:21]

Thanks for looking, LGTM

[Devlin, 2014-08-28 18:18:01]

https://codereview.chromium.org/512693003/diff/20001/chrome/browser/ui/locati...
File chrome/browser/ui/location_bar/location_bar_browsertest.cc (right):

https://codereview.chromium.org/512693003/diff/20001/chrome/browser/ui/locati...
chrome/browser/ui/location_bar/location_bar_browsertest.cc:17: #include
"chrome/browser/ui/location_bar/location_bar.h"
On 2014/08/28 11:42:04, Finnur wrote:
> Is that a recent rule change?

I think it's actually old, but never really standardized (and for awhile, the
presubmit include check didn't allow it).  But I'm happy to do it if it's
standard for ui code. :)

Done.

https://codereview.chromium.org/512693003/diff/20001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/512693003/diff/20001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:1463:
'browser/ui/location_bar/location_bar_browsertest.cc',
On 2014/08/28 00:03:05, sky wrote:
> nit: sort.

d'oh.  Done.

[Devlin, 2014-09-02 17:05:20]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-09-02 17:06:41]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/512693003/...

[commit-bot: I haz the power, 2014-09-02 18:05:49]

Committed patchset #4 (id:80001) as 0abaca08e99fa6671758a96829b99aa941696e41

[jam, 2014-09-04 03:25:04]

A revert of this CL (patchset #4 id:80001) has been created in
https://codereview.chromium.org/537813002/ by jam@chromium.org.

The reason for reverting is: Flaky test
see the following runs where the test fails even with 3 retries

http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...
.

[commit-bot: I haz the power, 2014-09-10 03:19:25]

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