Base ObserverList: Add basic support for standard C++ iterators.

Base ObserverList: Add basic support for standard C++ iterators.

Also add a support for const_iterator.

The range-based for loop support follows for free.

See the use case in the dependent CL.

BUG=634916
Committed: https://crrev.com/29025b60361ec76cf02bab720703c66ae8047be9
Cr-Commit-Position: refs/heads/master@{#424381}

[loyso (OOO), 2016-09-15 07:48:26]

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

[loyso (OOO), 2016-09-15 07:48:35]

Description was changed from

==========
Base ObserverList: Add basic support for standadrd C++ iterators.

See the use case in the dependent CL.

BUG=634916
==========

to

==========
Base ObserverList: Add basic support for standadrd C++ iterators.

See the use case in the dependent CL.

BUG=634916
==========

[loyso (OOO), 2016-09-15 07:48:35]

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

[commit-bot: I haz the power, 2016-09-15 07:48:39]

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

[loyso (OOO), 2016-09-15 07:56:28]

Description was changed from

==========
Base ObserverList: Add basic support for standadrd C++ iterators.

See the use case in the dependent CL.

BUG=634916
==========

to

==========
Base ObserverList: Add basic support for standard C++ iterators.

Also add a support for const_iterator.

The range-based for loop support follows for free. 

See the use case in the dependent CL.

BUG=634916
==========

[commit-bot: I haz the power, 2016-09-15 08:07:24]

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

[commit-bot: I haz the power, 2016-09-15 08:07:25]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[loyso (OOO), 2016-09-15 08:07:59]

Do not review please - to be fixed.

[loyso (OOO), 2016-09-16 00:45:26]

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

[commit-bot: I haz the power, 2016-09-16 00:46:04]

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-09-16 02:24:17]

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

[commit-bot: I haz the power, 2016-09-16 02:24:19]

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

[loyso (OOO), 2016-09-16 03:11:09]

PTAL! It's ready to be landed.

[loyso (OOO), 2016-09-16 03:31:49]

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

[dcheng, 2016-09-16 21:44:17]

I haven't reviewed the entire patch yet, but can we completely remove the old
FOR_EACH_OBSERVER macro? Will someone sign up for doing that, or will we have
n+1 ways of doing this for eternity =)

https://codereview.chromium.org/2340583005/diff/40001/base/observer_list.h
File base/observer_list.h (right):

https://codereview.chromium.org/2340583005/diff/40001/base/observer_list.h#ne...
base/observer_list.h:114: ThisType operator++(int);
Do we need to provide both prefix and postfix versions? It'd be nice to only
implement the prefix one.

[loyso (OOO), 2016-09-19 00:09:06]

On 2016/09/16 21:44:17, dcheng wrote:
> I haven't reviewed the entire patch yet, but can we completely remove the old
> FOR_EACH_OBSERVER macro? Will someone sign up for doing that, or will we have
> n+1 ways of doing this for eternity =)
Yes, we can:
- declare it as a deprecated
- send a PSA so people can migrate
- I could clean-up the leftovers

> https://codereview.chromium.org/2340583005/diff/40001/base/observer_list.h
> File base/observer_list.h (right):
>
https://codereview.chromium.org/2340583005/diff/40001/base/observer_list.h#ne...
> base/observer_list.h:114: ThisType operator++(int);
> Do we need to provide both prefix and postfix versions? It'd be nice to only
> implement the prefix one.
Ackd. Will upload the Patch Set #4.

[loyso (OOO), 2016-09-19 00:38:04]

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

[commit-bot: I haz the power, 2016-09-19 00:38:14]

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-09-19 01:59:20]

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

[commit-bot: I haz the power, 2016-09-19 01:59:21]

Dry run: This issue passed the CQ dry run.

[loyso (OOO), 2016-09-19 02:43:28]

PTAL! My CLs are blocked on this change.

[loyso (OOO), 2016-09-20 00:19:00]

Anyone, please? Feel free to reassign if you are busy.

[dcheng, 2016-09-20 00:36:36]

On 2016/09/20 00:19:00, loyso wrote:
> Anyone, please? Feel free to reassign if you are busy.

I'm planning on reviewing this again, but I think it would be better to not
couple CLs that don't strictly require this change to this CL (as this CL looks
more like a clean-up / improvement CL)

[loyso (OOO), 2016-09-23 00:01:07]

loyso@chromium.org changed reviewers:
- thakis@chromium.org

[loyso (OOO), 2016-09-23 00:01:23]

Friendly ping.

[dcheng, 2016-09-23 07:39:02]

I think my largest concern is whether it's important to keep the
might_have_observers() short-circuiting logic. This was added in
https://bugs.chromium.org/p/chromium/issues/detail?id=100121, and looks like a
reasonable optimization to have. With the for-each loop, we lose that though.
Have you done any measurements to verify that this has no perf impact?

For example, the MessageLoop can run observers before and after dispatching a
task. Is it the common case to have task observers attached now?

https://codereview.chromium.org/2340583005/diff/60001/base/observer_list.h
File base/observer_list.h (right):

https://codereview.chromium.org/2340583005/diff/60001/base/observer_list.h#ne...
base/observer_list.h:112: size_t checked_max_index() const {
Nit: checked here makes it sound like it will CHECK or crash or something if
there's an index out of bounds, but that's not really the case here.

https://codereview.chromium.org/2340583005/diff/60001/base/observer_list.h#ne...
base/observer_list.h:211: return GetCurrent();
If operator* DCHECKs() this, operator-> probably should too

[loyso (OOO), 2016-09-26 04:26:15]

On 2016/09/23 07:39:02, dcheng wrote:
> I think my largest concern is whether it's important to keep the
> might_have_observers() short-circuiting logic. This was added in
> https://bugs.chromium.org/p/chromium/issues/detail?id=100121, and looks like a
> reasonable optimization to have. With the for-each loop, we lose that though.
> Have you done any measurements to verify that this has no perf impact?
> 
> For example, the MessageLoop can run observers before and after dispatching a
> task. Is it the common case to have task observers attached now?
Done.

[loyso (OOO), 2016-09-26 04:26:24]

https://codereview.chromium.org/2340583005/diff/60001/base/observer_list.h
File base/observer_list.h (right):

https://codereview.chromium.org/2340583005/diff/60001/base/observer_list.h#ne...
base/observer_list.h:112: size_t checked_max_index() const {
On 2016/09/23 07:39:01, dcheng wrote:
> Nit: checked here makes it sound like it will CHECK or crash or something if
> there's an index out of bounds, but that's not really the case here.

Done.

https://codereview.chromium.org/2340583005/diff/60001/base/observer_list.h#ne...
base/observer_list.h:211: return GetCurrent();
On 2016/09/23 07:39:01, dcheng wrote:
> If operator* DCHECKs() this, operator-> probably should too

Done.

[loyso (OOO), 2016-09-26 04:26:48]

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

[commit-bot: I haz the power, 2016-09-26 04:26:57]

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-09-26 06:17:11]

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

[commit-bot: I haz the power, 2016-09-26 06:17:12]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[loyso (OOO), 2016-09-26 08:14:39]

In progress. MSVC is special (as usual).

[loyso (OOO), 2016-09-27 04:45:24]

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

[commit-bot: I haz the power, 2016-09-27 04:45:48]

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

[loyso (OOO), 2016-09-27 06:21:23]

All done. PTAL!

[commit-bot: I haz the power, 2016-09-27 06:22:23]

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

[commit-bot: I haz the power, 2016-09-27 06:22:24]

Dry run: This issue passed the CQ dry run.

[loyso (OOO), 2016-09-28 00:02:44]

dcheng@, ping?

[loyso (OOO), 2016-09-29 06:34:34]

dcheng@, friendly ping again. It's ready to be landed.

[dcheng, 2016-10-03 07:29:18]

Also, which benchmarking these things is notoriously hard, I think it'd be worth
trying to cook up some benchmarks just to make sure there's not much of an
impact.

(One example of how to write a perftest can be seen in
https://codereview.chromium.org/1874403003/)

https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h
File base/observer_list.h (right):

https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n...
base/observer_list.h:88: class Iter {
Derive from std::iterator.

https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n...
base/observer_list.h:180: : index_(), max_index_() {}
Nit: write 0 explicitly here forconsistency.

https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n...
base/observer_list.h:349: if ((observer_list).might_have_observers()) {         
              \
Can we structure the macro so we can use the new ranged-based for loop syntax
here too? That might simplify the iterator definition itself, since we won't
need to break out helpers to support the slightly different semantics of each
iterator.

[loyso (OOO), 2016-10-04 03:46:57]

https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h
File base/observer_list.h (right):

https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n...
base/observer_list.h:88: class Iter {
On 2016/10/03 07:29:18, dcheng wrote:
> Derive from std::iterator.
This comment conflicts with your previous comment "to delete the post fix ++
operator" (the one of InputIterator requirements).
Your comment on "might_have_observers" optimization conflicts with
ForwardIterator requirements.
Please, clarify those ambiguities.

Disclaimer: This CL is about very basic support of the C++ |for| loops. Iter
isn't a C++ iterator in its strict C++ sense.

https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n...
base/observer_list.h:180: : index_(), max_index_() {}
On 2016/10/03 07:29:18, dcheng wrote:
> Nit: write 0 explicitly here forconsistency.

Done.

https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n...
base/observer_list.h:349: if ((observer_list).might_have_observers()) {         
              \
On 2016/10/03 07:29:18, dcheng wrote:
> Can we structure the macro so we can use the new ranged-based for loop syntax
> here too? That might simplify the iterator definition itself, since we won't
> need to break out helpers to support the slightly different semantics of each
> iterator.
No, we can't. Some places use GetNext method and Iter construction explicitly,
bypassing this macro.

[loyso (OOO), 2016-10-04 04:08:16]

On 2016/10/03 07:29:18, dcheng wrote:
> Also, which benchmarking these things is notoriously hard, I think it'd be
worth
> trying to cook up some benchmarks just to make sure there's not much of an
> impact.
> 
> (One example of how to write a perftest can be seen in
> https://codereview.chromium.org/1874403003/)
In progress.

[loyso (OOO), 2016-10-04 04:12:02]

https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h
File base/observer_list.h (right):

https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n...
base/observer_list.h:88: class Iter {
Also, please notice that ObserverList isn't a C++ container.

[dcheng, 2016-10-04 23:53:40]

https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h
File base/observer_list.h (right):

https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n...
base/observer_list.h:88: class Iter {
On 2016/10/04 04:12:02, loyso wrote:
> Also, please notice that ObserverList isn't a C++ container.

As discussed offline, this does work (e.g.
https://cs.chromium.org/chromium/src/content/browser/frame_host/frame_tree.h?...)
but it's fine to leave it out, as it's generally easier to add than remove.

https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n...
base/observer_list.h:349: if ((observer_list).might_have_observers()) {         
              \
On 2016/10/04 03:46:57, loyso wrote:
> On 2016/10/03 07:29:18, dcheng wrote:
> > Can we structure the macro so we can use the new ranged-based for loop
syntax
> > here too? That might simplify the iterator definition itself, since we won't
> > need to break out helpers to support the slightly different semantics of
each
> > iterator.
> No, we can't. Some places use GetNext method and Iter construction explicitly,
> bypassing this macro.

As discussed offline, let's switch the macro to use it and followup with
incremental CLs to remove GetNext()

[loyso (OOO), 2016-10-05 04:11:03]

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

[commit-bot: I haz the power, 2016-10-05 04:11: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, 2016-10-05 04:29:23]

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

[commit-bot: I haz the power, 2016-10-05 04:29:24]

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

[loyso (OOO), 2016-10-05 04:31:37]

On 2016/10/04 23:53:40, dcheng wrote:
> On 2016/10/04 04:12:02, loyso wrote:
> > Also, please notice that ObserverList isn't a C++ container.
> As discussed offline, this does work (e.g.
>
https://cs.chromium.org/chromium/src/content/browser/frame_host/frame_tree.h?...)
Obviously, it doesn't. It doesn't even attempt to instantiate any template
algorithm beyond begin/end calls. It compiles, of course.

IMHO, saying that "FrameTree::NodeIterator is-a std::forward_iterator" and
violating
http://en.cppreference.com/w/cpp/concept/InputIterator
http://en.cppreference.com/w/cpp/concept/ForwardIterator
concepts is misleading.

Moreover, since NodeRange violates
http://en.cppreference.com/w/cpp/concept/Container requirements, the idea of
NodeRange's iterator is unclear.

> but it's fine to leave it out, as it's generally easier to add than remove.
Ackd.

[loyso (OOO), 2016-10-05 05:02:29]

On 2016/10/04 23:53:40, dcheng wrote:
> As discussed offline, this does work (e.g.
p.s. That NodeIterator sub-classing will produce an error one day in C++1z with
TS 19217:2015 adopted.
The compiler will be able to verify all the requirements for the operations.

[loyso (OOO), 2016-10-05 05:09:01]

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

[commit-bot: I haz the power, 2016-10-05 05:09:20]

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-05 07:08:24]

https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h
File base/observer_list.h (right):

https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h#n...
base/observer_list.h:122: size_t index_;
Let's add a comment to document the invariants for |index_|:

Something like:

When initially constructed and each time the iterator is incremented,
|index_| is guaranteed to point to a non-null index if the iterator
has not reached the end of the ObserverList.

https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h#n...
base/observer_list.h:192: index_(0),
Then to guarantee the invariant, initialize this to GetNonNullIndex()

https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h#n...
base/observer_list.h:209: return GetNonNullCurrent() !=
other.GetNonNullCurrent();
Hmm.

Given this observer list:

{ nullptr, nullptr, &foo, &bar }

If iterator A has index_ == 0, and iterator B has index_ == 1, they'll compare
equal. In some sense they are... and in some sense they aren't.

Perhaps this check could be:

return list_ == other_.list && index_ == other_.index_;

https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h#n...
base/observer_list.h:277: ObserverType* current = GetNonNullCurrent();
With the previously mentioned invariant in place, would it be possible to
simplify this to:

ObserverType* current = GetCurrent();
++(*this);
return current;

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

https://codereview.chromium.org/2340583005/diff/260001/base/observer_list_uni...
base/observer_list_unittest.cc:224: TEST(ObserverListTest, DesruptSelf) {
Nit: Desrupt -> Disrupt

[commit-bot: I haz the power, 2016-10-05 07:10:21]

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

[commit-bot: I haz the power, 2016-10-05 07:10:22]

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

[loyso (OOO), 2016-10-05 07:24:42]

https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h
File base/observer_list.h (right):

https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n...
base/observer_list.h:349: if ((observer_list).might_have_observers()) {         
              \
On 2016/10/04 23:53:39, dcheng wrote:
> On 2016/10/04 03:46:57, loyso wrote:
> > On 2016/10/03 07:29:18, dcheng wrote:
> > > Can we structure the macro so we can use the new ranged-based for loop
> syntax
> > > here too? That might simplify the iterator definition itself, since we
won't
> > > need to break out helpers to support the slightly different semantics of
> each
> > > iterator.
> > No, we can't. Some places use GetNext method and Iter construction
explicitly,
> > bypassing this macro.
> 
> As discussed offline, let's switch the macro to use it and followup with
> incremental CLs to remove GetNext()

Done.

https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h
File base/observer_list.h (right):

https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h#n...
base/observer_list.h:122: size_t index_;
On 2016/10/05 07:08:24, dcheng wrote:
> Let's add a comment to document the invariants for |index_|:
> When initially constructed and each time the iterator is incremented,
> |index_| is guaranteed to point to a non-null index if the iterator
> has not reached the end of the ObserverList.
What do you mean by "incremented"? This invariant isn't true for GetNext
increment, for example. Many observers can be unsubscribe at one time. Also, new
observers can subscribe. I'll check this tomorrow.

[dcheng, 2016-10-05 21:01:15]

https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h
File base/observer_list.h (right):

https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h#n...
base/observer_list.h:277: ObserverType* current = GetNonNullCurrent();
On 2016/10/05 07:08:24, dcheng wrote:
> With the previously mentioned invariant in place, would it be possible to
> simplify this to:
> 
> ObserverType* current = GetCurrent();
> ++(*this);
> return current;

You're right, this has to be tweaked a bit.

But this can also be written:

// Make sure index_ is pointing at a non-null element, if the
// current index_ has been nulled out.
index_ = GetNonNullIndex();
ObserverType* current = GetCurrent();
++(*this);
return current;

And we won't need GetNonNullCurrent(). I'm OK with slight inaccuracy (or
explicitly calling out GetNext()) in the invariant documentation previously
mentioned.

[loyso (OOO), 2016-10-06 07:00:27]

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

[commit-bot: I haz the power, 2016-10-06 07:00: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, 2016-10-06 08:41:08]

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

[commit-bot: I haz the power, 2016-10-06 08:41:09]

Dry run: This issue passed the CQ dry run.

[loyso (OOO), 2016-10-07 02:33:20]

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

[commit-bot: I haz the power, 2016-10-07 02:33:32]

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

[loyso (OOO), 2016-10-07 03:45:08]

PTAL!

https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h
File base/observer_list.h (right):

https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h#n...
base/observer_list.h:122: size_t index_;
On 2016/10/05 07:08:24, dcheng wrote:
> Let's add a comment to document the invariants for |index_|:
> 
> Something like:
> 
> When initially constructed and each time the iterator is incremented,
> |index_| is guaranteed to point to a non-null index if the iterator
> has not reached the end of the ObserverList.

Done.

https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h#n...
base/observer_list.h:192: index_(0),
On 2016/10/05 07:08:23, dcheng wrote:
> Then to guarantee the invariant, initialize this to GetNonNullIndex()

Done.

https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h#n...
base/observer_list.h:209: return GetNonNullCurrent() !=
other.GetNonNullCurrent();
On 2016/10/05 07:08:23, dcheng wrote:
> Hmm.
> 
> Given this observer list:
> 
> { nullptr, nullptr, &foo, &bar }
> 
> If iterator A has index_ == 0, and iterator B has index_ == 1, they'll compare
> equal. In some sense they are... and in some sense they aren't.
> 
> Perhaps this check could be:
> 
> return list_ == other_.list && index_ == other_.index_;

Done.

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

https://codereview.chromium.org/2340583005/diff/260001/base/observer_list_uni...
base/observer_list_unittest.cc:224: TEST(ObserverListTest, DesruptSelf) {
On 2016/10/05 07:08:24, dcheng wrote:
> Nit: Desrupt -> Disrupt

Done.

[commit-bot: I haz the power, 2016-10-07 04:22:26]

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

[commit-bot: I haz the power, 2016-10-07 04:22:27]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dcheng, 2016-10-07 05:43:17]

WDYT?

https://codereview.chromium.org/2340583005/diff/320001/base/observer_list.h
File base/observer_list.h (right):

https://codereview.chromium.org/2340583005/diff/320001/base/observer_list.h#n...
base/observer_list.h:217: if (is_end() && other.is_end())
Good catch on the is_end() thing! I think with one further tweak, we can make
remove the need to explicitly check is_end().

https://codereview.chromium.org/2340583005/diff/320001/base/observer_list.h#n...
base/observer_list.h:271: size_t index = index_;
Let's be a little clever here to remove the need for is_end().

Since GetNonNullIndex() is always assigned to |index_|, we can just directly
mutate |index_|. If we directly mutate |index_|, we can also  reset the iterator
state to match the default constructed iterator when we reach the end by
restructuring the loop slightly:

size_t max_index = clamped_max_index();
do {
  if (list_observers_[index]) return;
} while (++index < max_index);
// No more entries, set internal state to the end.
list_ = nullptr;
index_ = 0;

(We'd probably also want to rename this to EnsureValidState() or something,
since this does return a null index when iteration reaches the end of the list)

Then operator== can be the simple comparison.

[loyso (OOO), 2016-10-10 04:13:03]

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

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

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

[loyso (OOO), 2016-10-10 04:14:52]

On 2016/10/07 05:43:17, dcheng wrote:
> WDYT?
That loss of information doesn't work for GetNext and GetNext-like manual
iteration. An extra observer can be added, so the list becomes non-empty again.
See AddObserverInTheLastObserve unit test.

[commit-bot: I haz the power, 2016-10-10 06:43:39]

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

[commit-bot: I haz the power, 2016-10-10 06:43:40]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-10-11 05:02:13]

LGTM.

I'm hoping we can simplify the internal implementation down the road (the end
iterator makes things more tricky), but I'll settle for simplifying the external
interface for now.

[loyso (OOO), 2016-10-11 05:14:53]

The CQ bit was checked by loyso@chromium.org

[loyso (OOO), 2016-10-11 05:14:53]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2340583005/#ps360001
(title: "Remove excessive mutable.")

[commit-bot: I haz the power, 2016-10-11 05:15: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, 2016-10-11 06:51:56]

Description was changed from

==========
Base ObserverList: Add basic support for standard C++ iterators.

Also add a support for const_iterator.

The range-based for loop support follows for free. 

See the use case in the dependent CL.

BUG=634916
==========

to

==========
Base ObserverList: Add basic support for standard C++ iterators.

Also add a support for const_iterator.

The range-based for loop support follows for free. 

See the use case in the dependent CL.

BUG=634916
==========

[commit-bot: I haz the power, 2016-10-11 06:51:57]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2016-10-11 06:53:44]

Description was changed from

==========
Base ObserverList: Add basic support for standard C++ iterators.

Also add a support for const_iterator.

The range-based for loop support follows for free. 

See the use case in the dependent CL.

BUG=634916
==========

to

==========
Base ObserverList: Add basic support for standard C++ iterators.

Also add a support for const_iterator.

The range-based for loop support follows for free.

See the use case in the dependent CL.

BUG=634916

Committed: https://crrev.com/29025b60361ec76cf02bab720703c66ae8047be9
Cr-Commit-Position: refs/heads/master@{#424381}
==========

[commit-bot: I haz the power, 2016-10-11 06:53:45]

Patchset 19 (id:??) landed as
https://crrev.com/29025b60361ec76cf02bab720703c66ae8047be9
Cr-Commit-Position: refs/heads/master@{#424381}