[ios] Adds support for Find in Page to the new architecture.

[ios] Adds support for Find in Page to the new architecture.

BUG=

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

[rohitrao (ping after 24h), 2017-03-14 01:10:35]

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

[rohitrao (ping after 24h), 2017-03-14 01:10:37]

Here's the first pass at adding FIP to the clean skeleton.  If you'd like to
patch this in and try it out, please also patch
https://codereview.chromium.org/2741883002, because my CL relies on the
WebStateList being up to date.

I've added comments highlighting some of the key areas that need design
discussion.  Please let me know if you agree or disagree with the choices I
made.

For the time being, I'm only looking for high-level design comments.  I didn't
put too much effort into cleaning up the code.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/browser_coordinator.mm (right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/browser_coordinator.mm:71: [coordinator
coordinatorDidMoveToParent:self];
1) I found a need for a "didMoveToParent" method.  The FIP coordinator needs a
Browser (WebStateList and CommandDispatcher) in order to really initialize
itself.  It doesn't have one yet in |-init|, and |-start| is too late given that
coordinators may never be started under our new design.

I actually think that most coordinators will do nothing in init, but that seems
ok.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/model/browser.h (right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/model/browser.h:27: CommandDispatcher* dispatcher() {
return dispatcher_; }
2) Added the dispatcher to Browser, as discussed.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm
(right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:47:
CommandDispatcher* dispatcher = self.browser->dispatcher();
3) Here's an example of command registration using the dispatcher.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:58:
self.viewController.dispatcher = static_cast<id>(self.browser->dispatcher());
4) Here's an example of casting the dispatcher to |id| and then passing it to
the view controller.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:65:
mediator_->set_consumer(nil);
5) The consumer (UIVC) is destroyed when this coordinator is stopped, so we need
to push that through to the mediator.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:93:
mediator_->OnFindInPageResultsAvailable(model);
6) This is an unfortunate side effect of the FIP API being block-based.  When I
switch it to callbacks or observers, this code will move into the mediator.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:100: [self
start];
7) This is an example of a command that can be called when the UI is closed. 
There are two ways to handle this, and I actually think I picked the wrong one.

a) Have the controller layer open the UI and then kick off a model query, which
is what I do here.

b) Have the controller layer kick off a model query, then have the mediator
trigger UI creation when results come back.

I actually think (b) is already hooked up, through the createConsumer call, so I
should be able to remove the call to [self start] from this method.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.h (right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.h:20: -
(void)createConsumer;
8) In general, feature code can trigger model queries even when the
corresponding UI is hidden.  In the specific case of this mediator, we may get
FIP results when the UI is closed and no consumer exists.  I've addressed this
by introducing a delegate interface that allows the mediator to request a
non-nil consumer (which will generally mean starting the coordinator and showing
the UI).

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.h
(right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.h:15:
@property(nonatomic, readwrite, weak) id<FindInPageCommands> dispatcher;
9) VCs get dispatchers as id<FooCommands>, as discussed.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm
(right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm:44:
forControlEvents:UIControlEventTouchUpInside];
10) Example of a VC setting an action on a control and setting the target to
|self|, rather than using the responder chain.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm:81:
[self.dispatcher findNextInPage];
11) Example of a VC catching an action and forwarding it to the dispatcher.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tab/tab_coordinator.mm (right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tab/tab_coordinator.mm:121:
self.viewController.findBarViewController = nil;
12) The FIP coordinator can be stopped and its UI destroyed, unlike the others.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tools/menu_view_controller.mm (right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tools/menu_view_controller.mm:160: [self.dispatcher
showFindInPage];
13) Starting to convert the tools menu over to the dispatcher.

[rohitrao (ping after 24h), 2017-03-14 13:29:21]

rohitrao@chromium.org changed reviewers:
+ stkhapugin@chromium.org

[stkhapugin, 2017-03-14 14:51:06]

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm
(right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:88:
NSLog(@"findStringInPage command handler: %@", searchTerm);
Can you add metrics logging: UserActions.Counts.Find* here and in next/prev? We
don't log these currently, but other platforms do.

[lpromero, 2017-03-14 17:17:57]

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/browser_coordinator+internal.h (right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/browser_coordinator+internal.h:59: // override this
method to run code that requires a CoordinatorContext.
requires a "configured" CoordinatorContext?

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

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/commands/find_in_page_commands.h:21: // Hide the
find in page UI.
Hides

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

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/BUILD.gn:30:
source_set("find_in_page_ui") {
Can you follow up and integrate this in Showcase?

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_consumer.h (right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_consumer.h:10: -
(void)setCurrentMatch:(NSUInteger)current ofTotalMatches:(NSUInteger)total;
Documentation for the protocol and method.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm
(right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:25:
@property(nonatomic, readwrite, assign) BOOL started;
Should this be a property of all coordinators? In the coordination context?

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:33:
std::unique_ptr<FindInPageMediator> mediator_;
_mediator

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:58:
self.viewController.dispatcher = static_cast<id>(self.browser->dispatcher());
On 2017/03/14 01:10:36, rohitrao wrote:
> 4) Here's an example of casting the dispatcher to |id| and then passing it to
> the view controller.

And it's not possible to cast to id<FooCommands>, IIUC? The compiler doesn't
care we could put an id that doesn't implement the protocol?

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:80:
NSLog(@"showFindInPage command handler");
Remove

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:98:
NSLog(@"findNextInPage command handler");
Remove

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:100: [self
start];
On 2017/03/14 01:10:37, rohitrao wrote:
> 7) This is an example of a command that can be called when the UI is closed. 
> There are two ways to handle this, and I actually think I picked the wrong
one.
> 
> a) Have the controller layer open the UI and then kick off a model query,
which
> is what I do here.
> 
> b) Have the controller layer kick off a model query, then have the mediator
> trigger UI creation when results come back.
> 
> I actually think (b) is already hooked up, through the createConsumer call, so
I
> should be able to remove the call to [self start] from this method.

* When you say controller layer, it's the coordinators, right?
* Regarding (b), you mean because the mediator calls createConsumer which calls
start? Seems hooked up.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:110:
NSLog(@"findPreviousInPage command handler");
Remove.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.h (right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.h:36: //
web::WebStateListObserver implementation.
WebStateListObserver doesn't seem to be in the web namespace. Why this comment
at all?

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm (right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm:22:
NSLog(@"%lu of %lu", model.currentIndex, model.matches);
Remove.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.h
(right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.h:10:
#import "ios/clean/chrome/browser/ui/commands/find_in_page_commands.h"
This can be forward-declared.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.h:13:
@interface FindInPageViewController : UIViewController<FindInPageConsumer>
Documentation.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.h:15:
@property(nonatomic, readwrite, weak) id<FindInPageCommands> dispatcher;
On 2017/03/14 01:10:37, rohitrao wrote:
> 9) VCs get dispatchers as id<FooCommands>, as discussed.

No need for readwrite.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm
(right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm:12: 
Missing ARC header.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm:14: //
For the first |kSearchDelayChars| characters, delay by |kSearchLongDelay|
Missing period.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm:24:
@property(nonatomic, readwrite, strong) FindBarView* view;
Do you need the readwrite?

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm:51:
self.view = findBarView;
This should be done in -loadView.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm:66:
[static_cast<FindBarView*>(self.view) updateResultsLabelWithText:text];
Shouldn't we use base::mac::ObjCCast(Strict) instead?

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm:96:
return [self textChanged];
This would be more readable if on two lines:
if (length == 0) {
  [self textChanged];
  return;
}

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tab/tab_container_view_controller.mm (right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tab/tab_container_view_controller.mm:74:
self.findBarView.backgroundColor = [UIColor greenColor];
Should it be removed now?

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/tools/menu_view_controller.h (right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tools/menu_view_controller.h:14: #import
"ios/clean/chrome/browser/ui/commands/find_in_page_commands.h"
Can be forward declared.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tools/menu_view_controller.h:21:
@property(nonatomic, readwrite, weak) id<FindInPageCommands> dispatcher;
No need for readwrite.

[marq (ping after 24h), 2017-03-16 13:36:37]

Description was changed from

==========
[ios] Adds support for Find in Page to the new architecture.

BUG=
==========

to

==========
[ios] Adds support for Find in Page to the new architecture.

BUG=
==========

[marq (ping after 24h), 2017-03-16 13:36:37]

marq@chromium.org changed reviewers:
+ gchatz@chromium.org

[marq (ping after 24h), 2017-03-16 13:36:43]

On 2017/03/14 14:51:06, stkhapugin wrote:
>
https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
> File ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm
> (right):
> 
>
https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
> ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:88:
> NSLog(@"findStringInPage command handler: %@", searchTerm);
> Can you add metrics logging: UserActions.Counts.Find* here and in next/prev?
We
> don't log these currently, but other platforms do.

Added gchatz@, who's working on integrating metrics into the command dispatcher
for use cases exactly like this.

(the metrics won't happen in this CL, however).

[marq (ping after 24h), 2017-03-16 13:37:05]

High-level comments only.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm
(right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:25:
@property(nonatomic, readwrite, assign) BOOL started;
On 2017/03/14 17:17:56, lpromero wrote:
> Should this be a property of all coordinators? In the coordination context?

I don't think it should be in the context (which shouldn't contain mutable
state).

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.h (right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.h:20: -
(void)createConsumer;
On 2017/03/14 01:10:37, rohitrao wrote:
> 8) In general, feature code can trigger model queries even when the
> corresponding UI is hidden.  In the specific case of this mediator, we may get
> FIP results when the UI is closed and no consumer exists.  I've addressed this
> by introducing a delegate interface that allows the mediator to request a
> non-nil consumer (which will generally mean starting the coordinator and
showing
> the UI).

I'm not 100% happy with this pattern, but I'm not sure why (yet).

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.h:23: class
FindInPageMediator : public WebStateListObserver {
Other mediators are (so far) ObjC objects, so we should either change this, or
change all of the others.

The advantage of keeping to ObjC is that a common way to factor complexity out
of a coordinator will be to extract some logic into a mediator.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.h:39:
WebStateList* web_state_list_;
Why does this need the list rather than a specific webstate?

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm
(right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm:44:
forControlEvents:UIControlEventTouchUpInside];
On 2017/03/14 01:10:37, rohitrao wrote:
> 10) Example of a VC setting an action on a control and setting the target to
> |self|, rather than using the responder chain.

This means that the action protocol is either not needed, or can just live in
the feature's ui/ subdirectory -- that is, ui/actions/ can go away.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm:75: -
(void)showFindInPage {
This seems like it means -showFindInPage either should be optional or not part
of the FindInPageActions protocol.

[rohitrao (ping after 24h), 2017-03-16 14:00:18]

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm
(right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:25:
@property(nonatomic, readwrite, assign) BOOL started;
On 2017/03/16 13:37:05, marq (Slow - catching up) wrote:
> On 2017/03/14 17:17:56, lpromero wrote:
> > Should this be a property of all coordinators? In the coordination context?
> 
> I don't think it should be in the context (which shouldn't contain mutable
> state).

I was picturing this as a property on BrowserCoordinator.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:58:
self.viewController.dispatcher = static_cast<id>(self.browser->dispatcher());
On 2017/03/14 17:17:56, lpromero wrote:
> On 2017/03/14 01:10:36, rohitrao wrote:
> > 4) Here's an example of casting the dispatcher to |id| and then passing it
to
> > the view controller.
> 
> And it's not possible to cast to id<FooCommands>, IIUC? The compiler doesn't
> care we could put an id that doesn't implement the protocol?

I'm not sure.  I'll try it and see.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:100: [self
start];
On 2017/03/14 17:17:56, lpromero wrote:
> On 2017/03/14 01:10:37, rohitrao wrote:
> > 7) This is an example of a command that can be called when the UI is closed.

> > There are two ways to handle this, and I actually think I picked the wrong
> one.
> > 
> > a) Have the controller layer open the UI and then kick off a model query,
> which
> > is what I do here.
> > 
> > b) Have the controller layer kick off a model query, then have the mediator
> > trigger UI creation when results come back.
> > 
> > I actually think (b) is already hooked up, through the createConsumer call,
so
> I
> > should be able to remove the call to [self start] from this method.
> 
> * When you say controller layer, it's the coordinators, right?
> * Regarding (b), you mean because the mediator calls createConsumer which
calls
> start? Seems hooked up.

I use the phrase "controller layer" to collectively refer to UIVCs,
coordinators, and mediators.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.h (right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.h:20: -
(void)createConsumer;
On 2017/03/16 13:37:05, marq (Slow - catching up) wrote:
> On 2017/03/14 01:10:37, rohitrao wrote:
> > 8) In general, feature code can trigger model queries even when the
> > corresponding UI is hidden.  In the specific case of this mediator, we may
get
> > FIP results when the UI is closed and no consumer exists.  I've addressed
this
> > by introducing a delegate interface that allows the mediator to request a
> > non-nil consumer (which will generally mean starting the coordinator and
> showing
> > the UI).
> 
> I'm not 100% happy with this pattern, but I'm not sure why (yet).

We'll also have the opposite scenario, where "model" changes can cause the UI to
be hidden and destroyed.  (For example, when you switch from a tab that's
currently finding-in-page to a different tab that is not.)

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.h:23: class
FindInPageMediator : public WebStateListObserver {
On 2017/03/16 13:37:05, marq (Slow - catching up) wrote:
> Other mediators are (so far) ObjC objects, so we should either change this, or
> change all of the others.
> 
> The advantage of keeping to ObjC is that a common way to factor complexity out
> of a coordinator will be to extract some logic into a mediator.

Is it important to be consistent across mediators, or is this a case where we
can pick the language that fits best for a given mediator, case-by-case?

For FIP, I will eventually need a C++ object to listen for callbacks from the
FindTabHelper.  (I also need a WebStateListObserver, but we've already bridged
that to ObjC.)  Given the situation, it seemed simpler to have a single C++
object, but it's not hard to do it the other way.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.h:36: //
web::WebStateListObserver implementation.
On 2017/03/14 17:17:56, lpromero wrote:
> WebStateListObserver doesn't seem to be in the web namespace. Why this comment
> at all?

Cruft that will be useful once TabCoordinator stops being specific to a single
WebState.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.h:39:
WebStateList* web_state_list_;
On 2017/03/16 13:37:05, marq (Slow - catching up) wrote:
> Why does this need the list rather than a specific webstate?

Once TabCoordinator supports multiple WebStates, this mediator will become a
WebStateList observer and update the find bar UI whenever the active WebState
changes.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm
(right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm:44:
forControlEvents:UIControlEventTouchUpInside];
On 2017/03/16 13:37:05, marq (Slow - catching up) wrote:
> On 2017/03/14 01:10:37, rohitrao wrote:
> > 10) Example of a VC setting an action on a control and setting the target to
> > |self|, rather than using the responder chain.
> 
> This means that the action protocol is either not needed, or can just live in
> the feature's ui/ subdirectory -- that is, ui/actions/ can go away.

This is a consequence of actions becoming specific to a single view->VC pair.

A huge downside to the code I have here is that all of the actions need to be
set by the VC.  I believe there are two ways to send actions to targets:
1) Send the action to "nil" and have it propagate up the responder chain until
something handles it.
2) Send the action to a specific object, which throws an exception if that
object does not handle it.

I don't think there's a way for a button to send its action to |self| and have
that propagate up to the VC.  But the button also isn't necessarily the first
responder, so there's no guarantee that sending an action to nil will work,
because the button and VC may not be in the responder chain.

The problem with this is that buttons can't configure their actions in
isolation, because they don't have a direct pointer to their VC.  This would
force us to put all of the action setting code in the VC.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm:75: -
(void)showFindInPage {
On 2017/03/16 13:37:05, marq (Slow - catching up) wrote:
> This seems like it means -showFindInPage either should be optional or not part
> of the FindInPageActions protocol.

Correct.  I had it in the protocol originally, but then I realized that the find
bar ui never calls "show" (because it's already visible).  This probably belongs
only in the tools menu action protocol.

[lpromero, 2017-03-16 14:57:04]

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm
(right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:25:
@property(nonatomic, readwrite, assign) BOOL started;
On 2017/03/16 13:37:05, marq (Slow - catching up) wrote:
> On 2017/03/14 17:17:56, lpromero wrote:
> > Should this be a property of all coordinators? In the coordination context?
> 
> I don't think it should be in the context (which shouldn't contain mutable
> state).

Sidenote: One can change "animated" in the context for example. We could start
animated but stop non-animated.
I am fine with having it at the BrowserCoordinator level, especially since now
the -start and -stop methods are not no-op and are required to be called.

Rohit: I will make a CL.

[lpromero, 2017-03-16 17:12:30]

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm
(right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:25:
@property(nonatomic, readwrite, assign) BOOL started;
On 2017/03/16 14:57:04, lpromero wrote:
> On 2017/03/16 13:37:05, marq (Slow - catching up) wrote:
> > On 2017/03/14 17:17:56, lpromero wrote:
> > > Should this be a property of all coordinators? In the coordination
context?
> > 
> > I don't think it should be in the context (which shouldn't contain mutable
> > state).
> 
> Sidenote: One can change "animated" in the context for example. We could start
> animated but stop non-animated.
> I am fine with having it at the BrowserCoordinator level, especially since now
> the -start and -stop methods are not no-op and are required to be called.
> 
> Rohit: I will make a CL.

https://codereview.chromium.org/2750003004

[marq (ping after 24h), 2017-03-17 14:45:33]

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm
(right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:58:
self.viewController.dispatcher = static_cast<id>(self.browser->dispatcher());
On 2017/03/16 14:00:17, rohitrao wrote:
> On 2017/03/14 17:17:56, lpromero wrote:
> > On 2017/03/14 01:10:36, rohitrao wrote:
> > > 4) Here's an example of casting the dispatcher to |id| and then passing it
> to
> > > the view controller.
> > 
> > And it's not possible to cast to id<FooCommands>, IIUC? The compiler doesn't
> > care we could put an id that doesn't implement the protocol?
> 
> I'm not sure.  I'll try it and see.

Protocols aren't really part of the type system in ObjC, so while you can cast
to id<Foo>, there are no guarantees about whether the cast object actually
conforms to Foo. Any time we cast to a protocol, we should at least DCHECK
-conformsToProtocol:.

My assumption was that the dispatcher would explicitly conform to all of the
command protocols, so we can do simple assignment when hooking it up to clients.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.h (right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.h:23: class
FindInPageMediator : public WebStateListObserver {
On 2017/03/16 14:00:18, rohitrao wrote:
> On 2017/03/16 13:37:05, marq (Slow - catching up) wrote:
> > Other mediators are (so far) ObjC objects, so we should either change this,
or
> > change all of the others.
> > 
> > The advantage of keeping to ObjC is that a common way to factor complexity
out
> > of a coordinator will be to extract some logic into a mediator.
> 
> Is it important to be consistent across mediators, or is this a case where we
> can pick the language that fits best for a given mediator, case-by-case?
> 
> For FIP, I will eventually need a C++ object to listen for callbacks from the
> FindTabHelper.  (I also need a WebStateListObserver, but we've already bridged
> that to ObjC.)  Given the situation, it seemed simpler to have a single C++
> object, but it's not hard to do it the other way.

On one hand, consistency. On the other hand, mediator is a pattern, so we should
be able to implement it in whatever language makes sense.

So I guess leave it as is for now, but I think there will be a general pull
towards having mediators be ObjC. We'll see.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.h:39:
WebStateList* web_state_list_;
On 2017/03/16 14:00:17, rohitrao wrote:
> On 2017/03/16 13:37:05, marq (Slow - catching up) wrote:
> > Why does this need the list rather than a specific webstate?
> 
> Once TabCoordinator supports multiple WebStates, this mediator will become a
> WebStateList observer and update the find bar UI whenever the active WebState
> changes.

Or should something else be that observer and just update a WebState property on
this object? Should it be this object's job to know about the list of webStates?
This is probably part of a larger discussion about what happens when the active
webState changes.

[rohitrao (ping after 24h), 2017-03-20 23:37:55]

Rebased to pick up the changes that went in last week.

I've implemented Louis' suggestion of using a "consumer provider," as a
comparison to the proxy approach.  This approach seems more straightforward to
me -- does the proxy approach have other advantages that we should consider?

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm
(right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:58:
self.viewController.dispatcher = static_cast<id>(self.browser->dispatcher());
> Any time we cast to a protocol, we should at least DCHECK
> -conformsToProtocol:.
> 
> My assumption was that the dispatcher would explicitly conform to all of the
> command protocols, so we can do simple assignment when hooking it up to
clients.

This is still an open question.  Conforming to the protocol doesn't actually
provide any safety here, because there's no guarantee that anyone has registered
as a handler for a given method and no guarantee on initialization order between
callers and callees.

In the absence of actual safety checks, we couldn't come up with a reason to
have the dispatcher publicly conform to every protocol.

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.h (right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.h:39:
WebStateList* web_state_list_;
> Or should something else be that observer and just update a WebState property
on
> this object? Should it be this object's job to know about the list of
webStates?
> This is probably part of a larger discussion about what happens when the
active
> webState changes.

As you said, we can either farm this out and make everyone a
WebStateListObserver, or we can push updates down from the top.  I'm inclined to
the former, because otherwise we're going to add a bunch of plumbing that looks
similar to WebStateListObserver.

Is one of the options more testable than the other?

[marq (ping after 24h), 2017-04-06 16:13:27]

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm
(right):

https://codereview.chromium.org/2737563006/diff/80001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm:44:
forControlEvents:UIControlEventTouchUpInside];
On 2017/03/16 14:00:18, rohitrao wrote:
> On 2017/03/16 13:37:05, marq (Slow - catching up) wrote:
> > On 2017/03/14 01:10:37, rohitrao wrote:
> > > 10) Example of a VC setting an action on a control and setting the target
to
> > > |self|, rather than using the responder chain.
> > 
> > This means that the action protocol is either not needed, or can just live
in
> > the feature's ui/ subdirectory -- that is, ui/actions/ can go away.
> 
> This is a consequence of actions becoming specific to a single view->VC pair.
> 
> A huge downside to the code I have here is that all of the actions need to be
> set by the VC.  I believe there are two ways to send actions to targets:
> 1) Send the action to "nil" and have it propagate up the responder chain until
> something handles it.
> 2) Send the action to a specific object, which throws an exception if that
> object does not handle it.
> 
> I don't think there's a way for a button to send its action to |self| and have
> that propagate up to the VC.  But the button also isn't necessarily the first
> responder, so there's no guarantee that sending an action to nil will work,
> because the button and VC may not be in the responder chain.
> 
> The problem with this is that buttons can't configure their actions in
> isolation, because they don't have a direct pointer to their VC.  This would
> force us to put all of the action setting code in the VC.

I agree this is a problem, and it's one of the reasons we were keen on the
responder chain initially; a control could be entirely self-contained and
reusable without having to set it up at all. 

I don't have a good answer, unfortunately.

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/commands/find_in_page_search_commands.h
(right):

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/commands/find_in_page_search_commands.h:14: // Finds
the previous and next matches, using the current search string.
If -findStringInPage: hasn't been called, what is an adopter of this protocol
expected to do when either of these methods is called?

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm
(right):

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:93:
NSLog(@"showFindInPage command handler");
Remove logging.

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:101:
FindTabHelper* helper = FindTabHelper::FromWebState(webState);
Feels like the mediator should be doing this part.

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm (right):

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm:23:
@property(nonatomic, weak) CommandDispatcher<FindInPageVisibilityCommands>*
dispatcher;
Line wrap?

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm:54:
[_dispatcher startDispatchingToTarget:self
Another option is that the coordinator tells the dispatcher to dispatch to the
mediator. Not sure if that's helpful though.

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm:71: -
(void)onFindResultsAvailable:(FindInPageModel*)model {
Nit: 'on' for delegate-y/observer-y methods is a C++ism more than an ObjC-ism;
prefer just 'findResultsAvailable' or 'findResultsBecameAvailable' or something.

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm:72:
NSLog(@"%lu of %lu", model.currentIndex, model.matches);
Remove logging.

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm:74: if
(!consumer) {
Remove the nil test?

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.h
(right):

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.h:16:
@property(nonatomic, readwrite, weak)
id<FindInPageSearchCommands,FindInPageVisibilityCommands> dispatcher;
git cl format

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm
(right):

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm:22: //
Redeclare the |view| property to be the FindBarView subclass instead of a
So for the omnibox, we have the view controller's view remain a UIView that acts
as a container for the omnibox text field. Here, we just have the FindBarView
directly as the view. Is one approach better than the other? 

(I prefer not to use spurious container views if we can avoid it).

[rohitrao (ping after 24h), 2017-04-06 16:27:07]

Several questions below that are points for discussion:

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/commands/find_in_page_search_commands.h
(right):

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/commands/find_in_page_search_commands.h:14: // Finds
the previous and next matches, using the current search string.
On 2017/04/06 16:13:27, marq wrote:
> If -findStringInPage: hasn't been called, what is an adopter of this protocol
> expected to do when either of these methods is called?

Will figure this out and update the comment.  (In practice, it will probably
open the find bar and "search" for the empty string.)

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm
(right):

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:101:
FindTabHelper* helper = FindTabHelper::FromWebState(webState);
On 2017/04/06 16:13:27, marq wrote:
> Feels like the mediator should be doing this part.

Agreed, but I couldn't figure out how to push this into the mediator.

If I moved this into the mediator, how would the mediator cause the UI to be
closed?  We could use the dispatcher for that, but then we'd have both a
"public" hideFindInPage command as well as an "internal" noReallyCloseTheUINow
command, which I don't like.

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm (right):

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm:54:
[_dispatcher startDispatchingToTarget:self
On 2017/04/06 16:13:27, marq wrote:
> Another option is that the coordinator tells the dispatcher to dispatch to the
> mediator. Not sure if that's helpful though.

I originally had it that way.  How do we feel about exposing a bunch of "Handle
Foo Command" methods as public methods on the mediator?

This might address two other concerns:
1) The command protocol is effectively being split up based on who is handling
the messages, and not by any real semantic grouping.  Visibility vs Search kinda
makes sense as a reasonable split, but honestly I'm just rationalizing a split
that was necessitated by how I handled the commands.

2) The implementation of hideFindInPage could ask the mediator to end the find
operation and then call [self stop], which pushes the interaction with the
FindTabHelper out of the coordinator.

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm
(right):

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm:22: //
Redeclare the |view| property to be the FindBarView subclass instead of a
On 2017/04/06 16:13:27, marq wrote:
> So for the omnibox, we have the view controller's view remain a UIView that
acts
> as a container for the omnibox text field. Here, we just have the FindBarView
> directly as the view. Is one approach better than the other? 
> 
> (I prefer not to use spurious container views if we can avoid it).

We should avoid spurious containers if we can.  The location bar view contains
only the textfield for now, but it will eventually have other children (star
button, voice search button, etc).  I may change that one to be a UIStackView,
actually.

[marq (ping after 24h), 2017-04-07 09:45:56]

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm
(right):

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:101:
FindTabHelper* helper = FindTabHelper::FromWebState(webState);
On 2017/04/06 16:27:07, rohitrao wrote:
> On 2017/04/06 16:13:27, marq wrote:
> > Feels like the mediator should be doing this part.
> 
> Agreed, but I couldn't figure out how to push this into the mediator.
> 
> If I moved this into the mediator, how would the mediator cause the UI to be
> closed?  We could use the dispatcher for that, but then we'd have both a
> "public" hideFindInPage command as well as an "internal" noReallyCloseTheUINow
> command, which I don't like.

Would just replacing lines 100-102 with [self.mediator stopFinding] be horrible?
That keeps the coordinator from caring about the internals of how FIP works, but
it retains the responsibility for catching commands and bossing the view
controller around.

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm (right):

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm:54:
[_dispatcher startDispatchingToTarget:self
On 2017/04/06 16:27:07, rohitrao wrote:
> On 2017/04/06 16:13:27, marq wrote:
> > Another option is that the coordinator tells the dispatcher to dispatch to
the
> > mediator. Not sure if that's helpful though.
> 
> I originally had it that way.  How do we feel about exposing a bunch of
"Handle
> Foo Command" methods as public methods on the mediator?
> 
> This might address two other concerns:
> 1) The command protocol is effectively being split up based on who is handling
> the messages, and not by any real semantic grouping.  Visibility vs Search
kinda
> makes sense as a reasonable split, but honestly I'm just rationalizing a split
> that was necessitated by how I handled the commands.
> 
> 2) The implementation of hideFindInPage could ask the mediator to end the find
> operation and then call [self stop], which pushes the interaction with the
> FindTabHelper out of the coordinator.

My suggestion was just that the mediator publicly adopts
FindInPageSearchCommands, and then the coordinator calls:

[dispatcher startDispatchingToTarget:self.mediator forSelector...]

for those commands.

(the idea there being that the dispatcher itself doesn't get passed out of the
coordinator, but the coordinator can hook other objects into it both as senders
(by passing the dispatcher as an id<Foo>) and as receivers (as in this example).

That's separate from the problem of commands that need to do both model (and
thus mediator) work and UI work, like hideFindInPage.

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm
(right):

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.mm:22: //
Redeclare the |view| property to be the FindBarView subclass instead of a
On 2017/04/06 16:27:07, rohitrao wrote:
> On 2017/04/06 16:13:27, marq wrote:
> > So for the omnibox, we have the view controller's view remain a UIView that
> acts
> > as a container for the omnibox text field. Here, we just have the
FindBarView
> > directly as the view. Is one approach better than the other? 
> > 
> > (I prefer not to use spurious container views if we can avoid it).
> 
> We should avoid spurious containers if we can.  The location bar view contains
> only the textfield for now, but it will eventually have other children (star
> button, voice search button, etc).  I may change that one to be a UIStackView,
> actually.

Acknowledged.

[rohitrao (ping after 24h), 2017-04-11 20:46:09]

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

[rohitrao (ping after 24h), 2017-04-11 20:46:16]

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/commands/find_in_page_search_commands.h
(right):

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/commands/find_in_page_search_commands.h:14: // Finds
the previous and next matches, using the current search string.
On 2017/04/06 16:27:07, rohitrao wrote:
> On 2017/04/06 16:13:27, marq wrote:
> > If -findStringInPage: hasn't been called, what is an adopter of this
protocol
> > expected to do when either of these methods is called?
> 
> Will figure this out and update the comment.  (In practice, it will probably
> open the find bar and "search" for the empty string.)

Done.

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm
(right):

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:93:
NSLog(@"showFindInPage command handler");
On 2017/04/06 16:13:27, marq wrote:
> Remove logging.

Done.

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_coordinator.mm:101:
FindTabHelper* helper = FindTabHelper::FromWebState(webState);
On 2017/04/07 09:45:55, marq wrote:
> On 2017/04/06 16:27:07, rohitrao wrote:
> > On 2017/04/06 16:13:27, marq wrote:
> > > Feels like the mediator should be doing this part.
> > 
> > Agreed, but I couldn't figure out how to push this into the mediator.
> > 
> > If I moved this into the mediator, how would the mediator cause the UI to be
> > closed?  We could use the dispatcher for that, but then we'd have both a
> > "public" hideFindInPage command as well as an "internal"
noReallyCloseTheUINow
> > command, which I don't like.
> 
> Would just replacing lines 100-102 with [self.mediator stopFinding] be
horrible?
> That keeps the coordinator from caring about the internals of how FIP works,
but
> it retains the responsibility for catching commands and bossing the view
> controller around.

Done, at the cost of a larger public API for the mediator.

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm (right):

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm:23:
@property(nonatomic, weak) CommandDispatcher<FindInPageVisibilityCommands>*
dispatcher;
On 2017/04/06 16:13:27, marq wrote:
> Line wrap?

Ran git cl format.

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm:54:
[_dispatcher startDispatchingToTarget:self
On 2017/04/07 09:45:55, marq wrote:
> On 2017/04/06 16:27:07, rohitrao wrote:
> > On 2017/04/06 16:13:27, marq wrote:
> > > Another option is that the coordinator tells the dispatcher to dispatch to
> the
> > > mediator. Not sure if that's helpful though.
> > 
> > I originally had it that way.  How do we feel about exposing a bunch of
> "Handle
> > Foo Command" methods as public methods on the mediator?
> > 
> > This might address two other concerns:
> > 1) The command protocol is effectively being split up based on who is
handling
> > the messages, and not by any real semantic grouping.  Visibility vs Search
> kinda
> > makes sense as a reasonable split, but honestly I'm just rationalizing a
split
> > that was necessitated by how I handled the commands.
> > 
> > 2) The implementation of hideFindInPage could ask the mediator to end the
find
> > operation and then call [self stop], which pushes the interaction with the
> > FindTabHelper out of the coordinator.
> 
> My suggestion was just that the mediator publicly adopts
> FindInPageSearchCommands, and then the coordinator calls:
> 
> [dispatcher startDispatchingToTarget:self.mediator forSelector...]
> 
> for those commands.
> 
> (the idea there being that the dispatcher itself doesn't get passed out of the
> coordinator, but the coordinator can hook other objects into it both as
senders
> (by passing the dispatcher as an id<Foo>) and as receivers (as in this
example).
> 
> That's separate from the problem of commands that need to do both model (and
> thus mediator) work and UI work, like hideFindInPage. 

Done.

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm:71: -
(void)onFindResultsAvailable:(FindInPageModel*)model {
On 2017/04/06 16:13:27, marq wrote:
> Nit: 'on' for delegate-y/observer-y methods is a C++ism more than an ObjC-ism;
> prefer just 'findResultsAvailable' or 'findResultsBecameAvailable' or
something.

Done.

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm:72:
NSLog(@"%lu of %lu", model.currentIndex, model.matches);
On 2017/04/06 16:13:27, marq wrote:
> Remove logging.

Done.

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_mediator.mm:74: if
(!consumer) {
On 2017/04/06 16:13:27, marq wrote:
> Remove the nil test?

Removed, but for more complex updates, I think I would prefer to bail early
rather than send tens of messages to a nil consumer.  It provides a clearer
signal that nil is a possible and expected value for |consumer|.

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.h
(right):

https://codereview.chromium.org/2737563006/diff/120001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/find_in_page/find_in_page_view_controller.h:16:
@property(nonatomic, readwrite, weak)
id<FindInPageSearchCommands,FindInPageVisibilityCommands> dispatcher;
On 2017/04/06 16:13:27, marq wrote:
> git cl format

Done.

[commit-bot: I haz the power, 2017-04-11 20:46:58]

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-11 20:56:17]

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

[commit-bot: I haz the power, 2017-04-11 20:56:17]

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

[rohitrao (ping after 24h), 2017-04-11 22:26:41]

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

[commit-bot: I haz the power, 2017-04-11 22:27:09]

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-11 23:45:55]

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

[commit-bot: I haz the power, 2017-04-11 23:45:57]

Dry run: This issue passed the CQ dry run.

[marq (ping after 24h), 2017-04-12 11:40:23]

lgtm

[rohitrao (ping after 24h), 2017-04-12 12:00:51]

The CQ bit was checked by rohitrao@chromium.org

[commit-bot: I haz the power, 2017-04-12 12:01:01]

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-12 12:07:20]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1491998451607510,
"parent_rev": "4faf6a64037e14070bd243ceb964ac682e96808d", "commit_rev":
"acef7bfbb5007ff1ecf0c3ee734fb1f4197e851a"}

[commit-bot: I haz the power, 2017-04-12 12:08:16]

Description was changed from

==========
[ios] Adds support for Find in Page to the new architecture.

BUG=
==========

to

==========
[ios] Adds support for Find in Page to the new architecture.

BUG=

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

[commit-bot: I haz the power, 2017-04-12 12:08:17]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/acef7bfbb5007ff1ecf0c3ee734f...