[ios] Switches ToolbarModelDelegateIOS to use WebStateList.

[ios] Switches ToolbarModelDelegateIOS to use WebStateList.

There is one method whose implementation still requires Tab and WebController.
That method has been temporarily implemented using LegacyTabHelper to get the
Tab for a WebState.

BUG=None

Review-Url: https://codereview.chromium.org/2770223003
Cr-Commit-Position: refs/heads/master@{#459504}
Committed: https://chromium.googlesource.com/chromium/src/+/5eb449e1e24522d224cc61be49c0cce7c48d05b3

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

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

[commit-bot: I haz the power, 2017-03-24 14:09:33]

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-03-24 14:13:48]

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

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

https://codereview.chromium.org/2770223003/diff/1/ios/chrome/browser/ui/brows...
File ios/chrome/browser/ui/browser_view_controller_unittest.mm (right):

https://codereview.chromium.org/2770223003/diff/1/ios/chrome/browser/ui/brows...
ios/chrome/browser/ui/browser_view_controller_unittest.mm:118: @interface
BVCTestTabModel : OCMockComplexTypeHelper
This is "broken" because the ToolbarModel's WebStateList is not kept in sync
with the actual mock TabModel, but the ToolbarModel isn't used for anything in
this test, so everything passes just fine.

We should revisit and rewrite this test once TabModel is simply a shim on top of
WebStateList.  At that point, we can probably get rid of mock TabModels across
the whole codebase.

https://codereview.chromium.org/2770223003/diff/1/ios/chrome/browser/ui/toolb...
File ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios.mm (right):

https://codereview.chromium.org/2770223003/diff/1/ios/chrome/browser/ui/toolb...
ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios.mm:95: return tab &&
[tab.webController wantsLocationBarHintText];
There's no public API for this yet.

[commit-bot: I haz the power, 2017-03-24 14:18:01]

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

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

Dry run: Try jobs failed on following builders:
  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-03-24 14:29:46]

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

[commit-bot: I haz the power, 2017-03-24 14:30:16]

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

[sdefresne, 2017-03-24 15:09:52]

lgtm

https://codereview.chromium.org/2770223003/diff/20001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/toolbar_model_delegate_ios.mm (right):

https://codereview.chromium.org/2770223003/diff/20001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_model_delegate_ios.mm:24: :
web_state_list_(web_state_list) {}
I think we should DCHECK that web_state_list_ is not null here (or remove the
DCHECK in SetWebStateList). WDYT?

https://codereview.chromium.org/2770223003/diff/20001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios_unittest.mm (right):

https://codereview.chromium.org/2770223003/diff/20001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios_unittest.mm:87:
web_state_list_ = base::MakeUnique<WebStateList>(
Should we have a TestWebStateList that always contains one TestWebState as the
active web state?

https://codereview.chromium.org/2770223003/diff/20001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios_unittest.mm:94:
web_state_list_->ActivateWebStateAt(0);
Should WebStateList always mark the first WebState as active when it is
inserted?

[rohitrao (ping after 24h), 2017-03-24 15:40:21]

https://codereview.chromium.org/2770223003/diff/20001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/toolbar_model_delegate_ios.mm (right):

https://codereview.chromium.org/2770223003/diff/20001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_model_delegate_ios.mm:24: :
web_state_list_(web_state_list) {}
On 2017/03/24 15:09:52, sdefresne wrote:
> I think we should DCHECK that web_state_list_ is not null here (or remove the
> DCHECK in SetWebStateList). WDYT?

I don't think anyone calls SetWebStateList(), so I've deleted it.  I didn't
bother DCHECKing().

https://codereview.chromium.org/2770223003/diff/20001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios_unittest.mm (right):

https://codereview.chromium.org/2770223003/diff/20001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios_unittest.mm:87:
web_state_list_ = base::MakeUnique<WebStateList>(
On 2017/03/24 15:09:52, sdefresne wrote:
> Should we have a TestWebStateList that always contains one TestWebState as the
> active web state?

I think that would probably be useful?  We can add it when we start converting
other tests away from TabModel, because that will give us an idea of what
specifically we need.

https://codereview.chromium.org/2770223003/diff/20001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/toolbar/toolbar_model_impl_ios_unittest.mm:94:
web_state_list_->ActivateWebStateAt(0);
On 2017/03/24 15:09:52, sdefresne wrote:
> Should WebStateList always mark the first WebState as active when it is
> inserted?

No, I think it's better for callers to do this explicitly.  TestWebStateList
could add one WebState and activate it.

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

The CQ bit was checked by rohitrao@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2770223003/#ps40001
(title: "Review")

[commit-bot: I haz the power, 2017-03-24 16:00:48]

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-03-24 19:05:28]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490371206011060,
"parent_rev": "8fbf167e58ade9a7f226503f8b0cc1b91987d3a0", "commit_rev":
"5eb449e1e24522d224cc61be49c0cce7c48d05b3"}

[commit-bot: I haz the power, 2017-03-24 19:06:13]

Description was changed from

==========
[ios] Switches ToolbarModelDelegateIOS to use WebStateList.

There is one method whose implementation still requires Tab and WebController.
That method has been temporarily implemented using LegacyTabHelper to get the
Tab for a WebState.

BUG=None
==========

to

==========
[ios] Switches ToolbarModelDelegateIOS to use WebStateList.

There is one method whose implementation still requires Tab and WebController.
That method has been temporarily implemented using LegacyTabHelper to get the
Tab for a WebState.

BUG=None

Review-Url: https://codereview.chromium.org/2770223003
Cr-Commit-Position: refs/heads/master@{#459504}
Committed:
https://chromium.googlesource.com/chromium/src/+/5eb449e1e24522d224cc61be49c0...
==========

[commit-bot: I haz the power, 2017-03-24 19:06:14]

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

[rohitrao (ping after 24h), 2017-03-24 20:42:17]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2775943002/ by rohitrao@chromium.org.

The reason for reverting is: EG tests fail

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.fyi%2FEarlGreyi....