Stop showing page actions in the location bar with redesign enabled

Stop showing page actions in the location bar with redesign enabled

No longer show page actions in the location bar iff the
extension action redesign switch is enabled. In addition,
make all extensions tests* pass with the switch on! Woot!

*This is based on running these tests locally.  Once
https://codereview.chromium.org/489183005/ goes through,
I can push a run through the trybots with the switch on
by default for kicks and grins.

BUG=397259
BUG=408261
Committed: https://crrev.com/6e7e5edc6f6d817ed219a5683b6829291969d2a8
Cr-Commit-Position: refs/heads/master@{#292648}

[Devlin, 2014-08-27 23:05:14]

Patchset #1 (id:1) has been deleted

[Devlin, 2014-08-27 23:06:04]

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

[Devlin, 2014-08-27 23:06:04]

Woohoo!  Getting close.

[Finnur, 2014-08-28 11:35:35]

I like it!

LGTM if the OnExtensionUnloaded question is "we don't need to worry".

https://codereview.chromium.org/516633002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_action_test_util.cc (right):

https://codereview.chromium.org/516633002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_action_test_util.cc:46:
page_actions.push_back(extension_action);
Isn't it more efficient (and clearer) to check visibility here (if
|only_count_visible| is true) and return page_actions.size() afterwards?

And then move all code below us in this function into the if clause (to line
35).

https://codereview.chromium.org/516633002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_action_test_util.h (right):

https://codereview.chromium.org/516633002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_action_test_util.h:17: // TODO(devlin):
Should we also pull out methods to test browser actions?
I'll let you decide if and when. It kind of makes sense to group them together,
and I'd say absolutely go for it if it reduces complexity.

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

https://codereview.chromium.org/516633002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:48: action =
active_script_controller_->GetActionForExtension(iter->get());
This of course also suppresses active scripts icons, right?
What's the plan for that? I thought we were removing them altogether?

https://codereview.chromium.org/516633002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:65:
active_script_controller_->OnExtensionUnloaded(extension);
Now that we're not adding ourselves as an observer, will the lack of calling
this cause problems?

https://codereview.chromium.org/516633002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:75: }  // namespace
extensions
It looks like we're not far off from killing this class altogether...

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

https://codereview.chromium.org/516633002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_action_test_util.cc (right):

https://codereview.chromium.org/516633002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_action_test_util.cc:46:
page_actions.push_back(extension_action);
On 2014/08/28 11:35:34, Finnur wrote:
> Isn't it more efficient (and clearer) to check visibility here (if
> |only_count_visible| is true) and return page_actions.size() afterwards?
> 
> And then move all code below us in this function into the if clause (to line
> 35).

I was torn (and originally had it that way).  The downside is that then we have
to check visibility in two places, rather than having the same approach for when
the switch is on/off.

I'm not too worried about performance (only used in testing), but if you think
it's less readable this way, definitely worth changing.
Done.

https://codereview.chromium.org/516633002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_action_test_util.h (right):

https://codereview.chromium.org/516633002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_action_test_util.h:17: // TODO(devlin):
Should we also pull out methods to test browser actions?
On 2014/08/28 11:35:34, Finnur wrote:
> I'll let you decide if and when. It kind of makes sense to group them
together,
> and I'd say absolutely go for it if it reduces complexity.

Acknowledged.

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

https://codereview.chromium.org/516633002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:48: action =
active_script_controller_->GetActionForExtension(iter->get());
On 2014/08/28 11:35:34, Finnur wrote:
> This of course also suppresses active scripts icons, right?
> What's the plan for that? I thought we were removing them altogether?

Active script icons aren't being removed, but they are still behind a flag.
And, if the redesign flag is on and the active script flag is on, every
extension already has an icon, so adding an icon here actually doesn't make a
difference.  An icon will still show up in the extension toolbar.

Of course, it's difficult to tell when the extension wants to act, but that's
already a discussion we're having. :)

https://codereview.chromium.org/516633002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:65:
active_script_controller_->OnExtensionUnloaded(extension);
On 2014/08/28 11:35:34, Finnur wrote:
> Now that we're not adding ourselves as an observer, will the lack of calling
> this cause problems?

Ah, yes, we will.  Fixed it by making ASC an observer, too.  It was always a
slight hack to just forward the notifications anyway.

https://codereview.chromium.org/516633002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:75: }  // namespace
extensions
On 2014/08/28 11:35:35, Finnur wrote:
> It looks like we're not far off from killing this class altogether...

Yep! :)

[Devlin, 2014-08-28 18:10:09]

Patchset #2 (id:40001) has been deleted

[Devlin, 2014-08-28 22:35:33]

Patchset #3 (id:80001) has been deleted

[Devlin, 2014-08-29 00:04:54]

Patchset #3 (id:100001) has been deleted

[Finnur, 2014-08-29 12:10:41]

Still LGTM.

https://codereview.chromium.org/516633002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_action_test_util.cc (right):

https://codereview.chromium.org/516633002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_action_test_util.cc:46:
page_actions.push_back(extension_action);
Yeah, I think this is a little bit better.

[Devlin, 2014-08-29 14:17:06]

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

[commit-bot: I haz the power, 2014-08-29 14:17:36]

CQ is trying da patch. Follow status at

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

[commit-bot: I haz the power, 2014-08-29 15:40:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[commit-bot: I haz the power, 2014-08-29 16:23:29]

Committed patchset #3 (id:120001) as 1b46eb572f689e8f7e9f4d35c13b78da4ac52d30

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

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