Implement sidebar support for extension action popups

Implement sidebar support for extension action popups

This is an initial patch that provides the non-UI code to allow extensions to
show contents on a sidebar as an alternative to the popup.

BUG=477424

[ltilve, 2015-06-09 09:27:14]

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

[ltilve, 2015-06-09 09:45:40]

Adds the first part for the implementation to allow extensions to show contents
on a sidebar as an alternative to the popup.

This is the first CL from the suggested approach to split the code at
https://codereview.chromium.org/1152613003/#msg15

The other CLs with the views implementation and the cocoa part are respectively
https://codereview.chromium.org/1169823005/ and
https://codereview.chromium.org/1171873003/

[Devlin, 2015-06-10 17:25:05]

You seem to have omitted all the code that makes this do anything. ;)  When I
suggested you break this up, I was suggesting three patches, being the core
functionality (in theory this one), the views implementation, and the cocoa
implementation.  But this patch doesn't have any code that, e.g., opens a
sidebar anymore.  Basically, what I'm looking for is for this patch to do
everything *except* actually pop up a side bar UI element in the browser.  It
should include the logic of active sidebars, switching sidebars, etc, and should
definitely include some tests.

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

https://codereview.chromium.org/1168383002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.cc:30:
navigate_to_default_page_on_expand_(true) {
The way this is written, it doesn't seem like this is ever really used.

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

https://codereview.chromium.org/1168383002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.h:31: class SidebarContainer :
public extensions::ExtensionHostObserver,
You're not overriding anything from ExtensionHostObserver?

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

https://codereview.chromium.org/1168383002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.cc:103: void
SidebarManager::NavigateSidebar(content::WebContents* tab) {
Taken directly from
https://codereview.chromium.org/1152613003/diff/40001/chrome/browser/extensio...
which was not addressed.

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).

[ltilve, 2015-06-11 18:14:14]

Thanks for the comments. I have merged now also the changes to handle all the
sidebar logic (but the browser/ui part) and the browser_tests. Sorry about the
misunderstanding about the former split, I hope it's fine now :).
 
>
https://codereview.chromium.org/1168383002/diff/40001/chrome/browser/extensio...
> File chrome/browser/extensions/sidebar_container.cc (right):
> 
>
https://codereview.chromium.org/1168383002/diff/40001/chrome/browser/extensio...
> chrome/browser/extensions/sidebar_container.cc:30:
> navigate_to_default_page_on_expand_(true) {
> The way this is written, it doesn't seem like this is ever really used.

Fixed.

>
https://codereview.chromium.org/1168383002/diff/40001/chrome/browser/extensio...
> File chrome/browser/extensions/sidebar_container.h (right):
> 
>
https://codereview.chromium.org/1168383002/diff/40001/chrome/browser/extensio...
> chrome/browser/extensions/sidebar_container.h:31: class SidebarContainer :
> public extensions::ExtensionHostObserver,
> You're not overriding anything from ExtensionHostObserver?

Fixed. We don't need any more to implement ExtensionHostObserver interface here.

>
https://codereview.chromium.org/1168383002/diff/40001/chrome/browser/extensio...
> File chrome/browser/extensions/sidebar_manager.cc (right):
> 
>
https://codereview.chromium.org/1168383002/diff/40001/chrome/browser/extensio...
> chrome/browser/extensions/sidebar_manager.cc:103: void
> SidebarManager::NavigateSidebar(content::WebContents* tab) {
> Taken directly from
>
https://codereview.chromium.org/1152613003/diff/40001/chrome/browser/extensio...
> which was not addressed.
> 
> 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).

We have removed NavigateSidebar function, and have replaced ShowSidebar by a
clearer CreateSidebar. We are avoiding the Manager to be responsible of the
state, but just using the url param to build the Sidebar.

[Devlin, 2015-06-16 17:56:12]

You should also include the logic from ExtensionActionViewController, since that
class is platform-agnostic, and does not require a real view - thus it's part of
the "core" logic.  Then, we can have a slightly more intensive test, like
clicking on actions in order to open/close sidebars.

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

https://codereview.chromium.org/1168383002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.cc:109: void
SidebarManager::Observe(int type,
This should be accomplished without notifications.  Would TabStripModelObserver
work here?

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

https://codereview.chromium.org/1168383002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:45: void
NotifyStateChanges(content::WebContents* was_active_sidebar_contents,
When is this used?

https://codereview.chromium.org/1168383002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:95: typedef
std::map<content::WebContents*, SidebarContainer*>
Having two maps is probably excessive, since most users won't have that many
sidebars open, and keeping everything up to date is a bit of a pain.  I'd just
have one map, keyed on web contents, and then if you need to look up a
particular sidebar, you can just traverse it (since it will be small).

https://codereview.chromium.org/1168383002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/sidebar/sidebar_browsertest.cc (right):

https://codereview.chromium.org/1168383002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/sidebar/sidebar_browsertest.cc:32: class SidebarTest :
public ExtensionBrowserTest {
Is there a reason this needs to be a browser test?  Or in ui/views, for that
matter?

https://codereview.chromium.org/1168383002/diff/60001/extensions/browser/exte...
File extensions/browser/extension_host.h (right):

https://codereview.chromium.org/1168383002/diff/60001/extensions/browser/exte...
extensions/browser/extension_host.h:143: virtual void LoadURL(const GURL& url);
When is this used?

[ltilve, 2015-06-19 12:07:25]

> You should also include the logic from ExtensionActionViewController, since
that
> class is platform-agnostic, and does not require a real view - thus it's part
of
> the "core" logic.  Then, we can have a slightly more intensive test, like
> clicking on actions in order to open/close sidebars.

Thanks for the review. That logic is included now and the tests were updated to
have the browser action pressed to change the sidebar state.

>
https://codereview.chromium.org/1168383002/diff/60001/chrome/browser/extensio...
> chrome/browser/extensions/sidebar_manager.cc:109: void
> SidebarManager::Observe(int type,
> This should be accomplished without notifications.  Would
TabStripModelObserver
> work here?

Done. We have avoided using those notifications to have the
TabStripModelObserver instead.
 
>
https://codereview.chromium.org/1168383002/diff/60001/chrome/browser/extensio...
> chrome/browser/extensions/sidebar_manager.h:45: void
> NotifyStateChanges(content::WebContents* was_active_sidebar_contents,
> When is this used?

Done. I moved NotifyStateChanges to the split UI part of the CL, as it will be
needed later UpdateSidebarForContents() at the browser_view.
 
>
https://codereview.chromium.org/1168383002/diff/60001/chrome/browser/extensio...
> chrome/browser/extensions/sidebar_manager.h:95: typedef
> std::map<content::WebContents*, SidebarContainer*>
> Having two maps is probably excessive, since most users won't have that many
> sidebars open, and keeping everything up to date is a bit of a pain.  I'd just
> have one map, keyed on web contents, and then if you need to look up a
> particular sidebar, you can just traverse it (since it will be small).

Done. We are now just using one map.

>
https://codereview.chromium.org/1168383002/diff/60001/chrome/browser/ui/views...
> chrome/browser/ui/views/sidebar/sidebar_browsertest.cc:32: class SidebarTest :
> public ExtensionBrowserTest {
> Is there a reason this needs to be a browser test?  Or in ui/views, for that
> matter?

Done, the test  was moved from ui/views to chrome/browser/extensions. We think
that it makes sense for it being a browser test, as it needs interaction with
some other browser() components as the action buttons or tabs.

>
https://codereview.chromium.org/1168383002/diff/60001/extensions/browser/exte...
> extensions/browser/extension_host.h:143: virtual void LoadURL(const GURL&
url);
> When is this used?

Done. It was a leftover from previous code.

[Devlin, 2015-06-19 19:56:11]

Okay, now that the structure is out of the way, on to the easier fixes. :)

https://codereview.chromium.org/1168383002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/sidebar/sidebar_browsertest.cc (right):

https://codereview.chromium.org/1168383002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/sidebar/sidebar_browsertest.cc:32: class SidebarTest :
public ExtensionBrowserTest {
On 2015/06/16 17:56:12, Devlin wrote:
> Is there a reason this needs to be a browser test?  Or in ui/views, for that
> matter?

Two notes:
- When replying to specific comments in the code, it's helpful to use the
"reply" on the comment in the source file, rather than just replying inline on
the top-level CL, because that way it's easy to track them all.
- (Responding to using a browsertest) Browsertests are kind of misnomers - you
don't need them just for a Browser object or WebContents.  If you're really
determined, you only really need a browsertest for things that require renderer
interaction, though sometimes the amount of setup to get to that point is
prohibitive.  That said, for this particular test, I think a browser in a
unittest is plenty sufficient.   (For instance, a BrowserWithTestWindowTest will
give you a browser with a window.)

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.cc:575: void
ExtensionSystemImpl::CreateSidebarManager() {
Sidebar manager is basically free to create, so just initialize it in Init(),
rather than doing it lazily.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.h (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.h:22: // class SidebarManager;
remove

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/sidebar_browsertest.cc (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_browsertest.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
no (c) (Goes for all new files)

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_browsertest.cc:34: class SidebarTest : public
ExtensionBrowserTest {
This looks like it can be split into two tests:
- a SidebarManager test, that tests opening and closing sidebars through the
SidebarManager API.  This can be a unittest.  This is where the majority of the
edge-case logic should be tested.
- a SidebarBrowserTest, that tests opening and closing sidebars with extension
actions (more of an end-to-end test).

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_browsertest.cc:36: SidebarTest() {}
Please add a destructor

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_browsertest.cc:39: // InProcessBrowserTest
overrides.
s/ overrides./:
s/InProcessBrowserTest/ExtensionBrowserTest

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_browsertest.cc:61:
browser_action_test_util_.reset(new BrowserActionTestUtil(browser()));
again, no real point in lazily initializing this. Do it in setup.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_browsertest.cc:70: WebContents* tab =
static_cast<WebContents*>(temp);
what does this line do (and the similar ones below)?

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_browsertest.cc:104:
scoped_ptr<BrowserActionTestUtil> browser_action_test_util_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/sidebar_container.cc (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.cc:32:
extensions::ChromeExtensionWebContentsObserver::CreateForWebContents(
Isn't this done as part of the ExtensionHost initialization?

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.cc:42:
browser_->tab_strip_model()->AddObserver(this);
Use a ScopedObserver.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.cc:60: if (type !=
extensions::NOTIFICATION_EXTENSION_HOST_VIEW_SHOULD_CLOSE) {
instead DCHECK_EQ()

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/sidebar_container.h (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.h:28:
///////////////////////////////////////////////////////////////////////////////
nit: We don't really use this comment syntax that often.  Just put class
comments normally above the class, i.e.

// Here are the comments
// for this class.
class MyClass {
};

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.h:37: // Interface to implement to
listen for sidebar update notification.
?

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.h:44: // Retruns HostContents
sidebar is linked to.
typo

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.h:45: content::WebContents*
host_contents() const { return host_->host_contents(); }
Let's rename these - "host_contents" and "web_contents" are very confusing. 
They are both web_contents (that's the class name, after all), and the tab
"hosts" the sidebar, so I could see each meaning either one.

Instead, let's use "tab_contents()" and "sidebar_contents()"

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.h:52: //
content::NotificationObserver overrides.
s/ overrides./: (everywhere)

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.h:53: void Observe(int type,
make this private

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

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.cc:21: using content::BrowserContext;
use "using" like this sparingly (a good rule of thumb would be if you use the
object 5+ times).  Since you only use it once, remove this.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.cc:24: namespace extensions {
\n

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.cc:36: if (it ==
tab_to_sidebar_container_.end())
return it == tab_to_sidebar_container_.end() ? nullptr : it->second;

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

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:9: #include <string>
Do we need this?

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:12: #include
"base/strings/string16.h"
Or this?

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:13: #include
"chrome/browser/extensions/sidebar_container.h"
Or this? (Please comb through these regularly and remove what you don't need -
this goes for the rest of the patch)

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:38: // Returns SidebarContainer
registered for |tab| or nullptr if there is no
It seems like this and HasSidebar can be combined (doesn't null from this ==
HasSidebar() returning false?)

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:57: ~SidebarManager();
put this up near the constructor

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:61: SidebarContainer*
FindSidebarContainerFor(
Is this used?

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:67: ContentIdToSidebarContainerMap;
Do we need this?

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:74: typedef
std::map<content::WebContents*, SidebarContainer*>
nit: prefer 'using' syntax.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/sidebar_manager_observer.h (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager_observer.h:12: class
SidebarManagerObserver {
for good measure, add a virtual destructor.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager_observer.h:14: // Called when sidebar
is shown
Add periods to these comments.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager_observer.h:22: // Called when sidebar
changes
"changes" is pretty vague.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager_observer.h:23: virtual void
OnSidebarSwitched(content::WebContents* old_tab,
This is observed in the ExtensionActionViewController, but doesn't seem to be
called from the Manager?

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/extension_action_view_controller.cc (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_action_view_controller.cc:70:
sidebar_manager->RemoveObserver(this);
ScopedObserver

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_action_view_controller.cc:325: if
(!use_sidebar)
This raises the question of if a popup and a sidebar can be showing at the same
time.  If the answer is yes, then we need to restructure the idea of a popup
owner, since you're setting the popup owner here even if it's a sidebar, but
only closing the current one if it's not.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_action_view_controller.cc:352: return
true;
I think this should still handle sliding out the action if it's overflowed, if
we're going to try and have attribution.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_action_view_controller.cc:443: if
(old_content_id == GetId() && old_content_id != new_content_id) {
Under which circumstances would a sidebar be switched to one with the same id?

https://codereview.chromium.org/1168383002/diff/80001/chrome/common/extension...
File chrome/common/extensions/api/extension_action/action_info.cc (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/common/extension...
chrome/common/extensions/api/extension_action/action_info.cc:188:
result->open_in_sidebar = false;
Once open_in_sidebar is initialized, this line won't be needed.

https://codereview.chromium.org/1168383002/diff/80001/chrome/common/extension...
File chrome/common/extensions/api/extension_action/action_info.h (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/common/extension...
chrome/common/extensions/api/extension_action/action_info.h:68: bool
open_in_sidebar;
This should be initialized in the ctor

https://codereview.chromium.org/1168383002/diff/80001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/common/pref_name...
chrome/common/pref_names.h:543: extern const char kExtensionSidebarWidth[];
Is this used in this patch set?

[ltilve, 2015-06-28 22:44:21]

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.cc:575: void
ExtensionSystemImpl::CreateSidebarManager() {
On 2015/06/19 19:56:08, Devlin wrote:
> Sidebar manager is basically free to create, so just initialize it in Init(),
> rather than doing it lazily.

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.h (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.h:22: // class SidebarManager;
On 2015/06/19 19:56:08, Devlin wrote:
> remove

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/sidebar_browsertest.cc (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_browsertest.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/06/19 19:56:09, Devlin wrote:
> no (c) (Goes for all new files)

Done for all the new files.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_browsertest.cc:34: class SidebarTest : public
ExtensionBrowserTest {
On 2015/06/19 19:56:09, Devlin wrote:
> This looks like it can be split into two tests:
> - a SidebarManager test, that tests opening and closing sidebars through the
> SidebarManager API.  This can be a unittest.  This is where the majority of
the
> edge-case logic should be tested.
> - a SidebarBrowserTest, that tests opening and closing sidebars with extension
> actions (more of an end-to-end test).

Done. We are now having SidebarManager unnittests at this patch set, testing the
API, and browser tests that were moved to the next patch set, where the browser
actions are producing the expected output.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_browsertest.cc:36: SidebarTest() {}
On 2015/06/19 19:56:08, Devlin wrote:
> Please add a destructor

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_browsertest.cc:39: // InProcessBrowserTest
overrides.
On 2015/06/19 19:56:09, Devlin wrote:
> s/ overrides./:
> s/InProcessBrowserTest/ExtensionBrowserTest

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_browsertest.cc:61:
browser_action_test_util_.reset(new BrowserActionTestUtil(browser()));
On 2015/06/19 19:56:09, Devlin wrote:
> again, no real point in lazily initializing this. Do it in setup.

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_browsertest.cc:70: WebContents* tab =
static_cast<WebContents*>(temp);
On 2015/06/19 19:56:09, Devlin wrote:
> what does this line do (and the similar ones below)?

Done. Removed all of them.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_browsertest.cc:104:
scoped_ptr<BrowserActionTestUtil> browser_action_test_util_;
On 2015/06/19 19:56:09, Devlin wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/sidebar_container.cc (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.cc:32:
extensions::ChromeExtensionWebContentsObserver::CreateForWebContents(
On 2015/06/19 19:56:09, Devlin wrote:
> Isn't this done as part of the ExtensionHost initialization?

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.cc:42:
browser_->tab_strip_model()->AddObserver(this);
On 2015/06/19 19:56:09, Devlin wrote:
> Use a ScopedObserver.

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.cc:60: if (type !=
extensions::NOTIFICATION_EXTENSION_HOST_VIEW_SHOULD_CLOSE) {
On 2015/06/19 19:56:09, Devlin wrote:
> instead DCHECK_EQ()

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/sidebar_container.h (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.h:28:
///////////////////////////////////////////////////////////////////////////////
On 2015/06/19 19:56:09, Devlin wrote:
> nit: We don't really use this comment syntax that often.  Just put class
> comments normally above the class, i.e.
> 
> // Here are the comments
> // for this class.
> class MyClass {
> };

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.h:37: // Interface to implement to
listen for sidebar update notification.
On 2015/06/19 19:56:09, Devlin wrote:
> ?

Done. Removed wrong comment.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.h:44: // Retruns HostContents
sidebar is linked to.
On 2015/06/19 19:56:09, Devlin wrote:
> typo

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.h:45: content::WebContents*
host_contents() const { return host_->host_contents(); }
On 2015/06/19 19:56:09, Devlin wrote:
> Let's rename these - "host_contents" and "web_contents" are very confusing. 
> They are both web_contents (that's the class name, after all), and the tab
> "hosts" the sidebar, so I could see each meaning either one.
> 
> Instead, let's use "tab_contents()" and "sidebar_contents()"

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.h:52: //
content::NotificationObserver overrides.
On 2015/06/19 19:56:09, Devlin wrote:
> s/ overrides./: (everywhere)

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_container.h:53: void Observe(int type,
On 2015/06/19 19:56:09, Devlin wrote:
> make this private

Done.

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

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.cc:21: using content::BrowserContext;
On 2015/06/19 19:56:09, Devlin wrote:
> use "using" like this sparingly (a good rule of thumb would be if you use the
> object 5+ times).  Since you only use it once, remove this.

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.cc:24: namespace extensions {
On 2015/06/19 19:56:09, Devlin wrote:
> \n

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.cc:36: if (it ==
tab_to_sidebar_container_.end())
On 2015/06/19 19:56:09, Devlin wrote:
> return it == tab_to_sidebar_container_.end() ? nullptr : it->second;

Done.

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

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:9: #include <string>
On 2015/06/19 19:56:10, Devlin wrote:
> Do we need this?

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:12: #include
"base/strings/string16.h"
On 2015/06/19 19:56:10, Devlin wrote:
> Or this?

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:13: #include
"chrome/browser/extensions/sidebar_container.h"
On 2015/06/19 19:56:10, Devlin wrote:
> Or this? (Please comb through these regularly and remove what you don't need -
> this goes for the rest of the patch)

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:38: // Returns SidebarContainer
registered for |tab| or nullptr if there is no
On 2015/06/19 19:56:10, Devlin wrote:
> It seems like this and HasSidebar can be combined (doesn't null from this ==
> HasSidebar() returning false?)

By combining them I understand that you mean just removing the bool function,
and use the null check instead in all the cases. Is that correct?

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:57: ~SidebarManager();
On 2015/06/19 19:56:10, Devlin wrote:
> put this up near the constructor

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:61: SidebarContainer*
FindSidebarContainerFor(
On 2015/06/19 19:56:10, Devlin wrote:
> Is this used?

Done. This function has been moved to the UI patch set
https://codereview.chromium.org/1169823005/, where it is being used.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:67: ContentIdToSidebarContainerMap;
On 2015/06/19 19:56:10, Devlin wrote:
> Do we need this?

Done. It was not being longer used.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:74: typedef
std::map<content::WebContents*, SidebarContainer*>
On 2015/06/19 19:56:10, Devlin wrote:
> nit: prefer 'using' syntax.

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/sidebar_manager_observer.h (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager_observer.h:12: class
SidebarManagerObserver {
On 2015/06/19 19:56:10, Devlin wrote:
> for good measure, add a virtual destructor.

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager_observer.h:14: // Called when sidebar
is shown
On 2015/06/19 19:56:10, Devlin wrote:
> Add periods to these comments.

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager_observer.h:22: // Called when sidebar
changes
On 2015/06/19 19:56:10, Devlin wrote:
> "changes" is pretty vague.

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager_observer.h:23: virtual void
OnSidebarSwitched(content::WebContents* old_tab,
It's called from the Manager at the SidebarManager::NotifyStateChanges function
that was moved to the UI patchset. I guess that then all SidebarSwitched code
should be moved there. Is it correct?

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/extension_action_view_controller.cc (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_action_view_controller.cc:70:
sidebar_manager->RemoveObserver(this);
On 2015/06/19 19:56:10, Devlin wrote:
> ScopedObserver

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_action_view_controller.cc:325: if
(!use_sidebar)
Done, popups and sidebars should not be showing at the same time. You were
totally right about the problems of ownership, the behaviour should be the same
for any case, this is, closing the former sidebar/popup.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_action_view_controller.cc:352: return
true;
On 2015/06/19 19:56:10, Devlin wrote:
> I think this should still handle sliding out the action if it's overflowed, if
> we're going to try and have attribution.

Done. After the previous change, the attribution problem that could happen with
overflowed popups and sidebars is apparently fixed.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_action_view_controller.cc:443: if
(old_content_id == GetId() && old_content_id != new_content_id) {
On 2015/06/19 19:56:10, Devlin wrote:
> Under which circumstances would a sidebar be switched to one with the same id?

Done. That code was refactored into a clearer UpdateButtonState function.

https://codereview.chromium.org/1168383002/diff/80001/chrome/common/extension...
File chrome/common/extensions/api/extension_action/action_info.cc (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/common/extension...
chrome/common/extensions/api/extension_action/action_info.cc:188:
result->open_in_sidebar = false;
On 2015/06/19 19:56:10, Devlin wrote:
> Once open_in_sidebar is initialized, this line won't be needed.

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/common/extension...
File chrome/common/extensions/api/extension_action/action_info.h (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/common/extension...
chrome/common/extensions/api/extension_action/action_info.h:68: bool
open_in_sidebar;
On 2015/06/19 19:56:10, Devlin wrote:
> This should be initialized in the ctor

Done.

https://codereview.chromium.org/1168383002/diff/80001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/common/pref_name...
chrome/common/pref_names.h:543: extern const char kExtensionSidebarWidth[];
On 2015/06/19 19:56:10, Devlin wrote:
> Is this used in this patch set?

Done. It's necessary at the UI patch set, moved that change to it.

[ltilve, 2015-06-29 10:46:51]

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

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager.h:38: // Returns SidebarContainer
registered for |tab| or nullptr if there is no
On 2015/06/28 22:44:20, ltilve wrote:
> On 2015/06/19 19:56:10, Devlin wrote:
> > It seems like this and HasSidebar can be combined (doesn't null from this ==
> > HasSidebar() returning false?)
> 
> By combining them I understand that you mean just removing the bool function,
> and use the null check instead in all the cases. Is that correct? 

Done. I have applied that modification.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/sidebar_manager_observer.h (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_manager_observer.h:23: virtual void
OnSidebarSwitched(content::WebContents* old_tab,
On 2015/06/28 22:44:20, ltilve wrote:
> It's called from the Manager at the SidebarManager::NotifyStateChanges
function
> that was moved to the UI patchset. I guess that then all SidebarSwitched code
> should be moved there. Is it correct?

Done. I've moved it to https://codereview.chromium.org/1169823005

[ltilve, 2015-07-07 21:10:52]

On 2015/06/19 19:56:11, Devlin wrote:
> Okay, now that the structure is out of the way, on to the easier fixes. :)

We think that we have fixed all the issues that you mentioned on your comments
(thanks for such a detailed review, btw). Did you have the time to look at the
changes? Please let us know if you think that we should modify something else :)

[Devlin, 2015-07-07 21:39:30]

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/sidebar_browsertest.cc (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_browsertest.cc:34: class SidebarTest : public
ExtensionBrowserTest {
On 2015/06/28 22:44:19, ltilve wrote:
> On 2015/06/19 19:56:09, Devlin wrote:
> > This looks like it can be split into two tests:
> > - a SidebarManager test, that tests opening and closing sidebars through the
> > SidebarManager API.  This can be a unittest.  This is where the majority of
> the
> > edge-case logic should be tested.
> > - a SidebarBrowserTest, that tests opening and closing sidebars with
extension
> > actions (more of an end-to-end test).
> 
> Done. We are now having SidebarManager unnittests at this patch set, testing
the
> API, and browser tests that were moved to the next patch set, where the
browser
> actions are producing the expected output. 

Is there a reason the browser tests can't be in this patch set?

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/extension_action_view_controller.cc (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_action_view_controller.cc:325: if
(!use_sidebar)
On 2015/06/28 22:44:20, ltilve wrote:
> Done, popups and sidebars should not be showing at the same time. You were
> totally right about the problems of ownership, the behaviour should be the
same
> for any case, this is, closing the former sidebar/popup.

Why don't you think popups and sidebars should be able to both be present?  I
think it's probably okay, since sidebars are designed to be persistent
information added to a page.  I think it would be annoying to have a sidebar I
liked, but then be afraid to activate another popup because it closes that
sidebar... and it would be even weirder for the case of "pinned" sidebars
(though we'll still have to address something similar there when a new sidebar
opens).

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/sidebar_manager.cc (right):

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/sidebar_manager.cc:41: if (container)
There's a check in HideSidebar() that does this, so you don't have to do it
here, too.

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/sidebar_manager.h (right):

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/sidebar_manager.h:43: // tab).
What is a mini tab?

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/sidebar_manager.h:44: void
HideSidebar(content::WebContents* tab);
Let's rename this HideSidebarForTab().

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/sidebar_manager_unittest.cc (right):

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/sidebar_manager_unittest.cc:101:
TEST_F(SidebarManagerTest, CreateSidebar) {
Won't this be implicitly tested by any other test?

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/sidebar_manager_unittest.cc:111:
chrome::NewTab(browser());
prefer BrowserWithTestWindowTest::AddTab().

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/sidebar_manager_unittest.cc:130:
TEST_F(SidebarManagerTest, SidebarOnInactiveTab) {
All these tests can probably be combined into one.

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/ui/exte...
File chrome/browser/ui/extensions/extension_action_view_controller.cc (right):

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_action_view_controller.cc:331: bool
already_showing = is_showing_popup();
I think we should just re-brand most of these popup things to apply to either a
popup or a sidebar.  Something like is_showing_content().  That will simplify
quite a few of the checks later on.  You shouldn't have to add much new logic
for the sidebar.

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_action_view_controller.cc:451: this,
base::Bind(&ExtensionActionViewController::OnPopupShown,
I don't think we should have actions with a sidebar forced out.  It makes it
impossible to reason about reordering.

[ltilve, 2015-07-09 22:15:50]

Thanks for the reviews. Please do not hesitate in asking for any further
clarification about the proposed approach.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/sidebar_browsertest.cc (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_browsertest.cc:34: class SidebarTest : public
ExtensionBrowserTest {

> Is there a reason the browser tests can't be in this patch set?

I have moved them to the next patch set because as they use Press() on the
BrowserAction to show/hide the sidebars, without the UI part, this
HasSidebarForCurrentTab() is false and the tests are not passing. Should I
include them in any case, expecting false for HasSidebarForCurrentTab() after
the click?

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/extension_action_view_controller.cc (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_action_view_controller.cc:325: if
(!use_sidebar)

> Why don't you think popups and sidebars should be able to both be present?  I
> think it's probably okay, since sidebars are designed to be persistent
> information added to a page.  I think it would be annoying to have a sidebar I
> liked, but then be afraid to activate another popup because it closes that
> sidebar... and it would be even weirder for the case of "pinned" sidebars
> (though we'll still have to address something similar there when a new sidebar
> opens).

Done. I had misunderstood the ownership issue. We are allowing again opening an
extension popup without closing the visible sidebar. About the popup owner, as
sidebarManager owns the sidebars, we don't need to assign it to the
toolbarActionsBar, so we are not setting it any more if it's a sidebar.

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/sidebar_manager.cc (right):

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/sidebar_manager.cc:41: if (container)
On 2015/07/07 21:39:29, Devlin wrote:
> There's a check in HideSidebar() that does this, so you don't have to do it
> here, too.

Done.

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/sidebar_manager.h (right):

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/sidebar_manager.h:43: // tab).
On 2015/07/07 21:39:29, Devlin wrote:
> What is a mini tab?

Done. Removed the wrong comment.

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/sidebar_manager.h:44: void
HideSidebar(content::WebContents* tab);
On 2015/07/07 21:39:29, Devlin wrote:
> Let's rename this HideSidebarForTab().

Done.

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/sidebar_manager_unittest.cc (right):

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/sidebar_manager_unittest.cc:101:
TEST_F(SidebarManagerTest, CreateSidebar) {
On 2015/07/07 21:39:30, Devlin wrote:
> Won't this be implicitly tested by any other test?

Done.

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/sidebar_manager_unittest.cc:111:
chrome::NewTab(browser());
On 2015/07/07 21:39:30, Devlin wrote:
> prefer BrowserWithTestWindowTest::AddTab().

Done.

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/sidebar_manager_unittest.cc:130:
TEST_F(SidebarManagerTest, SidebarOnInactiveTab) {
On 2015/07/07 21:39:30, Devlin wrote:
> All these tests can probably be combined into one.

Done.

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/ui/exte...
File chrome/browser/ui/extensions/extension_action_view_controller.cc (right):

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_action_view_controller.cc:331: bool
already_showing = is_showing_popup();
On 2015/07/07 21:39:30, Devlin wrote:
> I think we should just re-brand most of these popup things to apply to either
a
> popup or a sidebar.  Something like is_showing_content().  That will simplify
> quite a few of the checks later on.  You shouldn't have to add much new logic
> for the sidebar.

We are having some doubts about mixing both functions in just one
(is_showing_content()), instead of maybe having another is_showing_sidebar(), as
we do that check in other parts.

For instance, in this TriggerPpopupWithUrl() if we are already showing a popup
we return; but if we are showing a sidebar, we close it. If we merge both
functions, we wouldn't be able to distinguish between both cases, and might need
some more checks after that. It happens quite the same with some other specific
popup things.

Maybe I'm missing some point here, so would be great if you let us know if this
is_showing_sidebar() approach makes sense.

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_action_view_controller.cc:451: this,
base::Bind(&ExtensionActionViewController::OnPopupShown,
On 2015/07/07 21:39:30, Devlin wrote:
> I don't think we should have actions with a sidebar forced out.  It makes it
> impossible to reason about reordering.

Could you please elaborate a bit more what you mean about this comment?

This current implementation slides out and in the browserAction from the
overview for hidden-in-menu extensions while the sidebar is visible (in the same
way that popups).

There is a case on which we are on a temporary state on which the attribution
was not explicit, and we have some doubts the expected behaviour to implement.
Clicking first on a hidden extension-1 that opens a sidebar (its pressed
browserAction slides out from the overflow), and after that, clicking on another
hidden extension-2 that opens a popup, slides out its button and hides
extension-1 one. If the popup is closed, we are seeing the sidebar visible
without any attribution.

We have seen that having 2 icons outside for the case of a simultaneous open
popup and sidebar makes all the popActions too complicated, so we understand
that in this case the expectable behaviour is sliding out again the sidebar
extension browserAction when the popup is closed. Is this correct?

[Devlin, 2015-07-10 20:18:42]

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/sidebar_browsertest.cc (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_browsertest.cc:34: class SidebarTest : public
ExtensionBrowserTest {
On 2015/07/09 22:15:50, ltilve wrote:
> 
> > Is there a reason the browser tests can't be in this patch set?
> 
> I have moved them to the next patch set because as they use Press() on the
> BrowserAction to show/hide the sidebars, without the UI part, this
> HasSidebarForCurrentTab() is false and the tests are not passing. Should I
> include them in any case, expecting false for HasSidebarForCurrentTab() after
> the click?

I don't see why it should.  HasSidebarForCurrentTab() is based solely on the
SidebarManager, which can set a sidebar independent of whether or not the
sidebar is visibly shown in the UI.  Handling the sidebar creation is done in
the ExtensionActionViewController, which is part of this patch.  So you should
be able to test it without it failing.

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/ui/exte...
File chrome/browser/ui/extensions/extension_action_view_controller.cc (right):

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_action_view_controller.cc:331: bool
already_showing = is_showing_popup();
On 2015/07/09 22:15:51, ltilve wrote:
> On 2015/07/07 21:39:30, Devlin wrote:
> > I think we should just re-brand most of these popup things to apply to
either
> a
> > popup or a sidebar.  Something like is_showing_content().  That will
simplify
> > quite a few of the checks later on.  You shouldn't have to add much new
logic
> > for the sidebar.
> 
> We are having some doubts about mixing both functions in just one
> (is_showing_content()), instead of maybe having another is_showing_sidebar(),
as
> we do that check in other parts.
> 
> For instance, in this TriggerPpopupWithUrl() if we are already showing a popup
> we return; but if we are showing a sidebar, we close it. If we merge both
> functions, we wouldn't be able to distinguish between both cases, and might
need
> some more checks after that. It happens quite the same with some other
specific
> popup things.
> 
> Maybe I'm missing some point here, so would be great if you let us know if
this
> is_showing_sidebar() approach makes sense.

We actually do close the popup, too - it's just handled as part of
HideActivePopup().  And we can still have some sidebar specific logic,
obviously, just most of it shouldn't be.  So in this case, it would be something
like:

bool already_showing = is_showing_content();
bool use_sidebar = exetnsion_action_->open_in_sidebar();
HideActivePopup();

if (already_showing) {
  if (use_sidebar)
    sidebar_manager->HideSidebar(web_contents);
  return;
}

Are there places, other than the showing/hiding of the popup/sidebar, that we
care about whether it's a popup or a sidebar?

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_action_view_controller.cc:451: this,
base::Bind(&ExtensionActionViewController::OnPopupShown,
On 2015/07/09 22:15:51, ltilve wrote:
> On 2015/07/07 21:39:30, Devlin wrote:
> > I don't think we should have actions with a sidebar forced out.  It makes it
> > impossible to reason about reordering.
> 
> Could you please elaborate a bit more what you mean about this comment?
> 
> This current implementation slides out and in the browserAction from the
> overview for hidden-in-menu extensions while the sidebar is visible (in the
same
> way that popups).
> 
> There is a case on which we are on a temporary state on which the attribution
> was not explicit, and we have some doubts the expected behaviour to implement.
> Clicking first on a hidden extension-1 that opens a sidebar (its pressed
> browserAction slides out from the overflow), and after that, clicking on
another
> hidden extension-2 that opens a popup, slides out its button and hides
> extension-1 one. If the popup is closed, we are seeing the sidebar visible
> without any attribution.
> 
> We have seen that having 2 icons outside for the case of a simultaneous open
> popup and sidebar makes all the popActions too complicated, so we understand
> that in this case the expectable behaviour is sliding out again the sidebar
> extension browserAction when the popup is closed. Is this correct?

Nah.  Having any kind of slid out action for a persistent and non modal thing
(like a sidebar) just doesn't really make sense, because it leads to too many
confusing instances that are really hard to explain to the user.  In the case
you described, you sometimes have the extension shown, and sometimes not - based
on the presence of a popup.  From a UI standpoint, why should that affect it? 
And more complicatedly, if you have the action slid out, what happens if the
user tries to reorder it?  Or reorder others around it?  It's just very
difficult to come up with logical flows that would make intuitive sense to a
user.  Instead, we should just not worry about sliding out the extension that
owns the sidebar.  Since it's gated on user action, the attribution is less
important.

[ltilve, 2015-07-17 16:26:23]

Thanks for the review.

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/sidebar_browsertest.cc (right):

https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/sidebar_browsertest.cc:34: class SidebarTest : public
ExtensionBrowserTest {
Done. We have created two separated tests for the case of the standard behaviour
of clicking to show/hide the sidebar, and to check open_in_sidebar. Please let
us know if you think it's better to merge them as a single one.

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/ui/exte...
File chrome/browser/ui/extensions/extension_action_view_controller.cc (right):

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_action_view_controller.cc:331: bool
already_showing = is_showing_popup();
We have done some refactoring to increase readability and to avoid redundant
code. We have introduced TriggerSidebarWithUrl with specific sidebar logic and
we don't need duplicated checks for is_sidebar, or if we are using the
redesigned toolbar.

https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_action_view_controller.cc:451: this,
base::Bind(&ExtensionActionViewController::OnPopupShown,
On 2015/07/10 20:18:42, Devlin wrote:
> Nah.  Having any kind of slid out action for a persistent and non modal thing
> (like a sidebar) just doesn't really make sense, because it leads to too many
> confusing instances that are really hard to explain to the user.  In the case
> you described, you sometimes have the extension shown, and sometimes not -
based
> on the presence of a popup.  From a UI standpoint, why should that affect it? 
> And more complicatedly, if you have the action slid out, what happens if the
> user tries to reorder it?  Or reorder others around it?  It's just very
> difficult to come up with logical flows that would make intuitive sense to a
> user.  Instead, we should just not worry about sliding out the extension that
> owns the sidebar.  Since it's gated on user action, the attribution is less
> important.

Done. We are not sliding out the browser action now for sidebar extensions.

[benjamin.j.mccann, 2015-07-22 20:13:36]

On 2015/07/17 16:26:23, ltilve wrote:
> Thanks for the review.
> 
>
https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
> File chrome/browser/extensions/sidebar_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1168383002/diff/80001/chrome/browser/extensio...
> chrome/browser/extensions/sidebar_browsertest.cc:34: class SidebarTest :
public
> ExtensionBrowserTest {
> Done. We have created two separated tests for the case of the standard
behaviour
> of clicking to show/hide the sidebar, and to check open_in_sidebar. Please let
> us know if you think it's better to merge them as a single one.
> 
>
https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/ui/exte...
> File chrome/browser/ui/extensions/extension_action_view_controller.cc (right):
> 
>
https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/ui/exte...
> chrome/browser/ui/extensions/extension_action_view_controller.cc:331: bool
> already_showing = is_showing_popup();
> We have done some refactoring to increase readability and to avoid redundant
> code. We have introduced TriggerSidebarWithUrl with specific sidebar logic and
> we don't need duplicated checks for is_sidebar, or if we are using the
> redesigned toolbar.
> 
>
https://codereview.chromium.org/1168383002/diff/120001/chrome/browser/ui/exte...
> chrome/browser/ui/extensions/extension_action_view_controller.cc:451: this,
> base::Bind(&ExtensionActionViewController::OnPopupShown,
> On 2015/07/10 20:18:42, Devlin wrote:
> > Nah.  Having any kind of slid out action for a persistent and non modal
thing
> > (like a sidebar) just doesn't really make sense, because it leads to too
many
> > confusing instances that are really hard to explain to the user.  In the
case
> > you described, you sometimes have the extension shown, and sometimes not -
> based
> > on the presence of a popup.  From a UI standpoint, why should that affect
it? 
> > And more complicatedly, if you have the action slid out, what happens if the
> > user tries to reorder it?  Or reorder others around it?  It's just very
> > difficult to come up with logical flows that would make intuitive sense to a
> > user.  Instead, we should just not worry about sliding out the extension
that
> > owns the sidebar.  Since it's gated on user action, the attribution is less
> > important.
> 
> Done. We are not sliding out the browser action now for sidebar extensions.

Just a quick reminder that this is ready for another look. Thanks!

[Devlin, 2015-07-22 20:16:36]

Hey Ben, I've been holding off on this until we finish reworking the doc, since
there could be some changes.  In particular, kalman@ was leaning towards a
per-window UI, rather than a per-tab UI, which would result in a decent size
change in this patch.  Once the doc is in a stable state, we can continue on
with this patch.  Sound good?

[ltilve, 2015-07-27 18:58:20]

Hi Devlin, thanks for your comments. It seems that everyone was fine with the
per-window approach, so we have modified the code to provide this implementation
(we have also updated https://codereview.chromium.org/1169823005/ for the UI
part).

Doing so, and having now only one sidebar per window, it seems that we could
avoid most the sidebarManager logic and have the code reduced by handling the
container at the extension_action_view_controller as for the case of popups. 

Because of that, we don't have any more methods as
SidebarManager::CreateSidebar() to use directly from unittests, so we have
updated those tests with the new behaviour when switching tabs, and moved them
into the browsertests. Please let us know if you are fine with this, or we
should do it somehow else.

[Devlin, 2015-07-28 22:59:02]

On 2015/07/27 18:58:20, ltilve wrote:
> Hi Devlin, thanks for your comments. It seems that everyone was fine with the
> per-window approach, so we have modified the code to provide this
implementation
> (we have also updated https://codereview.chromium.org/1169823005/ for the UI
> part).
> 
> Doing so, and having now only one sidebar per window, it seems that we could
> avoid most the sidebarManager logic and have the code reduced by handling the
> container at the extension_action_view_controller as for the case of popups. 
> 
> Because of that, we don't have any more methods as
> SidebarManager::CreateSidebar() to use directly from unittests, so we have
> updated those tests with the new behaviour when switching tabs, and moved them
> into the browsertests. Please let us know if you are fine with this, or we
> should do it somehow else.

I'd still really like to see the doc updated to reflect this.  Going forward,
it'd be good to make sure that we are in sync with our proposal and our
implementation.

[benjamin.j.mccann, 2015-07-28 23:10:08]

> I'd still really like to see the doc updated to reflect this.  Going forward,
> it'd be good to make sure that we are in sync with our proposal and our
> implementation.

Yep, absolutely. I think I already removed any reference to per-tab, but will
double check and will flesh out the reasoning for per-window. I also added a
screenshot to the doc per your earlier suggestion.

[Devlin, 2015-07-30 17:56:46]

https://codereview.chromium.org/1168383002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/sidebar_browsertest.cc (right):

https://codereview.chromium.org/1168383002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/sidebar_browsertest.cc:29: class SidebarTest : public
ExtensionBrowserTest {
Why does this need to be a browser test?

https://codereview.chromium.org/1168383002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/sidebar_browsertest.cc:103:
IN_PROC_BROWSER_TEST_F(SidebarTest, CreateDisabledSidebar) {
What about testing multiple extensions with sidebars?

https://codereview.chromium.org/1168383002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/sidebar_container.cc (right):

https://codereview.chromium.org/1168383002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/sidebar_container.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
Umm... this class doesn't do anything other than initialize the host.  Why do we
need it?

https://codereview.chromium.org/1168383002/diff/180001/chrome/browser/ui/exte...
File chrome/browser/ui/extensions/extension_action_view_controller.cc (right):

https://codereview.chromium.org/1168383002/diff/180001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_action_view_controller.cc:222: return
GetPreferredPopupViewController()->TriggerSidebarWithUrl(
The fact that so much of this code seems to be copy-pasted with s/popup/sidebar
suggests it could be factored better...

https://codereview.chromium.org/1168383002/diff/180001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_action_view_controller.cc:355:
PressButtonWithSlideOutIfEnabled(
Wait, what?  This calls into PressButtonWithSlideOutIfEnabled, but that method
ignores the case of the sidebar.  This means that if you enable the extension
action redesign, |closure| will never be run.

https://codereview.chromium.org/1168383002/diff/180001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_action_view_controller.cc:388: // In the
traditional toolbar, page actions only know how to close their own
Obviously from copy paste (another reason not to do it), but this won't work
here.  A page action will need to close another sidebar, too.

[ltilve, 2015-08-07 16:15:02]

Thanks for the reviews and sorry in advance if we were not understanding
correctly any of your comments :)

https://codereview.chromium.org/1168383002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/sidebar_browsertest.cc (right):

https://codereview.chromium.org/1168383002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/sidebar_browsertest.cc:29: class SidebarTest : public
ExtensionBrowserTest {
On 2015/07/30 17:56:46, Devlin wrote:
> Why does this need to be a browser test?

We have been earlier following the suggestion of testing opening and closing
sidebars through SidebarManager API with unittests. Now we don't have any more a
SidebarManager to be able to force creation of sidebars, and testing inside
unittests any popup/sidebar logic seems to be failing because at
ExtensionActionViewController::ExecuteAction the ExtensionAction::ACTION_NONE
type is returned due to null web_contents.

For those reasons we are thinking that we need it to be a browser test. Maybe we
are not properly setting anything for it, please let us know if we are wrong
about this.

https://codereview.chromium.org/1168383002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/sidebar_browsertest.cc:103:
IN_PROC_BROWSER_TEST_F(SidebarTest, CreateDisabledSidebar) {
On 2015/07/30 17:56:46, Devlin wrote:
> What about testing multiple extensions with sidebars?

Done.

https://codereview.chromium.org/1168383002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/sidebar_container.cc (right):

https://codereview.chromium.org/1168383002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/sidebar_container.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/07/30 17:56:46, Devlin wrote:
> Umm... this class doesn't do anything other than initialize the host.  Why do
we
> need it?

Done. We have refactored out sidebarContainer now.

https://codereview.chromium.org/1168383002/diff/180001/chrome/browser/ui/exte...
File chrome/browser/ui/extensions/extension_action_view_controller.cc (right):

https://codereview.chromium.org/1168383002/diff/180001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_action_view_controller.cc:222: return
GetPreferredPopupViewController()->TriggerSidebarWithUrl(
On 2015/07/30 17:56:46, Devlin wrote:
> The fact that so much of this code seems to be copy-pasted with
s/popup/sidebar
> suggests it could be factored better...

We need to handle a case which opens a popup when a sidebar is already shown.
For this case, we were thinking that maybe is more clear having separate methods
for sidebar and popups.

https://codereview.chromium.org/1168383002/diff/180001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_action_view_controller.cc:355:
PressButtonWithSlideOutIfEnabled(
On 2015/07/30 17:56:46, Devlin wrote:
> Wait, what?  This calls into PressButtonWithSlideOutIfEnabled, but that method
> ignores the case of the sidebar.  This means that if you enable the extension
> action redesign, |closure| will never be run.

Fixed. You were totally right, good catch.

https://codereview.chromium.org/1168383002/diff/180001/chrome/browser/ui/exte...
chrome/browser/ui/extensions/extension_action_view_controller.cc:388: // In the
traditional toolbar, page actions only know how to close their own
On 2015/07/30 17:56:46, Devlin wrote:
> Obviously from copy paste (another reason not to do it), but this won't work
> here.  A page action will need to close another sidebar, too.

Done. Because the sidebar extension is only instantiated using browser action,
we modified it to handle page action as a error.
Also, toolbar_actions_bar is null only if this is a page action without
the toolbar redesigned turned on, so we handle it as a error in the sidebar
case.