Implement sidebar support for extension action popups

Implement sidebar support for extension action popups

Patch generated from previous existing code provided for issue crbug.com/51084 "Add sidebar surface for extensions".

BUG=477424

[ltilve, 2015-05-27 06:46:15]

ltilve@igalia.com changed reviewers:
+ kalman@chromium.org

[ltilve, 2015-05-27 06:46:15]

This patch adds an initial sidebar implementation to allow extensions to show
contents on a sidebar as an alternative to the popup.

Instead of a new chromium.sidebar API, the problem is simplified by letting
extensions open as a sidebar when the browserAction or pageAction button is
clicked.

This initial implementation provides a perTab approach, allowing the user to
have separate independent sidebars open on different tabs or windows.

Attribution is always guaranteed while the sidebar is visible, as its extension
action button is kept depressed. In the case of the extension being in the
overflow, its icon is slided out while the sidebar is visible, just as done in
the case of popups after the fix for crbug.com/479387.

[not at google - send to devlin, 2015-05-27 15:50:09]

kalman@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org
- kalman@chromium.org

[not at google - send to devlin, 2015-05-27 15:50:10]

Awesome to see this happening!

Devlin will be the primary reviewer for the Sidebar API. Both of us (and Alex
and Jake) are 100% involved and invested in its success, the only change is that
there are now 4 Googlers involved :)

[Devlin, 2015-05-27 16:51:05]

Hey, great to see this is coming along!

It's gonna take me awhile to make it through this code - there's a lot going on
here. :)  Most likely, this would be better done in a couple different
patches... If you have an idea of a good way to split this up, that'd be great;
otherwise, I can try and suggest something once I grok it more fully.

In the meantime, a few things that immediately jumped out at me that should be
fixed.

https://codereview.chromium.org/1152613003/diff/20001/chrome/browser/browser_...
File chrome/browser/browser_process.h (right):

https://codereview.chromium.org/1152613003/diff/20001/chrome/browser/browser_...
chrome/browser/browser_process.h:124: virtual SidebarManager* sidebar_manager()
= 0;
We almost certainly don't want to have a SidebarManager as part of the browser
process (sidebars cannot, for instance, cross profiles).  This should be tied
more closely to a profile, and should probably either be a
BrowserContextKeyedService (or one of its subclasses) or live on the
ExtensionSystem.  My first inclination is probably that the latter makes more
sense.

https://codereview.chromium.org/1152613003/diff/20001/chrome/browser/chrome_n...
File chrome/browser/chrome_notification_types.h (right):

https://codereview.chromium.org/1152613003/diff/20001/chrome/browser/chrome_n...
chrome/browser/chrome_notification_types.h:343: // Sidebar
-----------------------------------------------------------------
We're working "hard" (read: we really want to, but bandwidth is hard to come by)
to deprecate notifications, so it'd be best to avoid adding any more, if we can.

[ltilve, 2015-05-27 19:13:17]

> Hey, great to see this is coming along!

Thanks for the comments, we are also really excited to help on making it happen.

> It's gonna take me awhile to make it through this code - there's a lot going
on
> here. :)  Most likely, this would be better done in a couple different
> patches... If you have an idea of a good way to split this up, that'd be
great;
> otherwise, I can try and suggest something once I grok it more fully.

We had doubts on the different alternatives to split the code, as it does not
seem to be a proper way to divide it into a valid subset of features. We have
discarded mocking up parts of the code as it had reached already a useful
testable state. Other option we had been considered was separating to a
different CL the Mac implementation to split the size.

In any case we are open to any suggestion in this regard.

We are also looking into the fixes that you have already pointed out :)

[not at google - send to devlin, 2015-05-27 19:40:03]

kalman@chromium.org changed reviewers:
+ kalman@chromium.org

[not at google - send to devlin, 2015-05-27 19:40:04]

Drive-by thought: should the sidebar implementation be a component?

[ltilve, 2015-05-28 14:03:59]

On 2015/05/27 19:40:04, kalman wrote:
> Drive-by thought: should the sidebar implementation be a component?

We think that sidebar implementation might not be something that could be
naturally componentized separatedly from the rest of the extension browser/page
action popup code, as it is only providing an alternative to show the
extensionAction.

In the case of considering the idea of making it a component, it should probably
imply moving all the extensionAction functionality to it, but we also have some
doubts on this code needing to be a component.

[Devlin, 2015-05-28 15:33:54]

On 2015/05/28 14:03:59, ltilve wrote:
> On 2015/05/27 19:40:04, kalman wrote:
> > Drive-by thought: should the sidebar implementation be a component?
> 
> We think that sidebar implementation might not be something that could be
> naturally componentized separatedly from the rest of the extension
browser/page
> action popup code, as it is only providing an alternative to show the
> extensionAction.
> 
> In the case of considering the idea of making it a component, it should
probably
> imply moving all the extensionAction functionality to it, but we also have
some
> doubts on this code needing to be a component.

I think that, for the time being at least, it's fine to have this not be a
component.

[not at google - send to devlin, 2015-05-28 15:54:40]

On 2015/05/28 15:33:54, Devlin wrote:
> On 2015/05/28 14:03:59, ltilve wrote:
> > On 2015/05/27 19:40:04, kalman wrote:
> > > Drive-by thought: should the sidebar implementation be a component?
> > 
> > We think that sidebar implementation might not be something that could be
> > naturally componentized separatedly from the rest of the extension
> browser/page
> > action popup code, as it is only providing an alternative to show the
> > extensionAction.
> > 
> > In the case of considering the idea of making it a component, it should
> probably
> > imply moving all the extensionAction functionality to it, but we also have
> some
> > doubts on this code needing to be a component.
> 
> I think that, for the time being at least, it's fine to have this not be a
> component.

I haven't looked at the code, so maybe what I proposed was/is impossible, but
ideally:

(1) I'd like to see the UI code independent of extensions. A panel which hosts a
WebContents, can size itself, can position itself relative to other UI
components - idiomatically, independent. It does so happen that the only client
is extensions, but maybe one day this will change, and at the very least it
separates the concerns of extensions and sidebars, forcing better interfaces and
testability and so on.
(2) I'd like to see the UI code independent of chrome, for the same reasons.
These are just widgets and WebContents, there's no reason to depend on Chrome.
(3) I'd like to see extensions have an API for showing a UI surface visible but
independent of the current tab being viewed. We're implementing this using
sidebars attached to tabs/windows/global but that isn't necessarily going to be
the case - consider other platforms and form factors. This is not merely a
hypothetical situation.

From just looking at the files changed and added, (1) is already the case? For
(2) there are legacy reasons why nothing is implemented inside //extensions but
I'd like to move away from this whenever possible. This implies //components to
me.

but now I should disappear and stop driving by in a big truck.

[Devlin, 2015-06-01 22:08:24]

A few more notes on this:
- If there is a significant amount of code that isn't tied closely to
ExtensionHost for the Sidebar, then Ben's idea of having its implementation live
in components is probably good.  If it ends up being very linked to
ExtensionHost (and ExtensionViewHost, ExtensionViewHostView, etc), then it may
be okay to leave it in chrome.
- I notice that the patch says a lot of this code was generated from the
previous implementation.  IIRC, the previous implementation wasn't all that
great, and left us wanting quite a bit more.  (And, in general, trying to add
things back in a few years later doesn't work well with projects that move as
fast as Chrome.)  Not saying it's not worth looking at, by any means, but
cautioning you to not use that code as dogma. :)

Please do ping again when this is ready for another look!

[ltilve, 2015-06-03 22:25:44]

On 2015/05/27 16:51:05, Devlin wrote:

> We almost certainly don't want to have a SidebarManager as part of the browser
> process (sidebars cannot, for instance, cross profiles).  This should be tied
> more closely to a profile, and should probably either be a
> BrowserContextKeyedService (or one of its subclasses) or live on the
> ExtensionSystem.  My first inclination is probably that the latter makes more
> sense.

Done. It does seem more reasonable to make it more independent of the browser
process, so we moved SidebarManager to the ExtensionSystem as you were
suggesting.

> We're working "hard" (read: we really want to, but bandwidth is hard to come
by)
> to deprecate notifications, so it'd be best to avoid adding any more, if we
can.

Done. It was possible to replace the notifications that we were using.

[ltilve, 2015-06-03 22:51:02]

> - If there is a significant amount of code that isn't tied closely to
> ExtensionHost for the Sidebar, then Ben's idea of having its implementation
live
> in components is probably good.  If it ends up being very linked to
> ExtensionHost (and ExtensionViewHost, ExtensionViewHostView, etc), then it may
> be okay to leave it in chrome.

Right now, there is still a quite strong connection with the Extension*Host, as
we are 'extending' the browserAction to be able to show the popup either as the
standard popup case, or as a sidebar.

> - I notice that the patch says a lot of this code was generated from the
> previous implementation.  IIRC, the previous implementation wasn't all that
> great, and left us wanting quite a bit more.  (And, in general, trying to add
> things back in a few years later doesn't work well with projects that move as
> fast as Chrome.)  Not saying it's not worth looking at, by any means, but
> cautioning you to not use that code as dogma. :)

We were using that code as departing base, but we refactored some parts and
tried to simplify it significantly, so we are already not intending to get too
conditioned by it. In any case, let us know about any other required
modifications.

> Please do ping again when this is ready for another look!

Thanks! we have just submitted a new patchset fixing the initial comments you
addressed.

[Devlin, 2015-06-04 18:00:15]

A few more high-level comments.  Also, this is still a pretty epicly-sized CL. 
I think we should split it up into (at least) 3.  The first one should include
no "real" UI code - that is, it shouldn't have anything touched in ui/views or
ui/cocoa, and you can probably leave out a little of the stuff in, e.g.,
ui/browser.  The second should add the views implementation, and the third
should add the cocoa implementation.

I know it's a little bit of a pain to do this (though hopefully not too much -
it looks like there shouldn't be much that you need to stub out for it to be
able to be cut up), but it will help, overall.  Not only will it help me review
it faster, but it's often nice when you don't need so many reviewers on a single
CL (i.e., you'll pretty just need my LG, rather than mine, a cocoa reviewer, and
a views reviewer).

If there's any huge problems that come up with this, let me know, but I think it
should be okay (though obviously it won't "work" out the gate).

https://codereview.chromium.org/1152613003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/sidebar_manager.cc (right):

https://codereview.chromium.org/1152613003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.cc:27: ContentIdToSidebarContainerMap
content_id_to_sidebar_container;
I don't think we should support multiple sidebars in a single tab - it's
wasteful with web contents, and this "active/inactive" stuff complicates the
code a lot.  Why not just have it behave similar to popups (though on a per-tab
basis) - you can have one sidebar open at a time, and if you open another, it
closes the first (completely)?

https://codereview.chromium.org/1152613003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.cc:96: was_active_sidebar_contents ==
NULL
nit (cl-wide): Prefer nullptr over NULL now.

https://codereview.chromium.org/1152613003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.cc:191: void
SidebarManager::NavigateSidebar(content::WebContents* tab,
Certain methods here (like this one) seem to belong purely on the Sidebar
itself, rather than in the manager.  I think the Manager should be responsible
for managing which sidebar is showing (if any), but not responsible for the
internal state of that sidebar (like url).

https://codereview.chromium.org/1152613003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/sidebar_manager.h (right):

https://codereview.chromium.org/1152613003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:35: public
base::RefCounted<SidebarManager> {
Is there a reason this is refcounted?

[ltilve, 2015-06-09 13:04:56]

On 2015/06/04 18:00:15, Devlin wrote:

> A few more high-level comments.  Also, this is still a pretty epicly-sized CL.

> I think we should split it up into (at least) 3.  The first one should include
> no "real" UI code - that is, it shouldn't have anything touched in ui/views or
> ui/cocoa, and you can probably leave out a little of the stuff in, e.g.,
> ui/browser.  The second should add the views implementation, and the third
> should add the cocoa implementation.

Thanks for the comments, I have split the CL into the parts that you suggested
(including the reviewed modifications):
https://codereview.chromium.org/1168383002/
https://codereview.chromium.org/1169823005/
https://codereview.chromium.org/1171873003/

We were aware that was a very big CL, but as it was not feasible separating it
into smaller functional parts, and it was now all working already, we were
thinking that reviewing/testing all together could be fine. But I agree that
making it smaller even though it would not "work" separately, makes sense.

>
https://codereview.chromium.org/1152613003/diff/40001/chrome/browser/extensio...
> File chrome/browser/extensions/sidebar_manager.cc (right):
> 
>
https://codereview.chromium.org/1152613003/diff/40001/chrome/browser/extensio...
> chrome/browser/extensions/sidebar_manager.cc:27:
ContentIdToSidebarContainerMap
> content_id_to_sidebar_container;
> I don't think we should support multiple sidebars in a single tab - it's
> wasteful with web contents, and this "active/inactive" stuff complicates the
> code a lot.  Why not just have it behave similar to popups (though on a
per-tab
> basis) - you can have one sidebar open at a time, and if you open another, it
> closes the first (completely)?

We managed to simplify the code keeping the behaviour, in order to allow two
extensions being able to have their sidebar open in different tabs. 

There would be other reason, if opening a sidebar destroys any other one on a
different tab. Per attribution reasons, we are only showing depressed the
extension action button when its sidebar is visible at the current tab. Because
of this, the user is on a tab without any sidebar, he would not have information
about if clicking on the extension, this could be closing a sidebar on a
different tab, which might have been intended to be more persistent.

>
https://codereview.chromium.org/1152613003/diff/40001/chrome/browser/extensio...
> chrome/browser/extensions/sidebar_manager.cc:96: was_active_sidebar_contents
==
> NULL
> nit (cl-wide): Prefer nullptr over NULL now.

Done.

>
https://codereview.chromium.org/1152613003/diff/40001/chrome/browser/extensio...
> File chrome/browser/extensions/sidebar_manager.h (right):
> 
>
https://codereview.chromium.org/1152613003/diff/40001/chrome/browser/extensio...
> chrome/browser/extensions/sidebar_manager.h:35: public
> base::RefCounted<SidebarManager> {
> Is there a reason this is refcounted?

Changed.

With the new smaller patches I assume we could close this one and use the split
CLs to have smaller deltas to keep on with the review. Is this ok?