Remove base::ObserverList<T>::Iter::GetNext().

Remove base::ObserverList<T>::Iter::GetNext().

Observer lists now support range-based for loops.

BUG=655021
R=danakj@chromium.org
TBR=marq@chromium.org,sky@chromium.org

Committed: https://crrev.com/c9dd01442c877d2f76cf750e7b52a59caba4b22b
Cr-Commit-Position: refs/heads/master@{#425549}

[dcheng, 2016-10-12 23:45:18]

dcheng@chromium.org changed reviewers:
+ danakj@chromium.org

[dcheng, 2016-10-12 23:45:18]

Note that this depends on at least 9 other CLs landing first.

There may be some cleanup in Iter possible after this, but that depends on the
semantics we want for the iterator, etc.

[danakj, 2016-10-13 19:38:13]

https://codereview.chromium.org/2419673003/diff/1/base/observer_list_unittest.cc
File base/observer_list_unittest.cc (left):

https://codereview.chromium.org/2419673003/diff/1/base/observer_list_unittest...
base/observer_list_unittest.cc:898: TEST(ObserverListTest,
AddObserverInTheLastObserve) {
Why kill the test?

[loyso (OOO), 2016-10-13 23:33:51]

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

[loyso (OOO), 2016-10-13 23:33:52]

https://codereview.chromium.org/2419673003/diff/1/base/observer_list_unittest.cc
File base/observer_list_unittest.cc (left):

https://codereview.chromium.org/2419673003/diff/1/base/observer_list_unittest...
base/observer_list_unittest.cc:898: TEST(ObserverListTest,
AddObserverInTheLastObserve) {
On 2016/10/13 19:38:13, danakj wrote:
> Why kill the test?
+1. Even without GetNext a user is able to write GetNext-like loops:

auto it = observers.begin();
auto end = observers.end();
do {
  Observer& observer = *it;
  it++;
  observer.Observe();
} while(it != end);

for(auto it = observers.begin(), end = observers.end(); it != end;) {
  Observer& observer = *it;
  it++;
  observer.Observe();  
}

[loyso (OOO), 2016-10-13 23:38:10]

^ ++it; // everywhere, of course

[loyso (OOO), 2016-10-13 23:40:14]

One more correction:

auto it = observers.begin();
auto end = observers.end();
while(it != end) {
  Observer& observer = *it;
  it++;
  observer.Observe();
}

[loyso (OOO), 2016-10-13 23:45:06]

On 2016/10/13 23:40:14, loyso wrote:
> auto it = observers.begin();
> auto end = observers.end();
> while(it != end) {
>   Observer& observer = *it;
>   it++;
>   observer.Observe();
> }
It looks like operator* and operator-> needs EnsureValidIndex if we support such
loops.
If we don't, how can we forbid them?

[dcheng, 2016-10-14 21:32:22]

On 2016/10/13 23:45:06, loyso wrote:
> On 2016/10/13 23:40:14, loyso wrote:
> > auto it = observers.begin();
> > auto end = observers.end();
> > while(it != end) {
> >   Observer& observer = *it;
> >   it++;
> >   observer.Observe();
> > }

Personally, I wouldn't see it as a terrible thing that code written like this
doesn't work 

> It looks like operator* and operator-> needs EnsureValidIndex if we support
such
> loops.
> If we don't, how can we forbid them?

Easy: by making it not work =P

However, I restored the test. I was hoping to simplify the semantics: a simpler
iterator comparator would support the range-based for loop case.

After thinking about this more though, I think keeping these semantics makes
more sense: the whole point of ObserverList's iterators is to provide iterators
that are robust against list mutation. Meh.

[danakj, 2016-10-14 21:52:10]

LGTM

https://codereview.chromium.org/2419673003/diff/40001/base/observer_list_unit...
File base/observer_list_unittest.cc (right):

https://codereview.chromium.org/2419673003/diff/40001/base/observer_list_unit...
base/observer_list_unittest.cc:911: ++it;
Leave a comment explaining why youre doing this ++ before the Observe

[dcheng, 2016-10-14 23:31:37]

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

[commit-bot: I haz the power, 2016-10-14 23:31:58]

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, 2016-10-15 00:11:29]

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

[commit-bot: I haz the power, 2016-10-15 00:11:29]

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

[dcheng, 2016-10-15 00:39:59]

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

[commit-bot: I haz the power, 2016-10-15 00:40:22]

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

[dcheng, 2016-10-15 00:42:46]

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

[commit-bot: I haz the power, 2016-10-15 00:43:07]

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, 2016-10-15 01:59:18]

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

[commit-bot: I haz the power, 2016-10-15 01:59:19]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-10-15 05:38:02]

Description was changed from

==========
Remove base::ObserverList<T>::Iter::GetNext().

Observer lists now support range-based for loops.

BUG=655021
==========

to

==========
Remove base::ObserverList<T>::Iter::GetNext().

Observer lists now support range-based for loops.

BUG=655021
R=danakj@chromium.org
TBR=marq@chromium.org,sky@chromium.org
==========

[dcheng, 2016-10-15 05:38:02]

dcheng@chromium.org changed reviewers:
+ marq@chromium.org, sky@chromium.org

[dcheng, 2016-10-15 05:38:20]

TBRing //ios and //ui OWNERS (danakj@ reviewed)

[dcheng, 2016-10-15 05:38:30]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-10-15 05:38:30]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2419673003/#ps100001
(title: "ios fix too")

[commit-bot: I haz the power, 2016-10-15 05:38:38]

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, 2016-10-15 05:43:41]

Description was changed from

==========
Remove base::ObserverList<T>::Iter::GetNext().

Observer lists now support range-based for loops.

BUG=655021
R=danakj@chromium.org
TBR=marq@chromium.org,sky@chromium.org
==========

to

==========
Remove base::ObserverList<T>::Iter::GetNext().

Observer lists now support range-based for loops.

BUG=655021
R=danakj@chromium.org
TBR=marq@chromium.org,sky@chromium.org
==========

[commit-bot: I haz the power, 2016-10-15 05:43:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-15 05:46:13]

Description was changed from

==========
Remove base::ObserverList<T>::Iter::GetNext().

Observer lists now support range-based for loops.

BUG=655021
R=danakj@chromium.org
TBR=marq@chromium.org,sky@chromium.org
==========

to

==========
Remove base::ObserverList<T>::Iter::GetNext().

Observer lists now support range-based for loops.

BUG=655021
R=danakj@chromium.org
TBR=marq@chromium.org,sky@chromium.org

Committed: https://crrev.com/c9dd01442c877d2f76cf750e7b52a59caba4b22b
Cr-Commit-Position: refs/heads/master@{#425549}
==========

[commit-bot: I haz the power, 2016-10-15 05:46:14]

Patchset 6 (id:??) landed as
https://crrev.com/c9dd01442c877d2f76cf750e7b52a59caba4b22b
Cr-Commit-Position: refs/heads/master@{#425549}

[loyso (OOO), 2016-10-16 23:20:17]

https://codereview.chromium.org/2419673003/diff/100001/base/observer_list_uni...
File base/observer_list_unittest.cc (right):

https://codereview.chromium.org/2419673003/diff/100001/base/observer_list_uni...
base/observer_list_unittest.cc:909: while (it != observer_list.end()) {
nit: you recreate the end iterator many times here (the opposite is to call it
once as in the range-based for loop). It may be a different test :)

[loyso (OOO), 2016-10-16 23:32:47]

On 2016/10/14 21:32:22, dcheng wrote:
> On 2016/10/13 23:45:06, loyso wrote:
> > On 2016/10/13 23:40:14, loyso wrote:
> > > auto it = observers.begin();
> > > auto end = observers.end();
> > > while(it != end) {
> > >   Observer& observer = *it;
> > >   it++;
> > >   observer.Observe();
> > > }
> 
> Personally, I wouldn't see it as a terrible thing that code written like this
> doesn't work 
> 
> > It looks like operator* and operator-> needs EnsureValidIndex if we support
> such
> > loops.
> > If we don't, how can we forbid them?
> 
> Easy: by making it not work =P
Such a crash would be a generic runtime DCHECK failure. We could mention in the
observer_list.h comment, which loop forms we don't support. (Plus a couple of
unit tests, which check that GetCurrent would return nullptr in a specific
situation)