[iOS Clean] Wired up ContextMenuCommands.

[iOS Clean] Wired up ContextMenuCommands.

This CL updates WebContextMenuCoordinator and its associated mediator
class to support more actions.  It will load URLs and create new
WebStates, but still does not perform corresponding animations to show
newly created Tabs and their loaded content.

BUG=none

Review-Url: https://codereview.chromium.org/2862783002
Cr-Commit-Position: refs/heads/master@{#475257}
Committed: https://chromium.googlesource.com/chromium/src/+/ffa157bf17edd17189c334a3bbdd5ec40d50d07c

[kkhorimoto, 2017-05-04 05:29:12]

kkhorimoto@chromium.org changed reviewers:
+ edchin@chromium.org, marq@chromium.org

[kkhorimoto, 2017-05-04 05:38:01]

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

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/commands/context_menu_commands.h:15: -
(void)openURLInNewTab:(const GURL*)URL;
Do we currently have any mechanism besides DCHECKs in CommandDispatcher to help
avoid naming collisions for commands?  I'm thinking it might be useful to make
more verbose naming here that mention that it's specific to context menu
actions.

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.mm (right):

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.mm:111: // C++
objects or C++ object references, so the URL is passed by pointer.
I know that you want to use NSInvocations here to make this more in line with
our current runtime selector hackery, but it's a bit cumbersome.  In particular,
it forces us to make stylistic compromises.  The style guide says that we should
be passing C++ arguments using const references, but the limitations of
NSInvocation's |-setArgument:atIndex:| disallow us from doing so.  Currently, I
have a ContextMenuCommands interface that takes const pointers, but the style
guide says we should only be using C++ pointer arguments for out-parameters.

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/context_menu/web_context_menu_coordinator.mm
(right):

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/context_menu/web_context_menu_coordinator.mm:142:
std::move(newWebState), opener);
How do we intend to handle showing links that are opened in new tabs?  Will this
need to wait until lpromero irons out the animation mechanisms?

[marq (ping after 24h), 2017-05-04 09:41:28]

Great start!

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

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/commands/context_menu_commands.h:15: -
(void)openURLInNewTab:(const GURL*)URL;
On 2017/05/04 05:38:01, kkhorimoto_ wrote:
> Do we currently have any mechanism besides DCHECKs in CommandDispatcher to
help
> avoid naming collisions for commands?  I'm thinking it might be useful to make
> more verbose naming here that mention that it's specific to context menu
> actions.

My thinking was that commands would be grouped by function, not source. There
should only need to be one 'openURLInNewTab' command; there may be several
places where that command is called from.

The tools menu, which by definition throws together a bunch of unrelated
functions, seems like its dispatcher will have to conform to a bucketload of
protocols.

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.h (right):

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.h:21: // title
and context menu items will be populated from |params|.  |isIncognito|
Let's hold off on incognito support for now.  The pattern we're considering is
to have incognito subclasses instead of having incognito state that needs to be
checked everywhere.

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.mm (right):

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.mm:20:
web::ContextMenuParams _params;
Why doesn't property autosynthesis take care of this?

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.mm:111: // C++
objects or C++ object references, so the URL is passed by pointer.
On 2017/05/04 05:38:01, kkhorimoto_ wrote:
> I know that you want to use NSInvocations here to make this more in line with
> our current runtime selector hackery, but it's a bit cumbersome.  In
particular,
> it forces us to make stylistic compromises.  The style guide says that we
should
> be passing C++ arguments using const references, but the limitations of
> NSInvocation's |-setArgument:atIndex:| disallow us from doing so.  Currently,
I
> have a ContextMenuCommands interface that takes const pointers, but the style
> guide says we should only be using C++ pointer arguments for out-parameters.

There are a couple of alternatives.

(1) Don't use invocations. But we also don't want to be explicitly passing GURL
objects to/from the view controller layer.

(2) Use invocations, but don't pass URLs at all. The coordinator/mediator should
always exist in the context of a single context menu invocation, so the URL
shouldn't need to go to the consumer at all; the semantics of the commands
called from the menu are then (e.g.) "open the current URL in a new tab". 

(3) Define a context object that the consumer knows about, and define a subclass
of it which has a GURL property. The command endpoint downcasts the object and
extracts the URL.

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/context_menu/web_context_menu_coordinator.mm
(right):

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/context_menu/web_context_menu_coordinator.mm:69:
#pragma mark - ContextMenuCommands
Almost all of these aren't the job of the context menu coordinator, I think.
Generally speaking coordinators create objects and connect them together, so
they should only handle commands that create new coordinators or cause existing
ones to stop.

Opening URLs in the current web state seems like a job for the
web_contents_mediator; opening new tabs seems like a job for the (currently
nonexistent) tab mediator or maybe the tab grid mediator.

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/context_menu/web_context_menu_coordinator.mm:142:
std::move(newWebState), opener);
On 2017/05/04 05:38:01, kkhorimoto_ wrote:
> How do we intend to handle showing links that are opened in new tabs?  Will
this
> need to wait until lpromero irons out the animation mechanisms?

The new tab animation needs to be designed; it's similar to other animations
that require showing other tabs and web states (such as swipe to change tabs, or
swipe navigation). But that should be the responsibility of the coordinator /
view controller that's handling the new tab creation, which won't be this one.

[kkhorimoto, 2017-05-05 05:17:54]

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

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/commands/context_menu_commands.h:15: -
(void)openURLInNewTab:(const GURL*)URL;
On 2017/05/04 09:41:28, marq (OOO to May 9) wrote:
> On 2017/05/04 05:38:01, kkhorimoto_ wrote:
> > Do we currently have any mechanism besides DCHECKs in CommandDispatcher to
> help
> > avoid naming collisions for commands?  I'm thinking it might be useful to
make
> > more verbose naming here that mention that it's specific to context menu
> > actions.
> 
> My thinking was that commands would be grouped by function, not source. There
> should only need to be one 'openURLInNewTab' command; there may be several
> places where that command is called from.
> 
> The tools menu, which by definition throws together a bunch of unrelated
> functions, seems like its dispatcher will have to conform to a bucketload of
> protocols.

Makes sense.  There's still the issue of commands that need to be chained (i.e.
a LoadURL command coupled with a call to WebContextMenuCoordinator's |-stop|). 
My current implementation uses blocks to chain these commands together, as it
seems better than creating ContextMenu-specific versions of these commands that
call through to other dispatcher targets.

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.h (right):

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.h:21: // title
and context menu items will be populated from |params|.  |isIncognito|
On 2017/05/04 09:41:28, marq (OOO to May 9) wrote:
> Let's hold off on incognito support for now.  The pattern we're considering is
> to have incognito subclasses instead of having incognito state that needs to
be
> checked everywhere.

Done.

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.mm (right):

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.mm:20:
web::ContextMenuParams _params;
On 2017/05/04 09:41:28, marq (OOO to May 9) wrote:
> Why doesn't property autosynthesis take care of this?

I'm doing something similar for the new ContextMenuDataSource class.  The
backing ivar is used to copy the const reference into the data source object,
then the property accessor is used to access the copied version via a const
reference.

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.mm:111: // C++
objects or C++ object references, so the URL is passed by pointer.
On 2017/05/04 09:41:28, marq (OOO to May 9) wrote:
> On 2017/05/04 05:38:01, kkhorimoto_ wrote:
> > I know that you want to use NSInvocations here to make this more in line
with
> > our current runtime selector hackery, but it's a bit cumbersome.  In
> particular,
> > it forces us to make stylistic compromises.  The style guide says that we
> should
> > be passing C++ arguments using const references, but the limitations of
> > NSInvocation's |-setArgument:atIndex:| disallow us from doing so. 
Currently,
> I
> > have a ContextMenuCommands interface that takes const pointers, but the
style
> > guide says we should only be using C++ pointer arguments for out-parameters.
> 
> There are a couple of alternatives.
> 
> (1) Don't use invocations. But we also don't want to be explicitly passing
GURL
> objects to/from the view controller layer.
> 
> (2) Use invocations, but don't pass URLs at all. The coordinator/mediator
should
> always exist in the context of a single context menu invocation, so the URL
> shouldn't need to go to the consumer at all; the semantics of the commands
> called from the menu are then (e.g.) "open the current URL in a new tab". 
> 
> (3) Define a context object that the consumer knows about, and define a
subclass
> of it which has a GURL property. The command endpoint downcasts the object and
> extracts the URL.

I went with approach (1).  Instead of NSInvocations, I've used ProceduralBlocks
that use the dispatcher to delegate URL loading to the TabCommands target while
also continuing to call WebContextMenuCoordinator's |-stop|.

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/context_menu/web_context_menu_coordinator.mm
(right):

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/context_menu/web_context_menu_coordinator.mm:69:
#pragma mark - ContextMenuCommands
On 2017/05/04 09:41:28, marq (OOO to May 9) wrote:
> Almost all of these aren't the job of the context menu coordinator, I think.
> Generally speaking coordinators create objects and connect them together, so
> they should only handle commands that create new coordinators or cause
existing
> ones to stop.
> 
> Opening URLs in the current web state seems like a job for the
> web_contents_mediator; opening new tabs seems like a job for the (currently
> nonexistent) tab mediator or maybe the tab grid mediator.

I've revamped my approach. Now there's a datasource object that handles
forwarding calls to |-stop| for the context menu coordinator while using the
dispatcher to send the non-context-menu commands to the appropriate target.

https://codereview.chromium.org/2862783002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/context_menu/web_context_menu_coordinator.mm:142:
std::move(newWebState), opener);
On 2017/05/04 09:41:28, marq (OOO to May 9) wrote:
> On 2017/05/04 05:38:01, kkhorimoto_ wrote:
> > How do we intend to handle showing links that are opened in new tabs?  Will
> this
> > need to wait until lpromero irons out the animation mechanisms?
> 
> The new tab animation needs to be designed; it's similar to other animations
> that require showing other tabs and web states (such as swipe to change tabs,
or
> swipe navigation). But that should be the responsibility of the coordinator /
> view controller that's handling the new tab creation, which won't be this one.

Right.  In that case, I think my ultimate approach here would be to dispatch a
command to the TabCoordinator (or whomever is responsible for creating new
Tabs), then will use a WebStateListObserver to initiate the load for the newly
created WebState.

[kkhorimoto, 2017-05-24 08:48:46]

edchin: PTAL!  I've updated this according to the advice Mark had in my
first-round review and also in our email thread.  This new approach updates
ContextMenuCommands to take an opaque context object that can be downcasted in
the coordinator layer.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm (right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:125: #pragma mark -
SettingsCommands
These were just moved to match the order in which the protocols are declared
above.

[edchin, 2017-05-24 22:35:37]

I will review first thing in the morning. Sorry for the delay.

[kkhorimoto, 2017-05-24 22:39:58]

On 2017/05/24 22:35:37, edchin wrote:
> I will review first thing in the morning. Sorry for the delay.

No worries!

[edchin, 2017-05-25 21:41:53]

Sorry for the late minute comments. They seem like a lot, but they are mostly
trivial. And I agree with the larger picture.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/commands/context_menu_commands.h (right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/commands/context_menu_commands.h:13: // Executes
|menuContext|'s script.
nit: s/menuContext/context. In all comments below.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/context_menu_context.h (right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_context.h:10: // An opaque
object sent along with ContextMenuCommands that can be used to
Could you clarify what opaque means in this context (no pun intended)?

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/context_menu_context_impl.h
(right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_context_impl.h:19:
@interface ContextMenuContextImpl : ContextMenuContext
The pseudo abstract base class and "Impl" seem more Java idiomatic to me. 
I'll followup in person to learn more about what this achieves.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_context_impl.h:27:
@property(nonatomic, readonly) NSString* menuTitle;
Are memory attributes not necessary here?

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/context_menu_item.mm (right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_item.mm:25: item->_title =
[title copy];
Could you incorporate the dot notation over pointer? It's more idiomatic to
ObjC.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.h (right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.h:15: @interface
ContextMenuMediator : NSObject
Note: this is the only mediator that I know of that isn't an observer
of some asynchronous events. This mediator simply processes 
some input and calls methods on the consumer in the initializer. 
So it's a bit different. But I'm not making an argument to change
anything here.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.mm (right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.mm:20:
@property(nonatomic, readonly, weak) id<ContextMenuConsumer> consumer;
The consumer is only used in the -init method so this property is unnecessary.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.h
(right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.h:18: //
Designated initializer that uses |dispatcher| to communicate commands.
Should this be labeled with NS_DESIGNATED_INITIALIZER?
And should the default initializer be restricted?

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.mm
(right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.mm:35:
@property(nonatomic, weak) ContextMenuContext* context;
Should this be strong?

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.mm:63:
self.context = context;
DCHECK(context)

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/web_context_menu_coordinator.mm
(right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/web_context_menu_coordinator.mm:49:
self.mediator =
Since the mediator does its job in its initializer, 
there's no need to keep it around as a property. 
Can we remove the property?

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tab_grid/BUILD.gn (right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tab_grid/BUILD.gn:22:
"//ios/clean/chrome/browser/ui/context_menu:context_menu",
nit: remove ":context_menu".

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/BUILD.gn (right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/BUILD.gn:19:
"//ios/clean/chrome/browser/ui/commands:commands",
nit: remove default target here and below.

[kkhorimoto, 2017-05-26 23:20:10]

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/commands/context_menu_commands.h (right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/commands/context_menu_commands.h:13: // Executes
|menuContext|'s script.
On 2017/05/25 21:41:53, edchin wrote:
> nit: s/menuContext/context. In all comments below.

Done.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/context_menu_context.h (right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_context.h:10: // An opaque
object sent along with ContextMenuCommands that can be used to
On 2017/05/25 21:41:53, edchin wrote:
> Could you clarify what opaque means in this context (no pun intended)?

Done.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/context_menu_context_impl.h
(right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_context_impl.h:19:
@interface ContextMenuContextImpl : ContextMenuContext
On 2017/05/25 21:41:53, edchin wrote:
> The pseudo abstract base class and "Impl" seem more Java idiomatic to me. 
> I'll followup in person to learn more about what this achieves. 

Acknowledged.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_context_impl.h:27:
@property(nonatomic, readonly) NSString* menuTitle;
On 2017/05/25 21:41:53, edchin wrote:
> Are memory attributes not necessary here?

The memory is manually managed (just passing through to the title in |params|,
which is owned by a scoped NSObject).  The annotations don't actually make a
difference if you're implementing them yourself, but I'll add strong regardless,
since this object does retain a strong reference through the params.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/context_menu_item.mm (right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_item.mm:25: item->_title =
[title copy];
On 2017/05/25 21:41:53, edchin wrote:
> Could you incorporate the dot notation over pointer? It's more idiomatic to
> ObjC. 

That would require redefining the property as readwrite in the implementation
file.  Rather than doing that, I created a private initializer that sets the
ivars backing the readonly properties.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.h (right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.h:15: @interface
ContextMenuMediator : NSObject
On 2017/05/25 21:41:53, edchin wrote:
> Note: this is the only mediator that I know of that isn't an observer
> of some asynchronous events. This mediator simply processes 
> some input and calls methods on the consumer in the initializer. 
> So it's a bit different. But I'm not making an argument to change
> anything here.

The mediator's main purpose is to act as the middle man between the coordinator
layer and the view layer, so populating the UI it's presenting is accomplishing
that.  I think it's a little different because everything that's needed for this
UI is already available when creating the coordinator and it never changes (so
there's no need for observers or other updates).

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.mm (right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_mediator.mm:20:
@property(nonatomic, readonly, weak) id<ContextMenuConsumer> consumer;
On 2017/05/25 21:41:53, edchin wrote:
> The consumer is only used in the -init method so this property is unnecessary.

Done.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.h
(right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.h:18: //
Designated initializer that uses |dispatcher| to communicate commands.
On 2017/05/25 21:41:53, edchin wrote:
> Should this be labeled with NS_DESIGNATED_INITIALIZER?
> And should the default initializer be restricted?

The only way to set the UIAlertController's |preferredStyle| property is via the
factory method that we call in our initializer.  If we were to make this the
designated initializer, we would need to call a superclass's designated
initializer instead of that factory method, so we wouldn't be able to set the
preferred style.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.mm
(right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.mm:35:
@property(nonatomic, weak) ContextMenuContext* context;
On 2017/05/25 21:41:53, edchin wrote:
> Should this be strong?

The context object is owned by the coordinator, so it doesn't have to be. 
However, from a layering perspective, it doesn't seem right for the consumer to
rely on memory management of the coordinator layer, so I'll convert this to
strong.  Both classes are torn down at the same time, so it won't actually
affect the lifetime of this object.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.mm:63:
self.context = context;
On 2017/05/25 21:41:53, edchin wrote:
> DCHECK(context)

Done.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/web_context_menu_coordinator.mm
(right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/web_context_menu_coordinator.mm:49:
self.mediator =
On 2017/05/25 21:41:53, edchin wrote:
> Since the mediator does its job in its initializer, 
> there's no need to keep it around as a property. 
> Can we remove the property?

Doing that gave me an unused variable warning when I tried to compile.  Since
there's no real reason to instantiate an object here, I've converted the
mediator's initializer to a class function that performs the work of setting the
items.  For consumers like the context menu that only require a 1-time
configuration, do we still want to try to use the mediator pattern like I'm
doing here, or should we just fold setup into a utility function or something?

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tab_grid/BUILD.gn (right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tab_grid/BUILD.gn:22:
"//ios/clean/chrome/browser/ui/context_menu:context_menu",
On 2017/05/25 21:41:53, edchin wrote:
> nit: remove ":context_menu". 

Done.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/BUILD.gn (right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/BUILD.gn:19:
"//ios/clean/chrome/browser/ui/commands:commands",
On 2017/05/25 21:41:53, edchin wrote:
> nit: remove default target here and below.

Done.

[kkhorimoto, 2017-05-26 23:25:48]

cleanup

[edchin, 2017-05-27 15:59:03]

lgtm with minor comments/nits.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/context_menu_context_impl.h
(right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_context_impl.h:27:
@property(nonatomic, readonly) NSString* menuTitle;
On 2017/05/26 23:20:09, kkhorimoto_ wrote:
> On 2017/05/25 21:41:53, edchin wrote:
> > Are memory attributes not necessary here?
> 
> The memory is manually managed (just passing through to the title in |params|,
> which is owned by a scoped NSObject).  The annotations don't actually make a
> difference if you're implementing them yourself, but I'll add strong
regardless,
> since this object does retain a strong reference through the params.

You're right, it's readonly and you override the accessor.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/context_menu_item.mm (right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/context_menu_item.mm:25: item->_title =
[title copy];
On 2017/05/26 23:20:09, kkhorimoto_ wrote:
> On 2017/05/25 21:41:53, edchin wrote:
> > Could you incorporate the dot notation over pointer? It's more idiomatic to
> > ObjC. 
> 
> That would require redefining the property as readwrite in the implementation
> file.  Rather than doing that, I created a private initializer that sets the
> ivars backing the readonly properties.

Yup. This looks more ObjC-y.

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/web_context_menu_coordinator.mm
(right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/web_context_menu_coordinator.mm:49:
self.mediator =
On 2017/05/26 23:20:09, kkhorimoto_ wrote:
> On 2017/05/25 21:41:53, edchin wrote:
> > Since the mediator does its job in its initializer, 
> > there's no need to keep it around as a property. 
> > Can we remove the property?
> 
> Doing that gave me an unused variable warning when I tried to compile.  Since
> there's no real reason to instantiate an object here, I've converted the
> mediator's initializer to a class function that performs the work of setting
the
> items.  For consumers like the context menu that only require a 1-time
> configuration, do we still want to try to use the mediator pattern like I'm
> doing here, or should we just fold setup into a utility function or something?

Interesting question. This is the only mediator that I know of that doesn't have
some asynchronous observer. Given that it's unique, 
I suggest we leave this as you have it, and see if this pattern emerges more
commonly.

https://codereview.chromium.org/2862783002/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/context_menu/BUILD.gn (right):

https://codereview.chromium.org/2862783002/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/context_menu/BUILD.gn:19:
"//ios/chrome/browser/web_state_list:web_state_list",
nit: redundant targets here and below.

https://codereview.chromium.org/2862783002/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/context_menu/BUILD.gn:42: "//base:base",
nit: here too.

https://codereview.chromium.org/2862783002/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm (right):

https://codereview.chromium.org/2862783002/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm:103:
[self.browser->dispatcher()
This would have a more symmetrical design if the -startDispatching calls were
moved to -childCoordinatorDidStart. This is since the -stopDispatching calls are
in -childCoordinatorWillStop. You probably put this here for good reason. Can
you explain why?

[edchin, 2017-05-27 16:00:41]

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

[commit-bot: I haz the power, 2017-05-27 16:00:46]

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-05-27 16:09:23]

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

[commit-bot: I haz the power, 2017-05-27 16:09:24]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[kkhorimoto, 2017-05-27 23:09:04]

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/context_menu/web_context_menu_coordinator.mm
(right):

https://codereview.chromium.org/2862783002/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/context_menu/web_context_menu_coordinator.mm:49:
self.mediator =
On 2017/05/27 15:59:03, edchin wrote:
> On 2017/05/26 23:20:09, kkhorimoto_ wrote:
> > On 2017/05/25 21:41:53, edchin wrote:
> > > Since the mediator does its job in its initializer, 
> > > there's no need to keep it around as a property. 
> > > Can we remove the property?
> > 
> > Doing that gave me an unused variable warning when I tried to compile. 
Since
> > there's no real reason to instantiate an object here, I've converted the
> > mediator's initializer to a class function that performs the work of setting
> the
> > items.  For consumers like the context menu that only require a 1-time
> > configuration, do we still want to try to use the mediator pattern like I'm
> > doing here, or should we just fold setup into a utility function or
something?
> 
> Interesting question. This is the only mediator that I know of that doesn't
have
> some asynchronous observer. Given that it's unique, 
> I suggest we leave this as you have it, and see if this pattern emerges more
> commonly. 

I'm also working on the javascript dialog coordinator, and they also require
only a one-time setup.  This will be true for any UI that doesn't update after
being presented.  For now, I've followed the same pattern as here, using a class
method to setup the consumer.

https://codereview.chromium.org/2862783002/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/context_menu/BUILD.gn (right):

https://codereview.chromium.org/2862783002/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/context_menu/BUILD.gn:19:
"//ios/chrome/browser/web_state_list:web_state_list",
On 2017/05/27 15:59:03, edchin wrote:
> nit: redundant targets here and below.

Done.

https://codereview.chromium.org/2862783002/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/context_menu/BUILD.gn:42: "//base:base",
On 2017/05/27 15:59:03, edchin wrote:
> nit: here too.

It's needed for DCHECKs.

https://codereview.chromium.org/2862783002/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm (right):

https://codereview.chromium.org/2862783002/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm:103:
[self.browser->dispatcher()
On 2017/05/27 15:59:03, edchin wrote:
> This would have a more symmetrical design if the -startDispatching calls were
> moved to -childCoordinatorDidStart. This is since the -stopDispatching calls
are
> in -childCoordinatorWillStop. You probably put this here for good reason. Can
> you explain why?

I didn't really have a good reason, so I followed your suggestion.

[kkhorimoto, 2017-05-27 23:09:11]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-05-27 23:09:12]

The patchset sent to the CQ was uploaded after l-g-t-m from edchin@chromium.org
Link to the patchset: https://codereview.chromium.org/2862783002/#ps140001
(title: "Ed's nits, test fixes")

[commit-bot: I haz the power, 2017-05-27 23:09:23]

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-05-27 23:16:52]

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

[commit-bot: I haz the power, 2017-05-27 23:16:53]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[kkhorimoto, 2017-05-28 05:29:33]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-05-28 05:29:34]

The patchset sent to the CQ was uploaded after l-g-t-m from edchin@chromium.org
Link to the patchset: https://codereview.chromium.org/2862783002/#ps160001
(title: "fix deps")

[commit-bot: I haz the power, 2017-05-28 05:29:44]

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-05-28 06:55:01]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1495949373730080,
"parent_rev": "bef80d93b4a5b160e66ceb6986fb31faa61fc892", "commit_rev":
"ffa157bf17edd17189c334a3bbdd5ec40d50d07c"}

[commit-bot: I haz the power, 2017-05-28 06:55:18]

Description was changed from

==========
[iOS Clean] Wired up ContextMenuCommands.

This CL updates WebContextMenuCoordinator and its associated mediator
class to support more actions.  It will load URLs and create new
WebStates, but still does not perform corresponding animations to show
newly created Tabs and their loaded content.

BUG=none
==========

to

==========
[iOS Clean] Wired up ContextMenuCommands.

This CL updates WebContextMenuCoordinator and its associated mediator
class to support more actions.  It will load URLs and create new
WebStates, but still does not perform corresponding animations to show
newly created Tabs and their loaded content.

BUG=none

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

[commit-bot: I haz the power, 2017-05-28 06:55:19]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/ffa157bf17edd17189c334a3bbdd...

[marq (ping after 24h), 2017-05-31 12:26:09]

Overall LGTM!

https://codereview.chromium.org/2862783002/diff/160001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.mm
(right):

https://codereview.chromium.org/2862783002/diff/160001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.mm:26:
command_imp(dispatcher, command, context);
Why not -performSelector:withObject: ?

(my concern is that directly grabbing the IMP here short-circuits any other
dynamic method stuff, which is probably not something a view controller should
be doing).

[kkhorimoto, 2017-05-31 18:00:20]

https://codereview.chromium.org/2862783002/diff/160001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.mm
(right):

https://codereview.chromium.org/2862783002/diff/160001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.mm:26:
command_imp(dispatcher, command, context);
On 2017/05/31 12:26:09, marq (ping after 24h) wrote:
> Why not -performSelector:withObject: ?
> 
> (my concern is that directly grabbing the IMP here short-circuits any other
> dynamic method stuff, which is probably not something a view controller should
> be doing).

|-performSelector:withObject| doesn't compile under ARC because the compiler has
no idea what the intended memory management is.  I've verified, and this is
correctly dispatched to the intended forwarding target, although I'll agree that
it seems a little ugly.

[lpromero, 2017-06-01 09:38:23]

On 2017/05/31 18:00:20, kkhorimoto_ wrote:
>
https://codereview.chromium.org/2862783002/diff/160001/ios/clean/chrome/brows...
> File ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.mm
> (right):
> 
>
https://codereview.chromium.org/2862783002/diff/160001/ios/clean/chrome/brows...
> ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.mm:26:
> command_imp(dispatcher, command, context);
> On 2017/05/31 12:26:09, marq (ping after 24h) wrote:
> > Why not -performSelector:withObject: ?
> > 
> > (my concern is that directly grabbing the IMP here short-circuits any other
> > dynamic method stuff, which is probably not something a view controller
should
> > be doing).
> 
> |-performSelector:withObject| doesn't compile under ARC because the compiler
has
> no idea what the intended memory management is.  I've verified, and this is
> correctly dispatched to the intended forwarding target, although I'll agree
that
> it seems a little ugly.

Note that we could silence the warning:
https://cs.chromium.org/search/?q=Warc-performSelector-leaks&sq=package:chrom...
The issue is that ARC doesn't know how to handle the memory management of the
returned value, so it warns.

[marq (ping after 24h), 2017-06-01 09:43:13]

On 2017/06/01 09:38:23, lpromero wrote:
> On 2017/05/31 18:00:20, kkhorimoto_ wrote:
> >
>
https://codereview.chromium.org/2862783002/diff/160001/ios/clean/chrome/brows...
> > File
ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.mm
> > (right):
> > 
> >
>
https://codereview.chromium.org/2862783002/diff/160001/ios/clean/chrome/brows...
> > ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.mm:26:
> > command_imp(dispatcher, command, context);
> > On 2017/05/31 12:26:09, marq (ping after 24h) wrote:
> > > Why not -performSelector:withObject: ?
> > > 
> > > (my concern is that directly grabbing the IMP here short-circuits any
other
> > > dynamic method stuff, which is probably not something a view controller
> should
> > > be doing).
> > 
> > |-performSelector:withObject| doesn't compile under ARC because the compiler
> has
> > no idea what the intended memory management is.  I've verified, and this is
> > correctly dispatched to the intended forwarding target, although I'll agree
> that
> > it seems a little ugly.
> 
> Note that we could silence the warning:
>
https://cs.chromium.org/search/?q=Warc-performSelector-leaks&sq=package:chrom...
> The issue is that ARC doesn't know how to handle the memory management of the
> returned value, so it warns.

In cases (like this) where there's no return value, that seems like a bug (in
ARC or clang). 

I'd prefer to silence the warning rather than jumping into runtime-land.

[marq (ping after 24h), 2017-06-01 09:48:47]

On 2017/06/01 09:43:13, marq (ping after 24h) wrote:
> On 2017/06/01 09:38:23, lpromero wrote:
> > On 2017/05/31 18:00:20, kkhorimoto_ wrote:
> > >
> >
>
https://codereview.chromium.org/2862783002/diff/160001/ios/clean/chrome/brows...
> > > File
> ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.mm
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2862783002/diff/160001/ios/clean/chrome/brows...
> > >
ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.mm:26:
> > > command_imp(dispatcher, command, context);
> > > On 2017/05/31 12:26:09, marq (ping after 24h) wrote:
> > > > Why not -performSelector:withObject: ?
> > > > 
> > > > (my concern is that directly grabbing the IMP here short-circuits any
> other
> > > > dynamic method stuff, which is probably not something a view controller
> > should
> > > > be doing).
> > > 
> > > |-performSelector:withObject| doesn't compile under ARC because the
compiler
> > has
> > > no idea what the intended memory management is.  I've verified, and this
is
> > > correctly dispatched to the intended forwarding target, although I'll
agree
> > that
> > > it seems a little ugly.
> > 
> > Note that we could silence the warning:
> >
>
https://cs.chromium.org/search/?q=Warc-performSelector-leaks&sq=package:chrom...
> > The issue is that ARC doesn't know how to handle the memory management of
the
> > returned value, so it warns.
> 
> In cases (like this) where there's no return value, that seems like a bug (in
> ARC or clang). 
> 
> I'd prefer to silence the warning rather than jumping into runtime-land.

Also, now that I think about it, maybe supporting an arbitrary vector of
commands is overkill. What's the problem with the view controller sending
-hideContextMenu: after any configured selector? My worry is that this has crept
into speculative generality at the expense of comprehensibility.

[kkhorimoto, 2017-06-01 16:52:18]

On 2017/06/01 09:48:47, marq (ping after 24h) wrote:
> On 2017/06/01 09:43:13, marq (ping after 24h) wrote:
> > On 2017/06/01 09:38:23, lpromero wrote:
> > > On 2017/05/31 18:00:20, kkhorimoto_ wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2862783002/diff/160001/ios/clean/chrome/brows...
> > > > File
> > ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.mm
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2862783002/diff/160001/ios/clean/chrome/brows...
> > > >
> ios/clean/chrome/browser/ui/context_menu/context_menu_view_controller.mm:26:
> > > > command_imp(dispatcher, command, context);
> > > > On 2017/05/31 12:26:09, marq (ping after 24h) wrote:
> > > > > Why not -performSelector:withObject: ?
> > > > > 
> > > > > (my concern is that directly grabbing the IMP here short-circuits any
> > other
> > > > > dynamic method stuff, which is probably not something a view
controller
> > > should
> > > > > be doing).
> > > > 
> > > > |-performSelector:withObject| doesn't compile under ARC because the
> compiler
> > > has
> > > > no idea what the intended memory management is.  I've verified, and this
> is
> > > > correctly dispatched to the intended forwarding target, although I'll
> agree
> > > that
> > > > it seems a little ugly.
> > > 
> > > Note that we could silence the warning:
> > >
> >
>
https://cs.chromium.org/search/?q=Warc-performSelector-leaks&sq=package:chrom...
> > > The issue is that ARC doesn't know how to handle the memory management of
> the
> > > returned value, so it warns.
> > 
> > In cases (like this) where there's no return value, that seems like a bug
(in
> > ARC or clang). 
> > 
> > I'd prefer to silence the warning rather than jumping into runtime-land.
> 
> Also, now that I think about it, maybe supporting an arbitrary vector of
> commands is overkill. What's the problem with the view controller sending
> -hideContextMenu: after any configured selector? My worry is that this has
crept
> into speculative generality at the expense of comprehensibility.

I'm fine with just silencing the warning and using |-performSelector:| since
it's definitely nicer than using the IMP.  The reason I used the vector of
commands was because |-hideContextMenu:| only sometimes needs to be sent.  For
commands that open a new Tab, the context menu coordinator (and its web
coordinator) have already been stopped.