Use a scoped WebStateListObserverBridge.

Use a scoped WebStateListObserverBridge.

BUG=none
R=rohitrao@chromium.org
CC=sdefresne@chromium.org

Review-Url: https://codereview.chromium.org/2820493002
Cr-Commit-Position: refs/heads/master@{#466321}
Committed: https://chromium.googlesource.com/chromium/src/+/8041d6145bbd1f92ff8d5dea5264be9e59d8fc3e

[lpromero, 2017-04-13 09:46:51]

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

[commit-bot: I haz the power, 2017-04-13 09:47:13]

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-04-13 10:57:12]

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

[commit-bot: I haz the power, 2017-04-13 10:57:13]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2017-04-18 10:06:32]

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

[sdefresne, 2017-04-18 10:06:33]

I don't think we should go this way as Observers are implementing a M:N
relationship (except for WebStateObserver that has a weird API).

For serialisation in the new architecture, I have a class that requires this M:N
relationship (as it needs to observe event from all WebStateLists corresponding
to a single BrowserList).

[lpromero, 2017-04-19 20:32:43]

Description was changed from

==========
Migrate to the RAII API of WebStateListObserver.

BUG=none
R=rohitrao@chromium.org
CC=sdefresne@chromium.org
==========

to

==========
Use a scoped WebStateListObserverBridge.

BUG=none
R=rohitrao@chromium.org
CC=sdefresne@chromium.org
==========

[lpromero, 2017-04-19 21:59:47]

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

[commit-bot: I haz the power, 2017-04-19 22:00:33]

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

[lpromero, 2017-04-19 22:00:42]

Updated.

[commit-bot: I haz the power, 2017-04-20 00:19:56]

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

[commit-bot: I haz the power, 2017-04-20 00:19:56]

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

[sdefresne, 2017-04-20 10:02:07]

https://codereview.chromium.org/2820493002/diff/20001/ios/chrome/browser/web_...
File ios/chrome/browser/web_state_list/web_state_list_fast_enumeration_helper.mm
(left):

https://codereview.chromium.org/2820493002/diff/20001/ios/chrome/browser/web_...
ios/chrome/browser/web_state_list/web_state_list_fast_enumeration_helper.mm:59:
_webStateList->RemoveObserver(_observerBridge.get());
This is incorrect, the registration needs to happens when the -shutdown method
is called. The correct solution with a ScopedObserver would be to delete the
ScopedObserver at that point, but I think it does not bring enough value since
this class need to explicitly control when registration/unregistration happen
anyway.

I would suggest reverting this file and leaving it to use manual
AddObserver/RemoveObserver calls.

BTW, this is because of this that all tests are failing.

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

https://codereview.chromium.org/2820493002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:40:
_scopedWebStateListObserver.reset();
You can use RemoveAll/Add to avoid destroying the observer and ScopedObserver
everytime.

- (instancetype)init {
  self = [super init];
  if (self) {
    _webStateListObserver = base::MakeUnique<WebStateListObserverBridge>(self);
    _scopedWebStateListObserver = base::MakeUnique<
        ScopedObserver<WebStateList, WebStateListObserverBridge>>(
        _webStateListObserver.get());
  }
}

- (void)setWebStateList:(WebStateList*)webStateList {
  _scopedWebStateListObserver->RemoveAll();
  _webStateList = webStateList;
  if (!_webStateList)
    return;
  _scopedWebStateListObserver->Add(_webStateList);
}

https://codereview.chromium.org/2820493002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm (right):

https://codereview.chromium.org/2820493002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm:50:
_scopedWebStateListObserver.reset();
ditto, use RemoveAll/Add to avoid creating destroying the observer and
ScopedObserver everytime

https://codereview.chromium.org/2820493002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm:51:
_webStateListObserver.reset();
Why don't we call "[self
disableWebUsage:self.webStateList->GetActiveWebState()];" here?

[lpromero, 2017-04-20 16:25:07]

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

[lpromero, 2017-04-20 16:25:14]

https://codereview.chromium.org/2820493002/diff/20001/ios/chrome/browser/web_...
File ios/chrome/browser/web_state_list/web_state_list_fast_enumeration_helper.mm
(left):

https://codereview.chromium.org/2820493002/diff/20001/ios/chrome/browser/web_...
ios/chrome/browser/web_state_list/web_state_list_fast_enumeration_helper.mm:59:
_webStateList->RemoveObserver(_observerBridge.get());
On 2017/04/20 10:02:07, sdefresne wrote:
> This is incorrect, the registration needs to happens when the -shutdown method
> is called. The correct solution with a ScopedObserver would be to delete the
> ScopedObserver at that point, but I think it does not bring enough value since
> this class need to explicitly control when registration/unregistration happen
> anyway.
> 
> I would suggest reverting this file and leaving it to use manual
> AddObserver/RemoveObserver calls.
> 
> BTW, this is because of this that all tests are failing.

Done.

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

https://codereview.chromium.org/2820493002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:40:
_scopedWebStateListObserver.reset();
On 2017/04/20 10:02:07, sdefresne wrote:
> You can use RemoveAll/Add to avoid destroying the observer and ScopedObserver
> everytime.
> 
> - (instancetype)init {
>   self = [super init];
>   if (self) {
>     _webStateListObserver =
base::MakeUnique<WebStateListObserverBridge>(self);
>     _scopedWebStateListObserver = base::MakeUnique<
>         ScopedObserver<WebStateList, WebStateListObserverBridge>>(
>         _webStateListObserver.get());
>   }
> }
> 
> - (void)setWebStateList:(WebStateList*)webStateList {
>   _scopedWebStateListObserver->RemoveAll();
>   _webStateList = webStateList;
>   if (!_webStateList)
>     return;
>   _scopedWebStateListObserver->Add(_webStateList);
> }

Done, although could we imagine a time when RemoveAll removes too much?

https://codereview.chromium.org/2820493002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm (right):

https://codereview.chromium.org/2820493002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm:50:
_scopedWebStateListObserver.reset();
On 2017/04/20 10:02:07, sdefresne wrote:
> ditto, use RemoveAll/Add to avoid creating destroying the observer and
> ScopedObserver everytime

Done.

https://codereview.chromium.org/2820493002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm:51:
_webStateListObserver.reset();
On 2017/04/20 10:02:07, sdefresne wrote:
> Why don't we call "[self
> disableWebUsage:self.webStateList->GetActiveWebState()];" here?

No idea. I filed http://crbug.com/713757.

[commit-bot: I haz the power, 2017-04-20 16:25: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-04-20 16:36:07]

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

[commit-bot: I haz the power, 2017-04-20 16:36:08]

Dry run: This issue passed the CQ dry run.

[lpromero, 2017-04-21 09:11:52]

PTAL. Thanks!

[sdefresne, 2017-04-21 13:30:07]

On 2017/04/20 16:25:14, lpromero wrote:
>
https://codereview.chromium.org/2820493002/diff/20001/ios/chrome/browser/web_...
> File
ios/chrome/browser/web_state_list/web_state_list_fast_enumeration_helper.mm
> (left):
> 
>
https://codereview.chromium.org/2820493002/diff/20001/ios/chrome/browser/web_...
>
ios/chrome/browser/web_state_list/web_state_list_fast_enumeration_helper.mm:59:
> _webStateList->RemoveObserver(_observerBridge.get());
> On 2017/04/20 10:02:07, sdefresne wrote:
> > This is incorrect, the registration needs to happens when the -shutdown
method
> > is called. The correct solution with a ScopedObserver would be to delete the
> > ScopedObserver at that point, but I think it does not bring enough value
since
> > this class need to explicitly control when registration/unregistration
happen
> > anyway.
> > 
> > I would suggest reverting this file and leaving it to use manual
> > AddObserver/RemoveObserver calls.
> > 
> > BTW, this is because of this that all tests are failing.
> 
> Done.
> 
>
https://codereview.chromium.org/2820493002/diff/20001/ios/clean/chrome/browse...
> File ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm
> (right):
> 
>
https://codereview.chromium.org/2820493002/diff/20001/ios/clean/chrome/browse...
> ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator.mm:40:
> _scopedWebStateListObserver.reset();
> On 2017/04/20 10:02:07, sdefresne wrote:
> > You can use RemoveAll/Add to avoid destroying the observer and
ScopedObserver
> > everytime.
> > 
> > - (instancetype)init {
> >   self = [super init];
> >   if (self) {
> >     _webStateListObserver =
> base::MakeUnique<WebStateListObserverBridge>(self);
> >     _scopedWebStateListObserver = base::MakeUnique<
> >         ScopedObserver<WebStateList, WebStateListObserverBridge>>(
> >         _webStateListObserver.get());
> >   }
> > }
> > 
> > - (void)setWebStateList:(WebStateList*)webStateList {
> >   _scopedWebStateListObserver->RemoveAll();
> >   _webStateList = webStateList;
> >   if (!_webStateList)
> >     return;
> >   _scopedWebStateListObserver->Add(_webStateList);
> > }
> 
> Done, although could we imagine a time when RemoveAll removes too much?

RemoveAll calls RemoveObserver for all observed Source. So I can't think of a
time it would do too much work. It may do unnecessary work if you do the
following:

  TabCollectionMediator* t = ...;
  t.webStateList = ...; // some non null WebStateList
  t.webStateList = t.webStateList;

This can be fixed by adding a check at the beginning of -setWebStateList:

- (void)setWebStateList:(WebStateList*)webStateList {
  if (_webStateList == webStateList)
    return;
  ...

>
https://codereview.chromium.org/2820493002/diff/20001/ios/clean/chrome/browse...
> File ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm
(right):
> 
>
https://codereview.chromium.org/2820493002/diff/20001/ios/clean/chrome/browse...
> ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm:50:
> _scopedWebStateListObserver.reset();
> On 2017/04/20 10:02:07, sdefresne wrote:
> > ditto, use RemoveAll/Add to avoid creating destroying the observer and
> > ScopedObserver everytime
> 
> Done.
> 
>
https://codereview.chromium.org/2820493002/diff/20001/ios/clean/chrome/browse...
> ios/clean/chrome/browser/ui/web_contents/web_contents_mediator.mm:51:
> _webStateListObserver.reset();
> On 2017/04/20 10:02:07, sdefresne wrote:
> > Why don't we call "[self
> > disableWebUsage:self.webStateList->GetActiveWebState()];" here?
> 
> No idea. I filed http://crbug.com/713757.

[sdefresne, 2017-04-21 13:30:12]

lgtm

[lpromero, 2017-04-21 13:35:51]

The CQ bit was checked by lpromero@chromium.org

[commit-bot: I haz the power, 2017-04-21 13:36:09]

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-04-21 13:40:04]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492781751862980,
"parent_rev": "a7629595cadb7b48331e9ae449d715693ac8e385", "commit_rev":
"8041d6145bbd1f92ff8d5dea5264be9e59d8fc3e"}

[commit-bot: I haz the power, 2017-04-21 13:40:16]

Description was changed from

==========
Use a scoped WebStateListObserverBridge.

BUG=none
R=rohitrao@chromium.org
CC=sdefresne@chromium.org
==========

to

==========
Use a scoped WebStateListObserverBridge.

BUG=none
R=rohitrao@chromium.org
CC=sdefresne@chromium.org

Review-Url: https://codereview.chromium.org/2820493002
Cr-Commit-Position: refs/heads/master@{#466321}
Committed:
https://chromium.googlesource.com/chromium/src/+/8041d6145bbd1f92ff8d5dea5264...
==========

[commit-bot: I haz the power, 2017-04-21 13:40:18]

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