Support chrome.windows extension API events for browserless Panels.

Support chrome.windows extension API events for browserless Panels.

Modified window_open/panel test to check for chrome.windows.onCreated event.

BUG=127323
TEST=Verified against WindowOpenApiTest.WindowOpen* using --browserless-panels flag


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

[jennb, 2012-07-13 00:56:57]

jam - could you do an OWNERS review for chrome_notification_types.h ?
mihai - could you review the changes under chrome/browser/extensions?
jianli - overall review

Thanks!

[jam, 2012-07-13 15:36:10]

On 2012/07/13 00:56:57, jennb wrote:
> jam - could you do an OWNERS review for chrome_notification_types.h ?

no owners needed...

> mihai - could you review the changes under chrome/browser/extensions?
> jianli - overall review
> 
> Thanks!

[jianli, 2012-07-13 17:46:40]

http://codereview.chromium.org/10777004/diff/4002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_panel_event_router.cc (right):

http://codereview.chromium.org/10777004/diff/4002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_panel_event_router.cc:81:
extension_event_names::kOnWindowCreated, json_args, profile_, GURL());
Can you share the above code with the similar one in OnPanelClosed, i.e., by
introducing a helper function DispatchSimpleEvent?

http://codereview.chromium.org/10777004/diff/4002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10777004/diff/4002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:482: panel_event_router_->Init();
Can we delay create ExtensionPanelEventRouter till 1st panel is created?

[Dmitry Titov, 2012-07-13 18:24:17]

http://codereview.chromium.org/10777004/diff/4002/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_cocoa.mm (right):

http://codereview.chromium.org/10777004/diff/4002/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_cocoa.mm:59: panel_->OnActiveStateChanged(true);
oy. 
Can it be that the consumers of that call are/will assume that the window
already changed the status when it is called? If so, they can be broken.

Or, what if ActivatePanel actually starts to fail to activate a panel, but the
tests will succeed because we call OnActiveStateChanged directly here?

[jennb, 2012-07-13 18:47:48]

http://codereview.chromium.org/10777004/diff/4002/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_cocoa.mm (right):

http://codereview.chromium.org/10777004/diff/4002/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_cocoa.mm:59: panel_->OnActiveStateChanged(true);
On 2012/07/13 18:24:17, Dmitry Titov wrote:
> oy. 
> Can it be that the consumers of that call are/will assume that the window
> already changed the status when it is called? If so, they can be broken.
> 
> Or, what if ActivatePanel actually starts to fail to activate a panel, but the
> tests will succeed because we call OnActiveStateChanged directly here?

I had the same concerns, therefore I added the EXPECT_EQ in
test_panel_active_state_observer.cc to see if the state is actually changed even
though |-windowDidBecomeKey| has not been fired. All was happy.

[jennb, 2012-07-13 22:12:14]

Sorry, forgot to separate changes from syncing and my changes.

Per dimich's suggestion, changed to avoid using notifications to trigger window
extension events. Rather, Panel tells the ExtensionPanelEventRouter directly
about changes that result in extension events.

http://codereview.chromium.org/10777004/diff/4002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_panel_event_router.cc (right):

http://codereview.chromium.org/10777004/diff/4002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_panel_event_router.cc:81:
extension_event_names::kOnWindowCreated, json_args, profile_, GURL());
On 2012/07/13 17:46:41, jianli wrote:
> Can you share the above code with the similar one in OnPanelClosed, i.e., by
> introducing a helper function DispatchSimpleEvent?

Done.

http://codereview.chromium.org/10777004/diff/4002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10777004/diff/4002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:482: panel_event_router_->Init();
On 2012/07/13 17:46:41, jianli wrote:
> Can we delay create ExtensionPanelEventRouter till 1st panel is created?

Great suggestion. Will delay creation until router is needed.

[Dmitry Titov, 2012-07-14 03:33:48]

lgtm

[jennb, 2012-07-17 16:55:45]

Mihai - ping? Could I get an OWNERS review for chrome/browser/extensions? (Or
redirect me to someone else?)

Thanks,
Jenn

[jianli, 2012-07-17 18:44:15]

lgtm

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:1167:
switches::kBrowserlessPanels) && !panel_event_router_.get()) {
nit: It would be more efficient to check !panel_event_router_.get() first.

[Mihai Parparita -not on Chrome, 2012-07-19 20:14:57]

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
File chrome/browser/extensions/extension_panel_event_router.h (right):

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
chrome/browser/extensions/extension_panel_event_router.h:17: // The
ExtensinoPanelEventRouter sends Panel window events
Typo ("Extensino")

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
chrome/browser/extensions/extension_panel_event_router.h:19: class
ExtensionPanelEventRouter {
In light of http://crrev.com/147083, this perhaps should go in the extensions
namespace (then you can just call it PanelEventRouter).

Steven had stronger opinions on namespaces, I've cc-ed him on this review.

A more important question for Steven is whether ExtensionPanelEventRouter and
ExtensionBrowserEventRouter should share code, the way that window
implementations do (via BaseWindow). OnPanelActiveStatusChanged is complex
enough (and is nearly identical to OnBrowserSetLastActive) that I would prefer
to see either a BaseExtensionWindowEventRouter class that they both inherit
from, or a ExtensionWindowEventRouterHelper class that they use composition
with.

[jennb, 2012-07-20 02:44:18]

Thanks for the feedback. I'll move the new class into the extension namespace
but leave it ExtensionPanelEventRouter to be consistent with existing classes.
(Don't want to turn this CL into a rename-all-event-routers change.) I'll also
change to share code between browser and panel window event routing.

Will ping again after new patch is ready.

[stevenjb, 2012-07-20 16:54:33]

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
File chrome/browser/extensions/extension_panel_event_router.cc (right):

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
chrome/browser/extensions/extension_panel_event_router.cc:17: namespace events =
extension_event_names;
Ideally, extension_event_names should be extension::event_names, and this should
be in the extensions namespace, so we wouldn't need this. For now, I'd at least
use event_names since that ebtter describes the namespace (I got confused about
what, for example, kOnWindowCreated was).

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
chrome/browser/extensions/extension_panel_event_router.cc:52: // window_profile
is either this panel's profile's default profile, its
nit: "the panel profile's" or "the default profile for this panel's profile"
would be easier to read

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
chrome/browser/extensions/extension_panel_event_router.cc:56: focused_window_id_
= window_id;
Couldn't window_id still be kUnknownWindowId at this point? Is that valid?

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
File chrome/browser/extensions/extension_panel_event_router.h (right):

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
chrome/browser/extensions/extension_panel_event_router.h:19: class
ExtensionPanelEventRouter {
On 2012/07/19 20:14:58, Mihai Parparita wrote:
> In light of http://crrev.com/147083, this perhaps should go in the extensions
> namespace (then you can just call it PanelEventRouter).
> 
> Steven had stronger opinions on namespaces, I've cc-ed him on this review.
> 
> A more important question for Steven is whether ExtensionPanelEventRouter and
> ExtensionBrowserEventRouter should share code, the way that window
> implementations do (via BaseWindow). OnPanelActiveStatusChanged is complex
> enough (and is nearly identical to OnBrowserSetLastActive) that I would prefer
> to see either a BaseExtensionWindowEventRouter class that they both inherit
> from, or a ExtensionWindowEventRouterHelper class that they use composition
> with.

I think this should be extensions::PanelEventRouter. 

I looked through ExtensionBrowserEventRouter, and it really does look like that
could become WindowControllerEventRouter with all references to Browser replaced
with ExtensionWindowController. Then the same code should work for panels also.
I believe that BrowserList in BrowserEventRouter could be replaced with
WindowControllerList (which may need to have an observer list added) and also
work with both Browser and Panel.

I'm not sure we need to do that re-factoring here, but it does seem like it
would avoid a lot of duplicate code, which would definitely be preferable.

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:1164: ExtensionPanelEventRouter*
ExtensionService::GetPanelEventRouter() {
Shouldn't this be initialized (conditionally) in
ExtensionService::InitEventRouters() like everything else? Why the special
accessor?

[jennb, 2012-07-20 18:09:36]

Thanks for the WindowControllerList observer suggestion, Steven. See design
proposal using it below.

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
File chrome/browser/extensions/extension_panel_event_router.cc (right):

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
chrome/browser/extensions/extension_panel_event_router.cc:56: focused_window_id_
= window_id;
On 2012/07/20 16:54:33, stevenjb (chromium) wrote:
> Couldn't window_id still be kUnknownWindowId at this point? Is that valid?

Yes, that's valid and expected when the current focused window loses focus.

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
File chrome/browser/extensions/extension_panel_event_router.h (right):

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
chrome/browser/extensions/extension_panel_event_router.h:19: class
ExtensionPanelEventRouter {
On 2012/07/20 16:54:33, stevenjb (chromium) wrote:
> On 2012/07/19 20:14:58, Mihai Parparita wrote:
> > In light of http://crrev.com/147083, this perhaps should go in the
extensions
> > namespace (then you can just call it PanelEventRouter).
> > 
> > Steven had stronger opinions on namespaces, I've cc-ed him on this review.
> > 
> > A more important question for Steven is whether ExtensionPanelEventRouter
and
> > ExtensionBrowserEventRouter should share code, the way that window
> > implementations do (via BaseWindow). OnPanelActiveStatusChanged is complex
> > enough (and is nearly identical to OnBrowserSetLastActive) that I would
prefer
> > to see either a BaseExtensionWindowEventRouter class that they both inherit
> > from, or a ExtensionWindowEventRouterHelper class that they use composition
> > with.
> 
> I think this should be extensions::PanelEventRouter. 
> 
> I looked through ExtensionBrowserEventRouter, and it really does look like
that
> could become WindowControllerEventRouter with all references to Browser
replaced
> with ExtensionWindowController. Then the same code should work for panels
also.
> I believe that BrowserList in BrowserEventRouter could be replaced with
> WindowControllerList (which may need to have an observer list added) and also
> work with both Browser and Panel.
> 
> I'm not sure we need to do that re-factoring here, but it does seem like it
> would avoid a lot of duplicate code, which would definitely be preferable.
> 

I propose creating extension::WindowEventRouter (these are chrome.windows.*
events). ExtensionBrowserEventRouter will only generate chrome.tabs.* events).
There's no need for a PanelEventRouter because the WindowEventRouter would
suffice. We also won't need hooks in Panel class as WindowEventRouter could be a
WindowControllerList observer.

I'll proceed with this plan unless anyone objects.

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:1164: ExtensionPanelEventRouter*
ExtensionService::GetPanelEventRouter() {
On 2012/07/20 16:54:33, stevenjb (chromium) wrote:
> Shouldn't this be initialized (conditionally) in
> ExtensionService::InitEventRouters() like everything else? Why the special
> accessor?

Panel class uses the event router directly, thus an accessor is needed. Also,
the event router is not created until it is used to avoid creating it when there
are no panels. (But this all goes away with the new design proposal.)

[stevenjb, 2012-07-20 18:26:14]

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
File chrome/browser/extensions/extension_panel_event_router.h (right):

http://codereview.chromium.org/10777004/diff/17011/chrome/browser/extensions/...
chrome/browser/extensions/extension_panel_event_router.h:19: class
ExtensionPanelEventRouter {
On 2012/07/20 18:09:37, jennb wrote:
> On 2012/07/20 16:54:33, stevenjb (chromium) wrote:
> > On 2012/07/19 20:14:58, Mihai Parparita wrote:
> > > In light of http://crrev.com/147083, this perhaps should go in the
> extensions
> > > namespace (then you can just call it PanelEventRouter).
> > > 
> > > Steven had stronger opinions on namespaces, I've cc-ed him on this review.
> > > 
> > > A more important question for Steven is whether ExtensionPanelEventRouter
> and
> > > ExtensionBrowserEventRouter should share code, the way that window
> > > implementations do (via BaseWindow). OnPanelActiveStatusChanged is complex
> > > enough (and is nearly identical to OnBrowserSetLastActive) that I would
> prefer
> > > to see either a BaseExtensionWindowEventRouter class that they both
inherit
> > > from, or a ExtensionWindowEventRouterHelper class that they use
composition
> > > with.
> > 
> > I think this should be extensions::PanelEventRouter. 
> > 
> > I looked through ExtensionBrowserEventRouter, and it really does look like
> that
> > could become WindowControllerEventRouter with all references to Browser
> replaced
> > with ExtensionWindowController. Then the same code should work for panels
> also.
> > I believe that BrowserList in BrowserEventRouter could be replaced with
> > WindowControllerList (which may need to have an observer list added) and
also
> > work with both Browser and Panel.
> > 
> > I'm not sure we need to do that re-factoring here, but it does seem like it
> > would avoid a lot of duplicate code, which would definitely be preferable.
> > 
> 
> I propose creating extension::WindowEventRouter (these are chrome.windows.*
> events). ExtensionBrowserEventRouter will only generate chrome.tabs.* events).
> There's no need for a PanelEventRouter because the WindowEventRouter would
> suffice. We also won't need hooks in Panel class as WindowEventRouter could be
a
> WindowControllerList observer.
> 
> I'll proceed with this plan unless anyone objects.
> 

That sounds great to me, thanks!

[jennb, 2012-07-21 00:57:43]

New revision ready. Window events are now all dispatched by WindowEventRouter
regardless of Browser or Panel window. ExtensionBrowserEventRouter only
dispatches tab events.

Ran into a glitch with WindowControllerListObserver as WindowController was
adding/removing itself from the list in its ctor/dtor. WindowController has pure
virtual functions and the subclass isn't around in the base class ctor/dtor so I
had to move the logic to the subclass ctor/dtor. 

PTAL

[stevenjb, 2012-07-23 18:02:41]

lgtm

http://codereview.chromium.org/10777004/diff/43001/chrome/browser/extensions/...
File chrome/browser/extensions/window_controller.h (right):

http://codereview.chromium.org/10777004/diff/43001/chrome/browser/extensions/...
chrome/browser/extensions/window_controller.h:33: // upon
construction/destruction.
Hm, I'm going back and forth on this. I'm not real fond of forced duplicate
code, and was thinking that rather than requiring each subclass to add/remove
itself from the list, we could make the destructor private and create a Destroy
function that first removes the instance from the list and then destroys it. On
the other hand, that would prevent the use of scoped containers, so maybe that's
not a good idea. I guess I'm OK either way, i.e. it's fine as-is.

[jennb, 2012-07-23 18:45:22]

+sky for ash OWNERS review.

[sky, 2012-07-23 22:02:39]

LGTM

[Mihai Parparita -not on Chrome, 2012-07-23 23:12:15]

LGTM

[commit-bot: I haz the power, 2012-07-23 23:14:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/10777004/41003

[commit-bot: I haz the power, 2012-07-24 01:19:01]

Failed to apply patch for
chrome/browser/extensions/extension_browser_event_router.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/extension_browser_event_router.cc
Hunk #1 FAILED at 11.
Hunk #2 FAILED at 78.
Hunk #3 FAILED at 112.
Hunk #4 FAILED at 138.
Hunk #5 FAILED at 172.
Hunk #6 FAILED at 565.
6 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_browser_event_router.cc.rej

[commit-bot: I haz the power, 2012-07-24 18:04:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/10777004/38005

[commit-bot: I haz the power, 2012-07-24 19:31:13]

Change committed as 148167