Clean up NOTIFICATION_EXTENSION_PAGE_ACTION_[ COUNT | VISIBILITY ]_CHANGED

Clean up NOTIFICATION_EXTENSION_PAGE_ACTION_[ COUNT | VISIBILITY ]_CHANGED

Remove the unneeded PAGE_ACTION_COUNT_CHANGED notification.
Replace the PAGE_ACTION_VISIBILITY_CHANGED notification with a
PAGE_ACTIONS_UPDATED notification, which is sent once per
update (instead of once per page action).
Added bonuses:
- consistent notification behavior across cocoa/views
- less cruft in ui code

BUG=405356
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291097

[Devlin, 2014-08-20 00:26:13]

All, please take a look.
finnur@ - extensions
sky@ - views
avi@ - cocoa

Cheers! :)

[Avi (use Gerrit), 2014-08-20 01:06:53]

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

https://codereview.chromium.org/475813004/diff/20001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:95:
content::NotificationService::NoDetails());
We're trying to kill off notifications. Overall this is an improvement, I
suppose, but can we move to some kind of registration? Can we have the
controller use an observer or callback interface? (base/observer_list.h or
base/callback_list.h)

[Devlin, 2014-08-20 03:21:00]

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

https://codereview.chromium.org/475813004/diff/20001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:95:
content::NotificationService::NoDetails());
On 2014/08/20 01:06:52, Avi wrote:
> We're trying to kill off notifications. Overall this is an improvement, I
> suppose, but can we move to some kind of registration? Can we have the
> controller use an observer or callback interface? (base/observer_list.h or
> base/callback_list.h)

TL;DR: That's the eventual goal, but I'd like to push this through in the
meantime (for reasons described in the bug and CL description, I think it's
still a big improvement).

Yeah, I know we're trying to kill off notifications...but... ;)

It's not a simple change like just making the LocationBarController have a
subscription/observer list because there are a bunch of different
LocationBarControllers (one per web contents), and not everything that cares
about PAGE_ACTION_VISIBILITY_CHANGED (old)/PAGE_ACTIONS_UPDATED (new) wants to
track each new tab created.

Eventually, I *would* like to move this to be an observer method on
ExtensionActionAPI::Observer (newly-added to take the place of other
notifications), but I want to hold off because:
a) I want to keep these patches close to the original implementation, since it's
juggling a lot of different parts.
b) I'd like to keep these patches small for ease comparing old vs new behavior.

In the meantime, I'd like to push this through so we have something to work off,
and because this change is still a step in the right direction towards moving
this towards an observer method.

Okay, done defending. :)  If you still would prefer me to move it all in this
patch, I can.  Just wanted to explain my reasoning, and assure you that it
wasn't gonna stay like this indefinitely.

[Avi (use Gerrit), 2014-08-20 03:52:27]

OK, gotcha.

This LGTM.

[Finnur, 2014-08-20 12:48:04]

LGTM

> We're trying to kill off notifications.

And, with this CL he's killing off one notification already. :)

[sky, 2014-08-20 15:45:30]

https://codereview.chromium.org/475813004/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/475813004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1203: changed =
old_visibility != (*i)->visible() ? true : changed;
Tricky, but isn't:
changed = old_visibility != (*i)->visible()
the same thing? Although don't you want to return true if any element changed,
not just the last? So maybe:
changed |= old_visiblity != (*i)->visible();
?

[Devlin, 2014-08-20 15:54:49]

https://codereview.chromium.org/475813004/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/475813004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1203: changed =
old_visibility != (*i)->visible() ? true : changed;
On 2014/08/20 15:45:30, sky wrote:
> Tricky, but isn't:
> changed = old_visibility != (*i)->visible()
> the same thing? Although don't you want to return true if any element changed,
> not just the last? So maybe:
> changed |= old_visiblity != (*i)->visible();
> ?

Right; changed = old_visibility != (*i)->visible() only accounts for the last
(and we want if there's any chnage).  But the changed |= ... will work, and is
perhaps slightly more elegant. :)  Done.

[sky, 2014-08-20 16:00:14]

LGTM

[Devlin, 2014-08-20 21:42:47]

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

[commit-bot: I haz the power, 2014-08-20 21:45:21]

CQ is trying da patch. Follow status at

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

[commit-bot: I haz the power, 2014-08-20 23:11:56]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-20 23:15:37]

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

[commit-bot: I haz the power, 2014-08-20 23:15:38]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[Devlin, 2014-08-20 23:29:59]

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

[commit-bot: I haz the power, 2014-08-20 23:31:46]

CQ is trying da patch. Follow status at

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

[commit-bot: I haz the power, 2014-08-20 23:50:42]

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

[commit-bot: I haz the power, 2014-08-20 23:54:39]

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

[commit-bot: I haz the power, 2014-08-20 23:54:40]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[Devlin, 2014-08-21 14:07:47]

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

[commit-bot: I haz the power, 2014-08-21 14:08:13]

CQ is trying da patch. Follow status at

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

[commit-bot: I haz the power, 2014-08-21 16:46:17]

Committed patchset #4 (100001) as 291097