[clean ios] Fix TabCollectionMediator disconnect.

[clean ios] Fix TabCollectionMediator disconnect.

TabCollectionMediator -disconnect wasn't removing observers, and was
spuriously resetting the observer (the scoped observer takes care of
that).

BUG=

Review-Url: https://codereview.chromium.org/2936563002
Cr-Commit-Position: refs/heads/master@{#479689}
Committed: https://chromium.googlesource.com/chromium/src/+/067285ecc6e5d1cc8fb16046274727d3b58123c3

[marq (ping after 24h), 2017-06-12 09:36:48]

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

[sdefresne, 2017-06-12 09:48:07]

https://codereview.chromium.org/2936563002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm
(right):

https://codereview.chromium.org/2936563002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:52:
_scopedWebStateListObserver->RemoveAll();
I don't understand the comment that ScopedObserver takes care of clearing
_webStateObserver.

_webStateObserver is std::unique_ptr<web::WebStateObserverBridge> while
_scopedWebStateListObserver is std::unique_ptr<ScopedObserver<WebStateList,
WebStateListObserverBridge>>.

WebStateListObserver != WebStateObserver so they are not related. So you should
remove the comment from the description.

Regarding whether the .reset() call is required or not, I think it should be
present because it unregister the _webStateObserver from WebState at a
known-time instead of doing it when the autoreleasepool is clearer. I think we
should restore the .reset().

[marq (ping after 24h), 2017-06-13 08:18:53]

https://codereview.chromium.org/2936563002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm
(right):

https://codereview.chromium.org/2936563002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:52:
_scopedWebStateListObserver->RemoveAll();
On 2017/06/12 09:48:07, sdefresne wrote:
> I don't understand the comment that ScopedObserver takes care of clearing
> _webStateObserver.
> 
> _webStateObserver is std::unique_ptr<web::WebStateObserverBridge> while
> _scopedWebStateListObserver is std::unique_ptr<ScopedObserver<WebStateList,
> WebStateListObserverBridge>>.
> 
> WebStateListObserver != WebStateObserver so they are not related. So you
should
> remove the comment from the description.
> 
> Regarding whether the .reset() call is required or not, I think it should be
> present because it unregister the _webStateObserver from WebState at a
> known-time instead of doing it when the autoreleasepool is clearer. I think we
> should restore the .reset().

Yeah, brain fart on my part.

Does it make sense to be using a scoped observer for the WSL observer, but not
for the WebState observer? Shouldn't both be done the same way?

[sdefresne, 2017-06-13 09:00:27]

lgtm

https://codereview.chromium.org/2936563002/diff/1/ios/clean/chrome/browser/ui...
File ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm
(right):

https://codereview.chromium.org/2936563002/diff/1/ios/clean/chrome/browser/ui...
ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:52:
_scopedWebStateListObserver->RemoveAll();
On 2017/06/13 08:18:53, marq (ping after 24h) wrote:
> On 2017/06/12 09:48:07, sdefresne wrote:
> > I don't understand the comment that ScopedObserver takes care of clearing
> > _webStateObserver.
> > 
> > _webStateObserver is std::unique_ptr<web::WebStateObserverBridge> while
> > _scopedWebStateListObserver is std::unique_ptr<ScopedObserver<WebStateList,
> > WebStateListObserverBridge>>.
> > 
> > WebStateListObserver != WebStateObserver so they are not related. So you
> should
> > remove the comment from the description.
> > 
> > Regarding whether the .reset() call is required or not, I think it should be
> > present because it unregister the _webStateObserver from WebState at a
> > known-time instead of doing it when the autoreleasepool is clearer. I think
we
> > should restore the .reset().
> 
> Yeah, brain fart on my part.
> 
> Does it make sense to be using a scoped observer for the WSL observer, but not
> for the WebState observer? Shouldn't both be done the same way?

WebStateObserver is a weird observer that auto-unregister itself in its
destructor (this is one of the rare observer that do it this way).

See web_state_observer.mm:

  WebStateObserver::~WebStateObserver() {
    if (web_state_)
      web_state_->RemoveObserver(this);
  }

It is also one of the only observer that can only observe a single WebState (so
if you want to observe multiple WebStates at the same time you need to use
multiple WebStateObserver, where as with other observers you can observe
multiple instance with the same observer).

[marq (ping after 24h), 2017-06-13 11:59:37]

Restore observer reset.

[sdefresne, 2017-06-15 13:48:19]

lgtm

[marq (ping after 24h), 2017-06-15 13:48:44]

The CQ bit was checked by marq@chromium.org

[commit-bot: I haz the power, 2017-06-15 13:48:57]

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-06-15 13:58:59]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497534524556530,
"parent_rev": "cd5c1814c53b4cb55a173224a640e34eefe721ca", "commit_rev":
"067285ecc6e5d1cc8fb16046274727d3b58123c3"}

[commit-bot: I haz the power, 2017-06-15 13:59:13]

Description was changed from

==========
[clean ios] Fix TabCollectionMediator disconnect.

TabCollectionMediator -disconnect wasn't removing observers, and was
spuriously resetting the observer (the scoped observer takes care of
that).

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

to

==========
[clean ios] Fix TabCollectionMediator disconnect.

TabCollectionMediator -disconnect wasn't removing observers, and was
spuriously resetting the observer (the scoped observer takes care of
that).

BUG=

Review-Url: https://codereview.chromium.org/2936563002
Cr-Commit-Position: refs/heads/master@{#479689}
Committed:
https://chromium.googlesource.com/chromium/src/+/067285ecc6e5d1cc8fb160462747...
==========

[commit-bot: I haz the power, 2017-06-15 13:59:15]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/067285ecc6e5d1cc8fb160462747...