Views - init less stuff in tabless browsers.

Views - init less stuff in tabless browsers.

There's no need to create views that are hidden, register listeners relevant to
those views, etc. in popup windows.

The one part of BrowserActionsContainer that we do want in popup windows is the ExtensionKeybindingRegistryViews instance. Move this to BrowserView.

BUG=560345
Committed: https://crrev.com/845adff50786ff3c7515d51d04370825ad9e3a8b
Cr-Commit-Position: refs/heads/master@{#373344}

[Evan Stade, 2016-02-01 20:30:24]

estade@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2016-02-03 00:02:08]

LGTM

https://codereview.chromium.org/1654223002/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):

https://codereview.chromium.org/1654223002/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_view.cc:178:
GetWidget()->AddObserver(this);
Don't we still need to do this?  It seems like without this, keybindings for
browser actions won't work in popup windows.  I realize we're not displaying the
icons for these, but I would have assumed you could still trigger them via these
keybindings today.  Is that not true?

[Evan Stade, 2016-02-03 00:04:36]

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

[Evan Stade, 2016-02-03 00:04:36]

https://codereview.chromium.org/1654223002/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):

https://codereview.chromium.org/1654223002/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_view.cc:178:
GetWidget()->AddObserver(this);
On 2016/02/03 00:02:08, Peter Kasting wrote:
> Don't we still need to do this?  It seems like without this, keybindings for
> browser actions won't work in popup windows.  I realize we're not displaying
the
> icons for these, but I would have assumed you could still trigger them via
these
> keybindings today.  Is that not true?

+Devlin to answer that

I kind of assumed that if that worked it was a bug. Devlin, does it work and
should that behavior be preserved?

[Devlin, 2016-02-03 01:02:41]

https://codereview.chromium.org/1654223002/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):

https://codereview.chromium.org/1654223002/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_view.cc:178:
GetWidget()->AddObserver(this);
On 2016/02/03 00:04:36, Evan Stade wrote:
> On 2016/02/03 00:02:08, Peter Kasting wrote:
> > Don't we still need to do this?  It seems like without this, keybindings for
> > browser actions won't work in popup windows.  I realize we're not displaying
> the
> > icons for these, but I would have assumed you could still trigger them via
> these
> > keybindings today.  Is that not true?
> 
> +Devlin to answer that
> 
> I kind of assumed that if that worked it was a bug. Devlin, does it work and
> should that behavior be preserved?

Yes, it works, and ideally, yes, it should be preserved.

[Evan Stade, 2016-02-03 01:21:44]

https://codereview.chromium.org/1654223002/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):

https://codereview.chromium.org/1654223002/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_view.cc:178:
GetWidget()->AddObserver(this);
On 2016/02/03 01:02:41, Devlin wrote:
> On 2016/02/03 00:04:36, Evan Stade wrote:
> > On 2016/02/03 00:02:08, Peter Kasting wrote:
> > > Don't we still need to do this?  It seems like without this, keybindings
for
> > > browser actions won't work in popup windows.  I realize we're not
displaying
> > the
> > > icons for these, but I would have assumed you could still trigger them via
> > these
> > > keybindings today.  Is that not true?
> > 
> > +Devlin to answer that
> > 
> > I kind of assumed that if that worked it was a bug. Devlin, does it work and
> > should that behavior be preserved?
> 
> Yes, it works, and ideally, yes, it should be preserved.

do you have an example of an extension that uses this? I don't know why this
keybinding registry thing is part of BrowserActionsContainer. I would have
assumed it was somehow related to browser actions, but it appears to be relevant
for all extensions.

[Evan Stade, 2016-02-03 02:10:41]

On 2016/02/03 01:21:44, Evan Stade wrote:
>
https://codereview.chromium.org/1654223002/diff/1/chrome/browser/ui/views/too...
> File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):
> 
>
https://codereview.chromium.org/1654223002/diff/1/chrome/browser/ui/views/too...
> chrome/browser/ui/views/toolbar/toolbar_view.cc:178:
> GetWidget()->AddObserver(this);
> On 2016/02/03 01:02:41, Devlin wrote:
> > On 2016/02/03 00:04:36, Evan Stade wrote:
> > > On 2016/02/03 00:02:08, Peter Kasting wrote:
> > > > Don't we still need to do this?  It seems like without this, keybindings
> for
> > > > browser actions won't work in popup windows.  I realize we're not
> displaying
> > > the
> > > > icons for these, but I would have assumed you could still trigger them
via
> > > these
> > > > keybindings today.  Is that not true?
> > > 
> > > +Devlin to answer that
> > > 
> > > I kind of assumed that if that worked it was a bug. Devlin, does it work
and
> > > should that behavior be preserved?
> > 
> > Yes, it works, and ideally, yes, it should be preserved.
> 
> do you have an example of an extension that uses this? I don't know why this
> keybinding registry thing is part of BrowserActionsContainer. I would have
> assumed it was somehow related to browser actions, but it appears to be
relevant
> for all extensions.

The latest patch moves the extension command registry, which seemingly has
nothing to do with the browser action container view, to BrowserView. It reduces
the number of thunks, consolidates related code, and doesn't break [1].

[1]
https://chrome.google.com/webstore/detail/duplicate-tab-shortcut-ke/pfippblam...

[Evan Stade, 2016-02-03 02:11:53]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-03 02:13:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1654223002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1654223002/40001

[Peter Kasting, 2016-02-03 02:41:31]

LGTM

[Devlin, 2016-02-03 02:50:43]

new patch set lgtm.  Might be worth updating the CL description to mention the
move of the registry.

https://codereview.chromium.org/1654223002/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):

https://codereview.chromium.org/1654223002/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/toolbar_view.cc:178:
GetWidget()->AddObserver(this);
On 2016/02/03 01:21:44, Evan Stade wrote:
> On 2016/02/03 01:02:41, Devlin wrote:
> > On 2016/02/03 00:04:36, Evan Stade wrote:
> > > On 2016/02/03 00:02:08, Peter Kasting wrote:
> > > > Don't we still need to do this?  It seems like without this, keybindings
> for
> > > > browser actions won't work in popup windows.  I realize we're not
> displaying
> > > the
> > > > icons for these, but I would have assumed you could still trigger them
via
> > > these
> > > > keybindings today.  Is that not true?
> > > 
> > > +Devlin to answer that
> > > 
> > > I kind of assumed that if that worked it was a bug. Devlin, does it work
and
> > > should that behavior be preserved?
> > 
> > Yes, it works, and ideally, yes, it should be preserved.
> 
> do you have an example of an extension that uses this? I don't know why this
> keybinding registry thing is part of BrowserActionsContainer. I would have
> assumed it was somehow related to browser actions, but it appears to be
relevant
> for all extensions.

Any extension can have a keybinding associated with it, so this is helpful for
extensions that, e.g., inject a script on a page when they are triggered.

[commit-bot: I haz the power, 2016-02-03 02:52:28]

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

[commit-bot: I haz the power, 2016-02-03 02:52:29]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-02-03 21:22:41]

Description was changed from

==========
Views - init less stuff in tabless browsers.

There's no need to create views that are hidden, register listeners relevant to
those views, etc. in popup windows.

BUG=560345
==========

to

==========
Views - init less stuff in tabless browsers.

There's no need to create views that are hidden, register listeners relevant to
those views, etc. in popup windows.

The one part of BrowserActionsContainer that we do want in popup windows is the
ExtensionKeybindingRegistryViews instance. Move this to BrowserView.

BUG=560345
==========

[Evan Stade, 2016-02-03 21:23:07]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-02-03 21:24:21]

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

[commit-bot: I haz the power, 2016-02-03 21:31:23]

Description was changed from

==========
Views - init less stuff in tabless browsers.

There's no need to create views that are hidden, register listeners relevant to
those views, etc. in popup windows.

The one part of BrowserActionsContainer that we do want in popup windows is the
ExtensionKeybindingRegistryViews instance. Move this to BrowserView.

BUG=560345
==========

to

==========
Views - init less stuff in tabless browsers.

There's no need to create views that are hidden, register listeners relevant to
those views, etc. in popup windows.

The one part of BrowserActionsContainer that we do want in popup windows is the
ExtensionKeybindingRegistryViews instance. Move this to BrowserView.

BUG=560345
==========

[commit-bot: I haz the power, 2016-02-03 21:31:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-03 21:32:11]

Description was changed from

==========
Views - init less stuff in tabless browsers.

There's no need to create views that are hidden, register listeners relevant to
those views, etc. in popup windows.

The one part of BrowserActionsContainer that we do want in popup windows is the
ExtensionKeybindingRegistryViews instance. Move this to BrowserView.

BUG=560345
==========

to

==========
Views - init less stuff in tabless browsers.

There's no need to create views that are hidden, register listeners relevant to
those views, etc. in popup windows.

The one part of BrowserActionsContainer that we do want in popup windows is the
ExtensionKeybindingRegistryViews instance. Move this to BrowserView.

BUG=560345

Committed: https://crrev.com/845adff50786ff3c7515d51d04370825ad9e3a8b
Cr-Commit-Position: refs/heads/master@{#373344}
==========

[commit-bot: I haz the power, 2016-02-03 21:32:12]

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

[Evan Stade, 2016-02-04 19:35:07]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1670663002/ by estade@chromium.org.

The reason for reverting is: this is causing a lot of crashes on canary, revert
for now and reland later with a fix.