Convert some of TabModel logic to WebStateListObservers.

Convert some of TabModel logic to WebStateListObservers.

BUG=687207
Review-Url: https://codereview.chromium.org/2687303003
Cr-Commit-Position: refs/heads/master@{#451593}
Committed: https://chromium.googlesource.com/chromium/src/+/39bfaaba5dc0d78349ccb2a1a6c2eb42d16dca69

[sdefresne, 2017-02-10 16:39:52]

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

[sdefresne, 2017-02-10 16:40:12]

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

[sdefresne, 2017-02-10 16:40:12]

Please take a look.

[commit-bot: I haz the power, 2017-02-10 16:40:17]

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-10 17:41:59]

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

[commit-bot: I haz the power, 2017-02-10 17:42:00]

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

[marq (ping after 24h), 2017-02-13 17:06:54]

https://codereview.chromium.org/2687303003/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model.mm (left):

https://codereview.chromium.org/2687303003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model.mm:547: [tab fetchFavicon];
Any idea why [tab setParentTabModel:self] isn't called in this method? I was
wondering if we could just move the -fetchFavicon calls into there, where at
least they would be contained in the "right" place, but it looks like the
current implementation is asymmetric and only calls -setParentTabModel in
-replaceTab:withTab:

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

https://codereview.chromium.org/2687303003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_metrics_observer.mm:5: #import
"ios/chrome/browser/tabs/tab_model_metrics_observer.h"
Can you put this in ios/shared/, since it's free of dependencies, and we should
probably just reuse it?

https://codereview.chromium.org/2687303003/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model_tab_model_observers_bridge.h (right):

https://codereview.chromium.org/2687303003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_tab_model_observers_bridge.h:14: @interface
TabModelTabModelObserversBridge : NSObject<WebStateListObserver>
Can this just be TabModelObserversBridge?

https://codereview.chromium.org/2687303003/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model_tab_parenting_observer.h (right):

https://codereview.chromium.org/2687303003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_tab_parenting_observer.h:11: @interface
TabModelTabParentingObserver : NSObject<WebStateListObserver>
Just TabParentingObserver?

[sdefresne, 2017-02-13 17:13:39]

https://codereview.chromium.org/2687303003/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model.mm (left):

https://codereview.chromium.org/2687303003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model.mm:547: [tab fetchFavicon];
On 2017/02/13 17:06:54, marq wrote:
> Any idea why [tab setParentTabModel:self] isn't called in this method? I was
> wondering if we could just move the -fetchFavicon calls into there, where at
> least they would be contained in the "right" place, but it looks like the
> current implementation is asymmetric and only calls -setParentTabModel in
> -replaceTab:withTab:

Yes, this is because this method is only ever called from one of TabModel method
that pass "self" as TabModel to the Tab initialiser. Tab -setParentTabModel:
DCHECK that parentTabModel_ ivar is nil or that tabModel is nil (to allow
resetting it before deallocation). The is no way to get the parentTabModel from
Tab public API, so it is not possible to see if it is correct and only
conditionally calling -setParentTabModel:.

I was considering changing this, but I think it should be done as a separate CL
(after introducing a protocol to break the dependency cycle between Tab and
TabModel).

[sdefresne, 2017-02-13 17:25:12]

Patchset #2 (id:20001) has been deleted

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

lgtm

https://codereview.chromium.org/2687303003/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model_metrics_observer.h (right):

https://codereview.chromium.org/2687303003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_metrics_observer.h:11: @interface
TabModelMetricsObserver : NSObject<WebStateListObserver>
Could this be a C++ object?  Is there a benefit to having it in ObjC?

[marq (ping after 24h), 2017-02-14 13:35:26]

lgtm

[sdefresne, 2017-02-14 16:46:30]

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

[commit-bot: I haz the power, 2017-02-14 16:47: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-02-14 16:55:24]

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

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

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

[sdefresne, 2017-02-14 17:15:43]

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

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

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

[sdefresne, 2017-02-14 17:19:39]

PTAL

https://codereview.chromium.org/2687303003/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model_metrics_observer.h (right):

https://codereview.chromium.org/2687303003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_metrics_observer.h:11: @interface
TabModelMetricsObserver : NSObject<WebStateListObserver>
On 2017/02/14 13:29:05, rohitrao wrote:
> Could this be a C++ object?  Is there a benefit to having it in ObjC?

Done.

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

https://codereview.chromium.org/2687303003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_metrics_observer.mm:5: #import
"ios/chrome/browser/tabs/tab_model_metrics_observer.h"
On 2017/02/13 17:06:54, marq wrote:
> Can you put this in ios/shared/, since it's free of dependencies, and we
should
> probably just reuse it?

Should I rename it WebStateListMetricsObserver then?

https://codereview.chromium.org/2687303003/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model_tab_model_observers_bridge.h (right):

https://codereview.chromium.org/2687303003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_tab_model_observers_bridge.h:14: @interface
TabModelTabModelObserversBridge : NSObject<WebStateListObserver>
On 2017/02/13 17:06:54, marq wrote:
> Can this just be TabModelObserversBridge?

Done.

I've kept this object in Objective-C as it allows me to use ARC for
implementation. Changing this to C++ would require moving the __weak annotated
pointer into the header file which does not work with ARC, thus requiring the
use of __unsafe_unretained. As the class is short lived, I think using
Objective-C makes the code simpler.

https://codereview.chromium.org/2687303003/diff/1/ios/chrome/browser/tabs/tab...
File ios/chrome/browser/tabs/tab_model_tab_parenting_observer.h (right):

https://codereview.chromium.org/2687303003/diff/1/ios/chrome/browser/tabs/tab...
ios/chrome/browser/tabs/tab_model_tab_parenting_observer.h:11: @interface
TabModelTabParentingObserver : NSObject<WebStateListObserver>
On 2017/02/13 17:06:54, marq wrote:
> Just TabParentingObserver?

Done.

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

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

[commit-bot: I haz the power, 2017-02-14 19:25:23]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2017-02-17 10:42:21]

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

[commit-bot: I haz the power, 2017-02-17 10:42:35]

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

[sdefresne, 2017-02-17 10:42:52]

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

[commit-bot: I haz the power, 2017-02-17 10:43: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-02-17 12:10:06]

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

[commit-bot: I haz the power, 2017-02-17 12:10:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rohitrao (ping after 24h), 2017-02-17 12:56:00]

lgtm

[sdefresne, 2017-02-17 13:30:30]

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

[commit-bot: I haz the power, 2017-02-17 13:30:42]

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-17 15:40:29]

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

[commit-bot: I haz the power, 2017-02-17 15:40:29]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2017-02-20 09:41:23]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2017-02-20 09:41:23]

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

[commit-bot: I haz the power, 2017-02-20 09:41:42]

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-20 11:06:40]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1487583683325810,
"parent_rev": "35820a2d012cf4d04bac9a571e8052047f336095", "commit_rev":
"39bfaaba5dc0d78349ccb2a1a6c2eb42d16dca69"}

[commit-bot: I haz the power, 2017-02-20 11:07:22]

Description was changed from

==========
Convert some of TabModel logic to WebStateListObservers.

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

to

==========
Convert some of TabModel logic to WebStateListObservers.

BUG=687207

Review-Url: https://codereview.chromium.org/2687303003
Cr-Commit-Position: refs/heads/master@{#451593}
Committed:
https://chromium.googlesource.com/chromium/src/+/39bfaaba5dc0d78349ccb2a1a6c2...
==========

[commit-bot: I haz the power, 2017-02-20 11:07:23]

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/39bfaaba5dc0d78349ccb2a1a6c2...