Only return the visible page actions from PageActionController.

Relanding after reverted in 137632 (broke ASAN).

Only return the visible page actions from PageActionController.

This is a bit of a yak shave to support having the "action box" show active
extensions on the current page rather than page actions. Previously,
PageActionController was creating widgets for everything then
LocationBarViewGtk was hiding them as necessary. This is assumes too much; we
need to push the visibility logic into the Controller for the future.

BUG=127988
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137630

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137638

[not at google - send to devlin, 2012-05-16 08:25:52]

mpcomplete: extensions etc
estade: gtk stuff

https://chromiumcodereview.appspot.com/10388160/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/action_box_controller.h (left):

https://chromiumcodereview.appspot.com/10388160/diff/1/chrome/browser/extensi...
chrome/browser/extensions/action_box_controller.h:21: // UI decoration on a page
box item.
I got news today that we probably won't need to change the decoration on the
page actions, so removing this until then.

[not at google - send to devlin, 2012-05-16 08:31:53]

avi, I was planning on TBR'ing you since it's so insignificant to
TabContentsWrapper but you're on the CC list anyway so added you as a reviewer
for that.

Further notes:

Most of the complication of this patch came from the way that the
extension-installed-previewing flow happens, where the GTK code assumes that
things are visible blah blah. I've changed it so that all the flow passes
through PageActionController.

(As before, I will update the views and cocao implementations of this in a
follow-up and address that TODO in the code; I'm admittedly holding off on that
a bit until I'm sure that all the machinery is in place to implement this, but I
really need to do it soon before they diverge too much).

[Avi (use Gerrit), 2012-05-16 14:37:35]

tcw lgtm

[Evan Stade, 2012-05-16 21:58:16]

gtk lgtm with nits

http://codereview.chromium.org/10388160/diff/1/chrome/browser/ui/gtk/location...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/10388160/diff/1/chrome/browser/ui/gtk/location...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:812: UpdatePageActions();
too much indent

http://codereview.chromium.org/10388160/diff/1/chrome/browser/ui/gtk/location...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1603: std::string icon_path =
(icon_index < 0) ?
no parens

http://codereview.chromium.org/10388160/diff/1/chrome/browser/ui/gtk/location...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1614:
gtk_image_set_from_pixbuf(GTK_IMAGE(image_.get()), pixbuf);
please add some more vertical space somewhere as this is a huge block of code
with no empty lines

[Matt Perry, 2012-05-17 00:17:23]

lgtm

https://chromiumcodereview.appspot.com/10388160/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/action_box_controller.cc (right):

https://chromiumcodereview.appspot.com/10388160/diff/1/chrome/browser/extensi...
chrome/browser/extensions/action_box_controller.cc:14:
std::vector<ExtensionAction*>* out) {
std::set_union(actions.begin(), actions.end(), out->begin(), out->end(),
std::back_inserter(*out));

https://chromiumcodereview.appspot.com/10388160/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/page_action_controller.cc (right):

https://chromiumcodereview.appspot.com/10388160/diff/1/chrome/browser/extensi...
chrome/browser/extensions/page_action_controller.cc:26:
PageActionController::~PageActionController() {}
RemoveObserver here

https://chromiumcodereview.appspot.com/10388160/diff/1/chrome/browser/ui/gtk/...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10388160/diff/1/chrome/browser/ui/gtk/...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:451:
preview_enabled_actions_.insert(page_action);
std::set allows you to test and set simultaneously. You could restructure this
as

if (preview_enabled && insert(page_action).second)
  update();
else if (!preview_enabled && erase(page_action) > 0)
  update();

[not at google - send to devlin, 2012-05-17 01:30:34]

http://codereview.chromium.org/10388160/diff/1/chrome/browser/extensions/acti...
File chrome/browser/extensions/action_box_controller.cc (right):

http://codereview.chromium.org/10388160/diff/1/chrome/browser/extensions/acti...
chrome/browser/extensions/action_box_controller.cc:14:
std::vector<ExtensionAction*>* out) {
On 2012/05/17 00:17:24, Matt Perry wrote:
> std::set_union(actions.begin(), actions.end(), out->begin(), out->end(),
> std::back_inserter(*out));

nice

http://codereview.chromium.org/10388160/diff/1/chrome/browser/extensions/page...
File chrome/browser/extensions/page_action_controller.cc (right):

http://codereview.chromium.org/10388160/diff/1/chrome/browser/extensions/page...
chrome/browser/extensions/page_action_controller.cc:26:
PageActionController::~PageActionController() {}
On 2012/05/17 00:17:24, Matt Perry wrote:
> RemoveObserver here

Done.

http://codereview.chromium.org/10388160/diff/1/chrome/browser/ui/gtk/location...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/10388160/diff/1/chrome/browser/ui/gtk/location...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:451:
preview_enabled_actions_.insert(page_action);
On 2012/05/17 00:17:24, Matt Perry wrote:
> std::set allows you to test and set simultaneously. You could restructure this
> as
> 
> if (preview_enabled && insert(page_action).second)
>   update();
> else if (!preview_enabled && erase(page_action) > 0)
>   update();

Done.

http://codereview.chromium.org/10388160/diff/1/chrome/browser/ui/gtk/location...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:812: UpdatePageActions();
On 2012/05/16 21:58:17, Evan Stade wrote:
> too much indent

Done.

http://codereview.chromium.org/10388160/diff/1/chrome/browser/ui/gtk/location...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1603: std::string icon_path =
(icon_index < 0) ?
On 2012/05/16 21:58:17, Evan Stade wrote:
> no parens

Done.

http://codereview.chromium.org/10388160/diff/1/chrome/browser/ui/gtk/location...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1614:
gtk_image_set_from_pixbuf(GTK_IMAGE(image_.get()), pixbuf);
On 2012/05/16 21:58:17, Evan Stade wrote:
> please add some more vertical space somewhere as this is a huge block of code
> with no empty lines

Done.

[commit-bot: I haz the power, 2012-05-17 01:31:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10388160/10002

[commit-bot: I haz the power, 2012-05-17 03:12:46]

Try job failure for 10388160-10002 (retry) (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[not at google - send to devlin, 2012-05-17 03:30:34]

Made tests pass again, had to fix ownership issue by moving ScriptExecutor from
TabContentsWrapper to ExtensionTabHelper. Oh well.

http://codereview.chromium.org/10388160/diff/2004/chrome/browser/extensions/a...
File chrome/browser/extensions/action_box_controller.cc (right):

http://codereview.chromium.org/10388160/diff/2004/chrome/browser/extensions/a...
chrome/browser/extensions/action_box_controller.cc:20: }
The cute thing there didn't work, don't know why, the stl stuff is pretty
voodoo-ish though.

[commit-bot: I haz the power, 2012-05-17 03:30:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10388160/12003

[commit-bot: I haz the power, 2012-05-17 04:47:48]

Change committed as 137630

[commit-bot: I haz the power, 2012-05-17 05:36:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10388160/3004

[commit-bot: I haz the power, 2012-05-17 06:56:50]

Change committed as 137638