Move the notion of current Tab from TabModel to WebStateList.

Move the notion of current Tab from TabModel to WebStateList.

Add methods to track and set the currently active WebState in the
WebStateList (referenced by index) and corresponding events when
the active WebState is changed (explicitly or automatically after
detaching a WebState).

Add new WebStateListObserver (and WebStateListObserving when the
observer need to keep a __weak pointer to an Objective-C class)
that react to the event send when the active WebState changes.

Add a -loadFinished property to Tab to avoid accessing the private
//ios/web private API in new code.

BUG=687207
Review-Url: https://codereview.chromium.org/2703333006
Cr-Commit-Position: refs/heads/master@{#454329}
Committed: https://chromium.googlesource.com/chromium/src/+/2f7781ce46951904a2615fbda44d2a9f4ced23ca

[sdefresne, 2017-02-21 16:33:47]

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

[sdefresne, 2017-02-21 16:33:48]

Please both take a look.

I tried to split this CL in smaller parts but did not manage to do it as I need
to move at the same time the currentTab to WebStateList and implement the
observers.

[sdefresne, 2017-02-21 17:09:49]

https://codereview.chromium.org/2703333006/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm (right):

https://codereview.chromium.org/2703333006/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm:13: __weak TabModel*
_tabModel;
This is an Objective-C class because I cannot use __weak in a header included by
tab_model.mm (as the file is compiled without ARC support).

[marq (ping after 24h), 2017-02-21 17:23:11]

LGTM but it's very likely I missed some issues, so please wait for rohitrao@'s
LGTM as well.

https://codereview.chromium.org/2703333006/diff/1/ios/shared/chrome/browser/t...
File ios/shared/chrome/browser/tabs/web_state_list.mm (right):

https://codereview.chromium.org/2703333006/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list.mm:238: if (active_index_ > index)
Use {} if there's an else clause.

[sdefresne, 2017-02-21 17:29:29]

Thank you for the review.

Yes, my plan is to wait for review by you both.

https://codereview.chromium.org/2703333006/diff/1/ios/shared/chrome/browser/t...
File ios/shared/chrome/browser/tabs/web_state_list.mm (right):

https://codereview.chromium.org/2703333006/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list.mm:238: if (active_index_ > index)
On 2017/02/21 17:23:11, marq wrote:
> Use {} if there's an else clause.

I checked the style guide [1] and it does not say to use {} if there is an else
clause. It says that if one of the two parts of the conditional uses braces,
then both need to use them. It also says that single line conditional may omit
braces. It understand the conjunction of both statement as meaning that you can
omit them in both part of the conditional if they are both single lines.

[1]: https://google.github.io/styleguide/cppguide.html#Conditionals

[rohitrao (ping after 24h), 2017-02-22 15:13:43]

lgtm

I think this is ok.  My biggest concern is that we're reordering the code that
used to be in changeSelectedTabFrom:to:persistState:, and maybe some of that
code can't be reordered.

https://codereview.chromium.org/2703333006/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm (right):

https://codereview.chromium.org/2703333006/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm:34: // Save state,
such as scroll position, before switching tabs.
This comment is half stale, because we no longer switch tabs in this function.

https://codereview.chromium.org/2703333006/diff/1/ios/shared/chrome/browser/t...
File ios/shared/chrome/browser/tabs/web_state_list.mm (right):

https://codereview.chromium.org/2703333006/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list.mm:231: web::WebState* web_state =
web_state_wrappers_[index]->web_state();
What keeps ownership of this object so that it doesn't get destroyed when the
next line runs?

[sdefresne, 2017-02-23 15:43:48]

I've not been able to find an issue with the re-ordering. If we want to not
change the order, it is possible to split TabModelSelectedTabObserver in two
classes and register the listeners in the correct order (one before
TabModelObserversBridge, the other after).

However, if we convert TabModelObservers to WebStateListObserving, then we won't
be able to ensure the ordering of the events won't be changed, so I'm not sure
whether it is worth doing or not.

WDYT?

https://codereview.chromium.org/2703333006/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm (right):

https://codereview.chromium.org/2703333006/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_selected_tab_observer.mm:34: // Save state,
such as scroll position, before switching tabs.
On 2017/02/22 15:13:43, rohitrao wrote:
> This comment is half stale, because we no longer switch tabs in this function.

Fixed the comment.

https://codereview.chromium.org/2703333006/diff/1/ios/shared/chrome/browser/t...
File ios/shared/chrome/browser/tabs/web_state_list.mm (right):

https://codereview.chromium.org/2703333006/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list.mm:231: web::WebState* web_state =
web_state_wrappers_[index]->web_state();
On 2017/02/22 15:13:43, rohitrao wrote:
> What keeps ownership of this object so that it doesn't get destroyed when the
> next line runs?

That's a really good remark. Fixed by
https://codereview.chromium.org/2710183003/.

[sdefresne, 2017-02-23 16:37:26]

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

[commit-bot: I haz the power, 2017-02-23 16:37:47]

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-23 16:40:14]

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

[commit-bot: I haz the power, 2017-02-23 16:40:15]

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-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...)

[sdefresne, 2017-03-02 03:20:27]

rohitrao: ping

[sdefresne, 2017-03-02 03:42:12]

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

[commit-bot: I haz the power, 2017-03-02 03:43:51]

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-03-02 07:33:15]

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

[commit-bot: I haz the power, 2017-03-02 07:33:16]

Dry run: This issue passed the CQ dry run.

[rohitrao (ping after 24h), 2017-03-02 18:26:40]

LGTM

I don't know of any concrete issues with reordering code either, was just
pointing it out as an area of risk.  If you've considered it in detail, that's
good enough for me, thanks!

[sdefresne, 2017-03-02 19:06:34]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2017-03-02 19:06:34]

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

[commit-bot: I haz the power, 2017-03-02 19:07:04]

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-02 19:12:50]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488481594100090,
"parent_rev": "e30455327cf0108d83b431127908c465e71d98b1", "commit_rev":
"2f7781ce46951904a2615fbda44d2a9f4ced23ca"}

[commit-bot: I haz the power, 2017-03-02 19:13:32]

Description was changed from

==========
Move the notion of current Tab from TabModel to WebStateList.

Add methods to track and set the currently active WebState in the
WebStateList (referenced by index) and corresponding events when
the active WebState is changed (explicitly or automatically after
detaching a WebState).

Add new WebStateListObserver (and WebStateListObserving when the
observer need to keep a __weak pointer to an Objective-C class)
that react to the event send when the active WebState changes.

Add a -loadFinished property to Tab to avoid accessing the private
//ios/web private API in new code.

BUG=687207
==========

to

==========
Move the notion of current Tab from TabModel to WebStateList.

Add methods to track and set the currently active WebState in the
WebStateList (referenced by index) and corresponding events when
the active WebState is changed (explicitly or automatically after
detaching a WebState).

Add new WebStateListObserver (and WebStateListObserving when the
observer need to keep a __weak pointer to an Objective-C class)
that react to the event send when the active WebState changes.

Add a -loadFinished property to Tab to avoid accessing the private
//ios/web private API in new code.

BUG=687207

Review-Url: https://codereview.chromium.org/2703333006
Cr-Commit-Position: refs/heads/master@{#454329}
Committed:
https://chromium.googlesource.com/chromium/src/+/2f7781ce46951904a2615fbda44d...
==========

[commit-bot: I haz the power, 2017-03-02 19:13:34]

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