[Extensions Toolbar] Add a finch config for the redesign

[Extensions Toolbar] Add a finch config for the redesign

Add an entry in field trials for the extension action redesign, and
fix any tests that fail with it as a default.

BUG=533101
TBR=avi@chromium.org for micro cocoa change

Committed: https://crrev.com/ca056fb34f9003262f28061856042ed87ed865c4
Cr-Commit-Position: refs/heads/master@{#350296}

[Devlin, 2015-09-21 23:16:38]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-09-21 23:21:32]

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

[Devlin, 2015-09-21 23:28:15]

Finnur, mind taking a look? :)

I've commented some of the changes that are more confusing, since they're just
changed as the result of a test that failed (but without seeing the test
failure, they may seem odd).

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc
(right):

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc:659:
FeatureSwitch::ScopedOverride override_toolbar_redesign(
This first block is identical to the one of the left, except for the feature
switch here.

The second block is also identical, except for the feature switch and different
"expects".

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_action.cc (right):

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_action.cc:264: placeholder_icon_image_ =
This was necessary because of a test that re-fetching the icon doesn't send back
a different instance.  I'm not sure it really matters, but I guess this has the
potential to be slightly faster.  The "mutable" in the .h is sad, but not
unreasonable or unprecedented, and taking the "const" off this method makes a
lot of other things blow up.

We could also deal with the churn and take the const off, decide we don't mind
the regeneration (alter the test), or initialize these on construction.  Lemme
know your preference. :)

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/lazy_background_page_apitest.cc (right):

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/lazy_background_page_apitest.cc:192:
extension_action_test_util::GetVisiblePageActionCount(
This handy method checks for page actions in the right place. I must have missed
this call when I made it and updated most of them.

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/location_bar_controller.cc (right):

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/location_bar_controller.cc:102: if
(UIOverrides::RemovesBookmarkButton(extension)) {
We need to do this part, even if the page actions aren't shown in the omnibox,
because it affects the star's visibility.

[Finnur, 2015-09-22 14:18:32]

LGTM, with nits and question mainly just for you to ponder.

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc
(right):

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc:659:
FeatureSwitch::ScopedOverride override_toolbar_redesign(
I probably would have created a helper function instead, but since this clause
goes away when the switch goes away, this is probably fine for now.

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_action.h (right):

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_action.h:292: // extension doesn't have its
own icon. (Mutable to allow lazy init.)
... to allow lazy init in GetDefaultIconImage.

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/browser_actions_bar_browsertest.h (right):

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/browser_actions_bar_browsertest.h:60:
scoped_refptr<const extensions::Extension> extension_c_;
Do all these need to be protected as well?

[Devlin, 2015-09-23 00:04:25]

Patchset #2 (id:40001) has been deleted

[Devlin, 2015-09-23 00:28:35]

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/extensio...
File
chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc
(right):

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc:659:
FeatureSwitch::ScopedOverride override_toolbar_redesign(
On 2015/09/22 14:18:32, Finnur wrote:
> I probably would have created a helper function instead, but since this clause
> goes away when the switch goes away, this is probably fine for now.

I was thinking about it, and there were enough differences (the switch,
reloading vs loading the extension, the test asserts, etc) that it really
wouldn't have saved much, and, like you said, it'll go away once the switch is
default.

But I can clean up one of the really ugly repeated bits, which is the hard-coded
script.  Pulled that out into a variable to make it a little nicer.

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_action.h (right):

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_action.h:292: // extension doesn't have its
own icon. (Mutable to allow lazy init.)
On 2015/09/22 14:18:32, Finnur wrote:
> ... to allow lazy init in GetDefaultIconImage.

Done.

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/browser_actions_bar_browsertest.h (right):

https://codereview.chromium.org/1363463002/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/browser_actions_bar_browsertest.h:60:
scoped_refptr<const extensions::Extension> extension_c_;
On 2015/09/22 14:18:32, Finnur wrote:
> Do all these need to be protected as well?

Probably not. :)  Done.

[Devlin, 2015-09-23 03:31:35]

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

[Devlin, 2015-09-23 03:31:35]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org
Link to the patchset: https://codereview.chromium.org/1363463002/#ps60001
(title: "Finnur's")

[Devlin, 2015-09-23 03:31:37]

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

[Devlin, 2015-09-23 03:32:04]

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

[commit-bot: I haz the power, 2015-09-23 03:33:53]

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

[commit-bot: I haz the power, 2015-09-23 04:04:52]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2015-09-23 04:06:07]

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

[Nico, 2015-09-23 12:36:58]

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

[Nico, 2015-09-23 12:36:59]

These tests seem to fail in buildtype=Official builds:
http://build.chromium.org/p/chromium.fyi/builders/CrWinClang%20tester/builds/...
(that bot also happens to use clang, but more importantly it does official
builds. Almost always when it's broken it's due to tests being broken in
official builds.)

Please take a look.

[huangs, 2015-09-23 13:36:16]

I'm thinking of reverting this.

[huangs, 2015-09-23 13:46:42]

A revert of this CL (patchset #2 id:60001) has been created in
https://codereview.chromium.org/1363943002/ by huangs@chromium.org.

The reason for reverting is: This is causing time out for XP Tests:
browser_tests: DeclarativeContentApiTest.ShowPageActionWithoutPageAction..