[ios] Use dispatcher for ToolsMenu commands.

[ios] Use dispatcher for ToolsMenu commands.

CL is pretty straightforward, but I want to note that I removed toolbar actions since I
don't think they will be needed in this case anymore.

BUG=682880
Review-Url: https://codereview.chromium.org/2785213003
Cr-Commit-Position: refs/heads/master@{#461838}
Committed: https://chromium.googlesource.com/chromium/src/+/b3edea41ec63503945b1135b34ccf1122098512d

[sczs, 2017-03-31 00:41:52]

Description was changed from

==========
[ios] Use dispatcher for ToolsMenu commands.

BUG= 682880
==========

to

==========
[ios] Use dispatcher for ToolsMenu commands.

CL is pretty straightforward, but I want to note that I removed toolbar actions
since I don't think they will be needed in this case anymore.

BUG= 682880
==========

[sczs, 2017-03-31 00:41:52]

sczs@chromium.org changed reviewers:
+ edchin@chromium.org, lpromero@chromium.org, marq@chromium.org,
rohitrao@chromium.org

[sczs, 2017-03-31 00:51:11]

Description was changed from

==========
[ios] Use dispatcher for ToolsMenu commands.

CL is pretty straightforward, but I want to note that I removed toolbar actions
since I don't think they will be needed in this case anymore.

BUG= 682880
==========

to

==========
[ios] Use dispatcher for ToolsMenu commands.

CL is pretty straightforward, but I want to note that I removed toolbar actions
since I
don't think they will be needed in this case anymore.

BUG= 682880
==========

[sczs, 2017-03-31 01:44:50]

PTAL

[edchin, 2017-03-31 02:44:57]

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/presenters/menu_presentation_controller.h
(right):

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/presenters/menu_presentation_controller.h:24: // The
dispatcher for this presentation controller
nit: period.

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm (right):

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:209: #pragma mark
- ToolsMenuCommands
These methods can just be marked as Private methods. 
They aren't actually implementations of the ToolsMenuCommands.

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/tools/tools_coordinator.h (right):

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/tools_coordinator.h:8: #import
<Foundation/Foundation.h>
Remove.

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/tools_coordinator.h:12: @protocol
ToolsMenuCommands;
Remove.

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/tools/tools_coordinator.mm (right):

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/tools_coordinator.mm:34:
static_cast<id>(self.browser->dispatcher());
This class isn't registering for any dispatches.

[edchin, 2017-03-31 02:50:33]

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm (right):

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:72: [dispatcher
stopDispatchingToTarget:self];
[self.browser->dispatcher() stopDispatchingToTarget:self];
fits in single line.

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/tools/tools_coordinator.mm (right):

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/tools_coordinator.mm:44: [super stop];
[self.browser->dispatcher() stopDispatchingToTarget:self];

[lpromero, 2017-03-31 06:34:39]

lgtm

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/commands/tools_menu_commands.h (right):

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/commands/tools_menu_commands.h:10: @optional
Why @optional?
When adding optional methods to a protocol, it is useful to also make the
protocol conform to the NSObject protocol. That way you can do:
id<MyProtocol> some = …
if ([some respondsToSelector:@selector(protocolMethod)]) {
…

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/tools/tools_coordinator.mm (right):

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/tools_coordinator.mm:34:
static_cast<id>(self.browser->dispatcher());
On 2017/03/31 02:44:57, edchin wrote:
> This class isn't registering for any dispatches.

Why should it?

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/tools_coordinator.mm:44: [super stop];
On 2017/03/31 02:50:33, edchin wrote:
> [self.browser->dispatcher() stopDispatchingToTarget:self];

Since this controller didn't register for dispatch, this seems unnecessary.

[edchin, 2017-03-31 15:01:30]

lgtm.

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/tools/tools_coordinator.mm (right):

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/tools_coordinator.mm:34:
static_cast<id>(self.browser->dispatcher());
On 2017/03/31 06:34:38, lpromero wrote:
> On 2017/03/31 02:44:57, edchin wrote:
> > This class isn't registering for any dispatches.
> 
> Why should it?

Ahh. I see the registration had already landed previously in another class.

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/tools_coordinator.mm:44: [super stop];
On 2017/03/31 06:34:38, lpromero wrote:
> On 2017/03/31 02:50:33, edchin wrote:
> > [self.browser->dispatcher() stopDispatchingToTarget:self];
> 
> Since this controller didn't register for dispatch, this seems unnecessary.

Acknowledged.

[sczs, 2017-03-31 17:32:57]

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/commands/tools_menu_commands.h (right):

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/commands/tools_menu_commands.h:10: @optional
On 2017/03/31 06:34:38, lpromero wrote:
> Why @optional?
> When adding optional methods to a protocol, it is useful to also make the
> protocol conform to the NSObject protocol. That way you can do:
> id<MyProtocol> some = …
> if ([some respondsToSelector:@selector(protocolMethod)]) {
> …

I was thinking about the same. Depending on how we group these protocol methods
I might change some to required. This because some other Classes/Coordinators
only need to implement some of them, like the PresentationController with close,
or the TabGrid Menu won't have a shareMenu implementation,etc.

Thanks for the NSObject protocol tip, makes a lot of sense!

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/presenters/menu_presentation_controller.h
(right):

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/presenters/menu_presentation_controller.h:24: // The
dispatcher for this presentation controller
On 2017/03/31 02:44:57, edchin wrote:
> nit: period.

Done.

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm (right):

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/toolbar/toolbar_coordinator.mm:72: [dispatcher
stopDispatchingToTarget:self];
On 2017/03/31 02:50:33, edchin wrote:
> [self.browser->dispatcher() stopDispatchingToTarget:self];
> fits in single line.

You're right, and there's not really a need for a new variable there! Done.

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm (right):

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:209: #pragma mark
- ToolsMenuCommands
On 2017/03/31 02:44:57, edchin wrote:
> These methods can just be marked as Private methods. 
> They aren't actually implementations of the ToolsMenuCommands.

I like the idea, thanks! Done.

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/tools/tools_coordinator.h (right):

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/tools_coordinator.h:8: #import
<Foundation/Foundation.h>
On 2017/03/31 02:44:57, edchin wrote:
> Remove.

Done.

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/tools_coordinator.h:12: @protocol
ToolsMenuCommands;
On 2017/03/31 02:44:57, edchin wrote:
> Remove.

Done.

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/tools/tools_coordinator.mm (right):

https://codereview.chromium.org/2785213003/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tools/tools_coordinator.mm:34:
static_cast<id>(self.browser->dispatcher());
On 2017/03/31 15:01:30, edchin wrote:
> On 2017/03/31 06:34:38, lpromero wrote:
> > On 2017/03/31 02:44:57, edchin wrote:
> > > This class isn't registering for any dispatches.
> > 
> > Why should it?
> 
> Ahh. I see the registration had already landed previously in another class.

Yes, the only thing being done is passing the dispatcher to the VC. Those
dispatcher calls will be handled by the ToolbarCoordinator for now.

[lpromero, 2017-04-03 12:36:47]

lgtm

https://codereview.chromium.org/2785213003/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm (right):

https://codereview.chromium.org/2785213003/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:141:
[self.toolsMenuButton addTarget:self
Did you consider:

[self.toolsMenuButton addTarget:self.dispatcher
                         action:@selector(showToolsMenu)
               forControlEvents:UIControlEventTouchUpInside];

Could remove 30 lines of boilerplate.

[sczs, 2017-04-04 17:56:54]

On 2017/04/03 12:36:47, lpromero wrote:
> lgtm
> 
>
https://codereview.chromium.org/2785213003/diff/20001/ios/clean/chrome/browse...
> File ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm (right):
> 
>
https://codereview.chromium.org/2785213003/diff/20001/ios/clean/chrome/browse...
> ios/clean/chrome/browser/ui/toolbar/toolbar_view_controller.mm:141:
> [self.toolsMenuButton addTarget:self
> Did you consider:
> 
> [self.toolsMenuButton addTarget:self.dispatcher
>                          action:@selector(showToolsMenu)
>                forControlEvents:UIControlEventTouchUpInside];
> 
> Could remove 30 lines of boilerplate.

I just tried that, seemed like a great idea but it's not working in some cases,
and in others there's some unexpected behavior. 
I think it has to do with the fact that dispatcher is nil at the time the action
is added to the button. 

Will investigate this approach later on, for now It would not be a trivial
change.

[sczs, 2017-04-04 18:01:42]

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

[commit-bot: I haz the power, 2017-04-04 18:02:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 19:09:57]

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

[commit-bot: I haz the power, 2017-04-04 19:09:58]

Dry run: This issue passed the CQ dry run.

[sczs, 2017-04-04 19:48:10]

The CQ bit was checked by sczs@chromium.org

[sczs, 2017-04-04 19:48:10]

The patchset sent to the CQ was uploaded after l-g-t-m from edchin@chromium.org,
lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2785213003/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-04 19:49:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 21:10:50]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491335290034950,
"parent_rev": "2c0a8f906209efba7539baf7fe1b270b950082c4", "commit_rev":
"b3edea41ec63503945b1135b34ccf1122098512d"}

[commit-bot: I haz the power, 2017-04-04 21:11:38]

Description was changed from

==========
[ios] Use dispatcher for ToolsMenu commands.

CL is pretty straightforward, but I want to note that I removed toolbar actions
since I
don't think they will be needed in this case anymore.

BUG= 682880
==========

to

==========
[ios] Use dispatcher for ToolsMenu commands.

CL is pretty straightforward, but I want to note that I removed toolbar actions
since I
don't think they will be needed in this case anymore.

BUG= 682880

Review-Url: https://codereview.chromium.org/2785213003
Cr-Commit-Position: refs/heads/master@{#461838}
Committed:
https://chromium.googlesource.com/chromium/src/+/b3edea41ec63503945b1135b34cc...
==========

[commit-bot: I haz the power, 2017-04-04 21:11:41]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/b3edea41ec63503945b1135b34cc...