[ios] Moves find-in-page code out of Tab and into FindTabHelper.

[ios] Moves find-in-page code out of Tab and into FindTabHelper.

BUG=681867
Review-Url: https://codereview.chromium.org/2654433007
Cr-Commit-Position: refs/heads/master@{#449610}
Committed: https://chromium.googlesource.com/chromium/src/+/b2bf3cb69cae29c1dc85f23a18f2f36587290c1e

[rohitrao (ping after 24h), 2017-01-31 15:49:41]

rohitrao@chromium.org changed reviewers:
+ eugenebut@chromium.org, justincohen@chromium.org, marq@chromium.org

[rohitrao (ping after 24h), 2017-01-31 15:49:41]

Preview of my changes in progress.  I've uploaded Step 1, which removes
FIP-specific code from Tab.  Step 2 will remove code from BVC.  I haven't yet
decided if this will be one CL or two.

[marq (ping after 24h), 2017-01-31 16:02:49]

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:294: // nullptr, returns
nullptr.
Why not just return the helper's FindInPageController or nil, since that's the
only part of the helper this class cares about? Then you can get rid of all the
null checking for the helper.

[Eugene But (OOO till 7-30), 2017-01-31 19:03:48]

Could you please add tests for newly written code.

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/find...
File ios/chrome/browser/find_in_page/find_tab_helper.h (right):

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/find...
ios/chrome/browser/find_in_page/find_tab_helper.h:9: #import
"ios/web/public/web_state/web_state_observer.h"
nit: s/import/include

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/find...
ios/chrome/browser/find_in_page/find_tab_helper.h:16: class FindTabHelper :
public web::WebStateObserver,
Should this be FindInPageTabHelper to keep consistent terminology?

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/find...
ios/chrome/browser/find_in_page/find_tab_helper.h:19: static void
CreateForWebState(
Could you please add comments. F.e. is it ok to pass null delegate?

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/find...
ios/chrome/browser/find_in_page/find_tab_helper.h:23:
FindTabHelper(web::WebState* web_state,
Should constructor, destructor and NavigationItemCommitted be private?

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/find...
File ios/chrome/browser/find_in_page/find_tab_helper.mm (right):

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/find...
ios/chrome/browser/find_in_page/find_tab_helper.mm:8: 
Please add ARC ifdefs

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/find...
ios/chrome/browser/find_in_page/find_tab_helper.mm:31: [controller_
detachFromWebState];
We should probably do this in |WebStateDestroyed|

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:299: return
FindTabHelper::FromWebState(tab.webState);
Optional nit: do you want to use ternary operator here?

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:3991:
FindTabHelper::FromWebState([_model currentTab].webState)
Should you use your helper function here and in other places?

[rohitrao (ping after 24h), 2017-02-08 16:14:29]

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

[commit-bot: I haz the power, 2017-02-08 16:14:55]

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

[rohitrao (ping after 24h), 2017-02-08 16:14:59]

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/find...
File ios/chrome/browser/find_in_page/find_tab_helper.h (right):

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/find...
ios/chrome/browser/find_in_page/find_tab_helper.h:9: #import
"ios/web/public/web_state/web_state_observer.h"
On 2017/01/31 19:03:48, Eugene But wrote:
> nit: s/import/include

Done.

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/find...
ios/chrome/browser/find_in_page/find_tab_helper.h:16: class FindTabHelper :
public web::WebStateObserver,
On 2017/01/31 19:03:48, Eugene But wrote:
> Should this be FindInPageTabHelper to keep consistent terminology? 

I kept it consistent with desktop.

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/find...
ios/chrome/browser/find_in_page/find_tab_helper.h:19: static void
CreateForWebState(
On 2017/01/31 19:03:48, Eugene But wrote:
> Could you please add comments. F.e. is it ok to pass null delegate?

It is now =)

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/find...
ios/chrome/browser/find_in_page/find_tab_helper.h:23:
FindTabHelper(web::WebState* web_state,
On 2017/01/31 19:03:48, Eugene But wrote:
> Should constructor, destructor and NavigationItemCommitted be private?

Done.

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/find...
File ios/chrome/browser/find_in_page/find_tab_helper.mm (right):

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/find...
ios/chrome/browser/find_in_page/find_tab_helper.mm:8: 
On 2017/01/31 19:03:48, Eugene But wrote:
> Please add ARC ifdefs

Done.

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/find...
ios/chrome/browser/find_in_page/find_tab_helper.mm:31: [controller_
detachFromWebState];
On 2017/01/31 19:03:48, Eugene But wrote:
> We should probably do this in |WebStateDestroyed|

Done.

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:294: // nullptr, returns
nullptr.
On 2017/01/31 16:02:49, marq wrote:
> Why not just return the helper's FindInPageController or nil, since that's the
> only part of the helper this class cares about? Then you can get rid of all
the
> null checking for the helper.

Done.

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:299: return
FindTabHelper::FromWebState(tab.webState);
On 2017/01/31 19:03:48, Eugene But wrote:
> Optional nit: do you want to use ternary operator here?

Acknowledged.

https://codereview.chromium.org/2654433007/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:3991:
FindTabHelper::FromWebState([_model currentTab].webState)
On 2017/01/31 19:03:48, Eugene But wrote:
> Should you use your helper function here and in other places?

Changed for now, but I'm going to revisit.  I think in some of these places it's
safe to assume that |tab| is not nil.

[rohitrao (ping after 24h), 2017-02-08 16:15:52]

The tests are weak sauce, but should get better as I pull more functionality out
of BVC and do some other cleanups.

[commit-bot: I haz the power, 2017-02-08 16:16:49]

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

[commit-bot: I haz the power, 2017-02-08 16:16:50]

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...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[rohitrao (ping after 24h), 2017-02-08 16:22:18]

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

[commit-bot: I haz the power, 2017-02-08 16:22:45]

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-02-08 17:00:52]

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

[commit-bot: I haz the power, 2017-02-08 17:00:53]

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

[rohitrao (ping after 24h), 2017-02-08 19:21:27]

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

[commit-bot: I haz the power, 2017-02-08 19:22:22]

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-02-08 21:28:34]

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

[commit-bot: I haz the power, 2017-02-08 21:28:35]

Dry run: This issue passed the CQ dry run.

[marq (ping after 24h), 2017-02-09 11:22:47]

lgtm

[Eugene But (OOO till 7-30), 2017-02-10 00:03:37]

lgtm, thank you for the test

https://codereview.chromium.org/2654433007/diff/80001/ios/chrome/browser/find...
File ios/chrome/browser/find_in_page/find_tab_helper.mm (right):

https://codereview.chromium.org/2654433007/diff/80001/ios/chrome/browser/find...
ios/chrome/browser/find_in_page/find_tab_helper.mm:21:
web_state->SetUserData(UserDataKey(),
nit: do you want to add braces, because block is multiline?

Style Guide is quite vague on that:

"In general, curly braces are not required for single-line statements, but they
are allowed if you like them; conditional or loop statements with complex
conditions or statements may be more readable with curly braces. Some projects
require that an if must always always have an accompanying brace."

https://codereview.chromium.org/2654433007/diff/80001/ios/chrome/browser/find...
File ios/chrome/browser/find_in_page/find_tab_helper_unittest.mm (right):

https://codereview.chromium.org/2654433007/diff/80001/ios/chrome/browser/find...
ios/chrome/browser/find_in_page/find_tab_helper_unittest.mm:25:
DISALLOW_COPY_AND_ASSIGN(FindTabHelperTest);
nit: Do we need this?

https://codereview.chromium.org/2654433007/diff/80001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2654433007/diff/80001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:3834: FindInPageController*
findInPageController =
|FindInPageController* findInPageController = GetFindInPageController([_model
currentTab])| ?

https://codereview.chromium.org/2654433007/diff/80001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:3845: FindInPageController*
findInPageController =
ditto

[sdefresne, 2017-02-10 09:39:51]

drive-by comment

https://codereview.chromium.org/2654433007/diff/80001/ios/chrome/browser/find...
File ios/chrome/browser/find_in_page/find_tab_helper.mm (right):

https://codereview.chromium.org/2654433007/diff/80001/ios/chrome/browser/find...
ios/chrome/browser/find_in_page/find_tab_helper.mm:21:
web_state->SetUserData(UserDataKey(),
On 2017/02/10 00:03:36, Eugene But wrote:
> nit: do you want to add braces, because block is multiline?
> 
> Style Guide is quite vague on that:
> 
> "In general, curly braces are not required for single-line statements, but
they
> are allowed if you like them; conditional or loop statements with complex
> conditions or statements may be more readable with curly braces. Some projects
> require that an if must always always have an accompanying brace."
> 

Even though the style guide does not enforce this, it looks like brace on
complex if statement are more frequent in Chromium (at least my reviewer ask me
to put braces there).

[rohitrao (ping after 24h), 2017-02-10 12:57:03]

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

[commit-bot: I haz the power, 2017-02-10 12:57:21]

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

[rohitrao (ping after 24h), 2017-02-10 13:52:18]

https://codereview.chromium.org/2654433007/diff/80001/ios/chrome/browser/find...
File ios/chrome/browser/find_in_page/find_tab_helper.mm (right):

https://codereview.chromium.org/2654433007/diff/80001/ios/chrome/browser/find...
ios/chrome/browser/find_in_page/find_tab_helper.mm:21:
web_state->SetUserData(UserDataKey(),
On 2017/02/10 09:39:51, sdefresne wrote:
> On 2017/02/10 00:03:36, Eugene But wrote:
> > nit: do you want to add braces, because block is multiline?
> > 
> > Style Guide is quite vague on that:
> > 
> > "In general, curly braces are not required for single-line statements, but
> they
> > are allowed if you like them; conditional or loop statements with complex
> > conditions or statements may be more readable with curly braces. Some
projects
> > require that an if must always always have an accompanying brace."
> > 
> 
> Even though the style guide does not enforce this, it looks like brace on
> complex if statement are more frequent in Chromium (at least my reviewer ask
me
> to put braces there).

I prefer braces for all conditionals, thanks =)

https://codereview.chromium.org/2654433007/diff/80001/ios/chrome/browser/find...
File ios/chrome/browser/find_in_page/find_tab_helper_unittest.mm (right):

https://codereview.chromium.org/2654433007/diff/80001/ios/chrome/browser/find...
ios/chrome/browser/find_in_page/find_tab_helper_unittest.mm:25:
DISALLOW_COPY_AND_ASSIGN(FindTabHelperTest);
On 2017/02/10 00:03:36, Eugene But wrote:
> nit: Do we need this? 

I've always included it in every class, unless I explicitly want them to be
copyable.  Does it do harm to have it?

https://codereview.chromium.org/2654433007/diff/80001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2654433007/diff/80001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:3834: FindInPageController*
findInPageController =
On 2017/02/10 00:03:36, Eugene But wrote:
> |FindInPageController* findInPageController = GetFindInPageController([_model
> currentTab])| ?

Missed these two, thanks.

[commit-bot: I haz the power, 2017-02-10 14:03:15]

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

[commit-bot: I haz the power, 2017-02-10 14:03:16]

Dry run: This issue passed the CQ dry run.

[rohitrao (ping after 24h), 2017-02-10 14:06:10]

The CQ bit was checked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2017-02-10 14:06:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2654433007/#ps100001
(title: "Review.")

[commit-bot: I haz the power, 2017-02-10 14:06:22]

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-02-10 14:10:39]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486735570073990,
"parent_rev": "446d56d8eb1add44e684834cd29406e0a9c9eace", "commit_rev":
"b2bf3cb69cae29c1dc85f23a18f2f36587290c1e"}

[commit-bot: I haz the power, 2017-02-10 14:11:23]

Description was changed from

==========
[ios] Moves find-in-page code out of Tab and into FindTabHelper.

BUG=681867
==========

to

==========
[ios] Moves find-in-page code out of Tab and into FindTabHelper.

BUG=681867

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

[commit-bot: I haz the power, 2017-02-10 14:11:25]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/b2bf3cb69cae29c1dc85f23a18f2...

[Eugene But (OOO till 7-30), 2017-02-10 18:31:20]

https://codereview.chromium.org/2654433007/diff/80001/ios/chrome/browser/find...
File ios/chrome/browser/find_in_page/find_tab_helper_unittest.mm (right):

https://codereview.chromium.org/2654433007/diff/80001/ios/chrome/browser/find...
ios/chrome/browser/find_in_page/find_tab_helper_unittest.mm:25:
DISALLOW_COPY_AND_ASSIGN(FindTabHelperTest);
On 2017/02/10 13:52:18, rohitrao wrote:
> On 2017/02/10 00:03:36, Eugene But wrote:
> > nit: Do we need this? 
> 
> I've always included it in every class, unless I explicitly want them to be
> copyable.  Does it do harm to have it?
I can't recall any tests which have this macro. I think this affects readability
as this code is not useful but you still have to read it.