Make a platform-independent ToolbarActionViewController

Make a platform-independent ToolbarActionViewController

Create a platform-independent ToolbarActionViewController
and platform-independent ExtensionActionViewController, and
wire the controller into the cocoa implementation. Add
platform specific implementations as needed.

BUG=422976
Committed: https://crrev.com/09530741dc9ed3963ba9ef9929efe1bdc99eb309
Cr-Commit-Position: refs/heads/master@{#302462}

[Devlin, 2014-10-23 17:02:05]

Patchset #1 (id:1) has been deleted

[Devlin, 2014-10-23 17:02:15]

Patchset #1 (id:20001) has been deleted

[Devlin, 2014-10-23 17:02:32]

Patchset #1 (id:40001) has been deleted

[Devlin, 2014-10-23 17:02:41]

Patchset #1 (id:60001) has been deleted

[Devlin, 2014-10-23 17:02:50]

Patchset #1 (id:80001) has been deleted

[Devlin, 2014-10-23 17:02:58]

Patchset #1 (id:100001) has been deleted

[Devlin, 2014-10-23 17:03:06]

Patchset #1 (id:120001) has been deleted

[Devlin, 2014-10-23 17:03:12]

Patchset #1 (id:140001) has been deleted

[Devlin, 2014-10-23 17:03:18]

Patchset #1 (id:160001) has been deleted

[Devlin, 2014-10-23 17:03:23]

Patchset #1 (id:180001) has been deleted

[Devlin, 2014-10-23 17:07:06]

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

[Devlin, 2014-10-23 18:18:50]

Scott, please take a look when you can.  This would be a pretty large step
towards platform-independence for a good amount of the toolbar actions logic.

The views side is primarily a refactor.  If you can, I'd also like you to take a
high-level look at the cocoa, to see if the overall architecture looks good.
(It's a shame in my mind that we need so many different versions of many of
these classes, but there's not really a good way around it with different view
platforms).

[sky, 2014-10-23 20:24:30]

Finnur should look at this too.

https://codereview.chromium.org/670463004/diff/200001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/extension_action_view_controller.h (right):

https://codereview.chromium.org/670463004/diff/200001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_action_view_controller.h:22: class
ExtensionActionViewController
Add a description.

https://codereview.chromium.org/670463004/diff/200001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_action_view_controller.h:71: virtual bool
IsShowingPopup() const = 0;
Is there a reason you're going with subclassing instead of moving this to a
delegate that is owned by ExtensionActionViewController? A delegate seems a bit
cleaner for the case you have here.

https://codereview.chromium.org/670463004/diff/200001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_action_view_controller.h:86: virtual void
OnDelegateSet() {}
nit: don't inline virtual methods.

[Devlin, 2014-10-23 20:50:32]

https://codereview.chromium.org/670463004/diff/200001/chrome/browser/ui/exten...
File chrome/browser/ui/extensions/extension_action_view_controller.h (right):

https://codereview.chromium.org/670463004/diff/200001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_action_view_controller.h:22: class
ExtensionActionViewController
On 2014/10/23 20:24:30, sky wrote:
> Add a description.

Done.

https://codereview.chromium.org/670463004/diff/200001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_action_view_controller.h:71: virtual bool
IsShowingPopup() const = 0;
On 2014/10/23 20:24:30, sky wrote:
> Is there a reason you're going with subclassing instead of moving this to a
> delegate that is owned by ExtensionActionViewController? A delegate seems a
bit
> cleaner for the case you have here.

Two reasons:
- I thought a delegate might get confusing, since we already have a
ToolbarActionViewDelegate.  We can, of course, have a "PopupDelegate" and a
"ViewDelegate", but I wasn't sure that would be cleaner.
- ExtensionActionViewDelegate can't (directly) implement the methods
GetPopupView(), IsMenuRunning(), and RegisterCommand() (since these are handled
differently on cocoa/views).  These, too, could be ported to the delegate, with
the implementation in ExtensionActionViewController simply being a forward to
those, but it seemed a little messier that way.

Would you prefer the delegate solution?  (Has the advantage of giving us a
concrete controller, which is quite nice.)

https://codereview.chromium.org/670463004/diff/200001/chrome/browser/ui/exten...
chrome/browser/ui/extensions/extension_action_view_controller.h:86: virtual void
OnDelegateSet() {}
On 2014/10/23 20:24:30, sky wrote:
> nit: don't inline virtual methods.

Thought the rule was "don't inline non-empty virtual methods" (which is what
clang warns on).  But it appears the style guide is ambiguous on it, so OWNERs
rules. :)

Done.

[Devlin, 2014-10-24 00:19:03]

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

[Devlin, 2014-10-24 00:19:05]

Finnur, please also take a look (like sky, ideally at views and also high-level
architecture).  Feel free to also weigh in on sky's comment. :)

[Finnur, 2014-10-24 09:50:22]

I'm supportive and happy Devlin is taking a thorough look at restructuring,
untangling and modernizing these classes and unafraid of shaking things up.

I'm not sure what is right to do in the delegate issue Scott brought up, though.
I believe I can trust whatever decision you reach.

[sky, 2014-10-24 14:41:44]

On Thu, Oct 23, 2014 at 1:50 PM,  <rdevlin.cronin@chromium.org> wrote:
>
>
https://codereview.chromium.org/670463004/diff/200001/chrome/browser/ui/exten...
> File chrome/browser/ui/extensions/extension_action_view_controller.h
> (right):
>
>
https://codereview.chromium.org/670463004/diff/200001/chrome/browser/ui/exten...
> chrome/browser/ui/extensions/extension_action_view_controller.h:22:
> class ExtensionActionViewController
> On 2014/10/23 20:24:30, sky wrote:
>>
>> Add a description.
>
>
> Done.
>
>
https://codereview.chromium.org/670463004/diff/200001/chrome/browser/ui/exten...
> chrome/browser/ui/extensions/extension_action_view_controller.h:71:
> virtual bool IsShowingPopup() const = 0;
> On 2014/10/23 20:24:30, sky wrote:
>>
>> Is there a reason you're going with subclassing instead of moving this
>
> to a
>>
>> delegate that is owned by ExtensionActionViewController? A delegate
>
> seems a bit
>>
>> cleaner for the case you have here.
>
>
> Two reasons:
> - I thought a delegate might get confusing, since we already have a
> ToolbarActionViewDelegate.  We can, of course, have a "PopupDelegate"
> and a "ViewDelegate", but I wasn't sure that would be cleaner.
> - ExtensionActionViewDelegate can't (directly) implement the methods
> GetPopupView(), IsMenuRunning(), and RegisterCommand() (since these are
> handled differently on cocoa/views).  These, too, could be ported to the
> delegate, with the implementation in ExtensionActionViewController
> simply being a forward to those, but it seemed a little messier that
> way.
>
> Would you prefer the delegate solution?  (Has the advantage of giving us
> a concrete controller, which is quite nice.)

One more comment. Wouldn't an explicit delegate make testing a bit easier too?

  -Scott
>
>
https://codereview.chromium.org/670463004/diff/200001/chrome/browser/ui/exten...
> chrome/browser/ui/extensions/extension_action_view_controller.h:86:
> virtual void OnDelegateSet() {}
> On 2014/10/23 20:24:30, sky wrote:
>>
>> nit: don't inline virtual methods.
>
>
> Thought the rule was "don't inline non-empty virtual methods" (which is
> what clang warns on).  But it appears the style guide is ambiguous on
> it, so OWNERs rules. :)
>
> Done.
>
> https://codereview.chromium.org/670463004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Devlin, 2014-10-24 15:54:30]

On 2014/10/24 14:41:44, sky wrote:
> One more comment. Wouldn't an explicit delegate make testing a bit easier too?
> 
>   -Scott

For these purposes, probably not.  Since the delegate wouldn't have much to do
other than fulfill the virtual methods in ExtensionActionViewController, the
testing would probably be best in an ExtensionActionViewController test, so that
it could be platform-independent.

That said, I've got some time today, so I'll try out a delegate set up if it
still sounds good to you. :)

[Devlin, 2014-10-24 17:35:02]

On 2014/10/24 15:54:30, Devlin wrote:
> On 2014/10/24 14:41:44, sky wrote:
> > One more comment. Wouldn't an explicit delegate make testing a bit easier
too?
> > 
> >   -Scott
> 
> For these purposes, probably not.  Since the delegate wouldn't have much to do
> other than fulfill the virtual methods in ExtensionActionViewController, the
> testing would probably be best in an ExtensionActionViewController test, so
that
> it could be platform-independent.
> 
> That said, I've got some time today, so I'll try out a delegate set up if it
> still sounds good to you. :)

Patch Set 3 shows what this would be like with a platform delegate.  WDYT?

[sky, 2014-10-24 20:16:40]

I like it, LGTM. Please get someone more knowledgable of the Mac side to look it
over too.

[Devlin, 2014-10-30 20:11:14]

Patchset #5 (id:280001) has been deleted

[Devlin, 2014-10-30 20:33:19]

Patchset #5 (id:300001) has been deleted

[Devlin, 2014-10-30 21:12:59]

On 2014/10/24 20:16:40, sky wrote:
> I like it, LGTM. Please get someone more knowledgable of the Mac side to look
it
> over too.

Sorry for the delay on this one; had to debug mac issues - which is difficult
when one doesn't have a mac.

[Devlin, 2014-10-30 21:14:25]

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

[Devlin, 2014-10-30 21:14:26]

Avi, mind taking a look at the cocoa?  There's some background in the bug, but,
in short, we want to have the ability to make component actions in the toolbar
(not just extension actions), and need to ideally:
a) abstract out the idea of extensions from the views code,
b) make the platforms share as much logic as possible.

[Avi (use Gerrit), 2014-10-30 23:21:26]

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/browser_action_button.h (right):

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/browser_action_button.h:60:
menuController:(ExtensionActionContextMenuController*)menuController;
What are the ownership semantics of the items being passed in here?

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (right):

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:17: #import
"chrome/browser/ui/cocoa/extensions/browser_actions_controller.h"
alphabetize prev 4 lines

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:324:
CHECK(viewController_);
Why CHECK and not DCHECK? Usually that's saved for potential security issues
which this is not.

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:481:
CHECK(extensionAction)
Again, why not DCHECK?

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/extensions/extension_action_platform_delegate_cocoa.mm
(right):

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_action_platform_delegate_cocoa.mm:28:
return popup != nil ? [popup view] : nullptr;
NativeView is an NSView, so nil rather than nullptr.

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:87: bool
PageActionHasBoomarkManagerPrivate(PageActionDecoration* decoration) {
Fix typo: "boomark"?

[Devlin, 2014-10-31 17:50:43]

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/browser_action_button.h (right):

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/browser_action_button.h:60:
menuController:(ExtensionActionContextMenuController*)menuController;
On 2014/10/30 23:21:25, Avi wrote:
> What are the ownership semantics of the items being passed in here? 

Ah, good point (related: just to confirm, there isn't a Pass() method for a
scoped_nsobject right?)

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (right):

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:17: #import
"chrome/browser/ui/cocoa/extensions/browser_actions_controller.h"
On 2014/10/30 23:21:25, Avi wrote:
> alphabetize prev 4 lines

Whoops! (Clearly I became reliant on the presubmit to tell me.) Done.

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:324:
CHECK(viewController_);
On 2014/10/30 23:21:25, Avi wrote:
> Why CHECK and not DCHECK? Usually that's saved for potential security issues
> which this is not.

Was just matching the previous CHECK(extensionAction_). Changed to a DCHECK.

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:481:
CHECK(extensionAction)
On 2014/10/30 23:21:26, Avi wrote:
> Again, why not DCHECK?

Ditto and done. :)

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/extensions/extension_action_platform_delegate_cocoa.mm
(right):

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_action_platform_delegate_cocoa.mm:28:
return popup != nil ? [popup view] : nullptr;
On 2014/10/30 23:21:26, Avi wrote:
> NativeView is an NSView, so nil rather than nullptr.

Done.

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/670463004/diff/320001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:87: bool
PageActionHasBoomarkManagerPrivate(PageActionDecoration* decoration) {
On 2014/10/30 23:21:26, Avi wrote:
> Fix typo: "boomark"?

It was just in Halloween spirit! ;)

[Avi (use Gerrit), 2014-11-02 07:17:49]

lgtm

[Devlin, 2014-11-03 18:33:03]

rdevlin.cronin@chromium.org changed reviewers:
- finnur@chromium.org

[Devlin, 2014-11-03 18:34:59]

On 2014/10/24 09:50:22, Finnur wrote:
> I believe I can trust whatever decision you reach.

FYI Finnur says he's good with what we came up with; moved him to cc.

[Devlin, 2014-11-03 18:35:12]

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

[commit-bot: I haz the power, 2014-11-03 18:35:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/670463004/360001

[commit-bot: I haz the power, 2014-11-03 19:23:40]

Committed patchset #7 (id:360001)

[commit-bot: I haz the power, 2014-11-03 19:24:54]

Patchset 7 (id:??) landed as
https://crrev.com/09530741dc9ed3963ba9ef9929efe1bdc99eb309
Cr-Commit-Position: refs/heads/master@{#302462}