Remove usage of base::ObserverList<T>::Iter::GetNext() in //ui.

Remove usage of base::ObserverList<T>::Iter::GetNext() in //ui.

Observer lists now support range-based for loops.

BUG=655021
Committed: https://crrev.com/d38152e1420a5ee6dcf4768a8254da959045307a
Cr-Commit-Position: refs/heads/master@{#425094}

[dcheng, 2016-10-12 22:41:31]

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

[commit-bot: I haz the power, 2016-10-12 22:42:01]

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-12 23:04:44]

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

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

https://codereview.chromium.org/2414863002/diff/1/ui/wm/core/compound_event_f...
File ui/wm/core/compound_event_filter.cc (right):

https://codereview.chromium.org/2414863002/diff/1/ui/wm/core/compound_event_f...
ui/wm/core/compound_event_filter.cc:133: if (event->stopped_propagation())
I'm not sure that this actually needs to be checked before the first event is
dispatched, but that's the way it was originally written.

[dcheng, 2016-10-12 23:04:57]

Description was changed from

==========
Remove usage of base::ObserverList<T>::Iter::GetNext() in //ui.

Observer lists now support range-based for loops.

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

to

==========
Remove usage of base::ObserverList<T>::Iter::GetNext() in //ui.

Observer lists now support range-based for loops.

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

[commit-bot: I haz the power, 2016-10-12 23:33:54]

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

[commit-bot: I haz the power, 2016-10-12 23:33:55]

Dry run: This issue passed the CQ dry run.

[sky, 2016-10-13 02:20:58]

https://codereview.chromium.org/2414863002/diff/1/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/2414863002/diff/1/ui/aura/window.cc#newcode147
ui/aura/window.cc:147: for (auto& observer : observers_) {
It's my understanding observer is a WindowObserver*. Assuming I'm right doesn't
that mean you should use auto* here? Same comment for the rest of the patch. Of
course I actually prefer being explicit and using WindowObserver*, but I realize
auto is allowed.

https://codereview.chromium.org/2414863002/diff/1/ui/compositor/layer_animato...
File ui/compositor/layer_animator.cc (right):

https://codereview.chromium.org/2414863002/diff/1/ui/compositor/layer_animato...
ui/compositor/layer_animator.cc:875: for (auto& observer : observers_) {
no {}

https://codereview.chromium.org/2414863002/diff/1/ui/events/platform/x11/x11_...
File ui/events/platform/x11/x11_event_source_libevent.cc (left):

https://codereview.chromium.org/2414863002/diff/1/ui/events/platform/x11/x11_...
ui/events/platform/x11/x11_event_source_libevent.cc:182: if
(dispatchers_xevent_.might_have_observers()) {
This code is called on every mouse event, so it seems worth optimizing. Are you
sure you don't want the check here?

https://codereview.chromium.org/2414863002/diff/1/ui/views/mus/pointer_watche...
File ui/views/mus/pointer_watcher_event_router.cc (left):

https://codereview.chromium.org/2414863002/diff/1/ui/views/mus/pointer_watche...
ui/views/mus/pointer_watcher_event_router.cc:20: if
(!observer_list->might_have_observers())
Similar comment about optimizing.

[dcheng, 2016-10-13 02:39:23]

https://codereview.chromium.org/2414863002/diff/1/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/2414863002/diff/1/ui/aura/window.cc#newcode147
ui/aura/window.cc:147: for (auto& observer : observers_) {
On 2016/10/13 02:20:57, sky wrote:
> It's my understanding observer is a WindowObserver*. Assuming I'm right
doesn't
> that mean you should use auto* here? Same comment for the rest of the patch.
Of
> course I actually prefer being explicit and using WindowObserver*, but I
realize
> auto is allowed.

Dereferencing the iterator currently gives you a reference. That's why this
patch uses & throughout rather than *.

I could see an argument for this either way. I'll restore the explicit typenames
since that's pretty straightforward.

https://codereview.chromium.org/2414863002/diff/1/ui/compositor/layer_animato...
File ui/compositor/layer_animator.cc (right):

https://codereview.chromium.org/2414863002/diff/1/ui/compositor/layer_animato...
ui/compositor/layer_animator.cc:875: for (auto& observer : observers_) {
On 2016/10/13 02:20:57, sky wrote:
> no {}

Done.

https://codereview.chromium.org/2414863002/diff/1/ui/events/platform/x11/x11_...
File ui/events/platform/x11/x11_event_source_libevent.cc (left):

https://codereview.chromium.org/2414863002/diff/1/ui/events/platform/x11/x11_...
ui/events/platform/x11/x11_event_source_libevent.cc:182: if
(dispatchers_xevent_.might_have_observers()) {
On 2016/10/13 02:20:57, sky wrote:
> This code is called on every mouse event, so it seems worth optimizing. Are
you
> sure you don't want the check here?

Creation of the iterator is cheap if might_have_observers() is false. Ideally,
I'd like to remove this from ObserverList altogether.

https://codereview.chromium.org/2414863002/diff/1/ui/views/mus/pointer_watche...
File ui/views/mus/pointer_watcher_event_router.cc (left):

https://codereview.chromium.org/2414863002/diff/1/ui/views/mus/pointer_watche...
ui/views/mus/pointer_watcher_event_router.cc:20: if
(!observer_list->might_have_observers())
On 2016/10/13 02:20:57, sky wrote:
> Similar comment about optimizing.

See above.

[loyso (OOO), 2016-10-13 02:40:26]

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

[loyso (OOO), 2016-10-13 02:40:26]

https://codereview.chromium.org/2414863002/diff/1/ui/events/platform/x11/x11_...
File ui/events/platform/x11/x11_event_source_libevent.cc (left):

https://codereview.chromium.org/2414863002/diff/1/ui/events/platform/x11/x11_...
ui/events/platform/x11/x11_event_source_libevent.cc:182: if
(dispatchers_xevent_.might_have_observers()) {
On 2016/10/13 02:20:57, sky wrote:
> This code is called on every mouse event, so it seems worth optimizing. Are
you
> sure you don't want the check here?
We have that optimization as a built-in. If underlying vector is empty, begin()
and end() return lightweight empty iterators, not involving weak pointers
creation stuff.

[sky, 2016-10-13 02:41:03]

https://codereview.chromium.org/2414863002/diff/1/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/2414863002/diff/1/ui/aura/window.cc#newcode147
ui/aura/window.cc:147: for (auto& observer : observers_) {
On 2016/10/13 02:39:22, dcheng wrote:
> On 2016/10/13 02:20:57, sky wrote:
> > It's my understanding observer is a WindowObserver*. Assuming I'm right
> doesn't
> > that mean you should use auto* here? Same comment for the rest of the patch.
> Of
> > course I actually prefer being explicit and using WindowObserver*, but I
> realize
> > auto is allowed.
> 
> Dereferencing the iterator currently gives you a reference. That's why this
> patch uses & throughout rather than *.
> 
> I could see an argument for this either way. I'll restore the explicit
typenames
> since that's pretty straightforward.

If that's the case, then wouldn't the 'observer.' call on 149 fail as it has to
derefence the iterator when the iterator points at the underlying storage which
now has a null?

[dcheng, 2016-10-13 02:48:42]

https://codereview.chromium.org/2414863002/diff/1/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/2414863002/diff/1/ui/aura/window.cc#newcode147
ui/aura/window.cc:147: for (auto& observer : observers_) {
On 2016/10/13 02:41:03, sky wrote:
> On 2016/10/13 02:39:22, dcheng wrote:
> > On 2016/10/13 02:20:57, sky wrote:
> > > It's my understanding observer is a WindowObserver*. Assuming I'm right
> > doesn't
> > > that mean you should use auto* here? Same comment for the rest of the
patch.
> > Of
> > > course I actually prefer being explicit and using WindowObserver*, but I
> > realize
> > > auto is allowed.
> > 
> > Dereferencing the iterator currently gives you a reference. That's why this
> > patch uses & throughout rather than *.
> > 
> > I could see an argument for this either way. I'll restore the explicit
> typenames
> > since that's pretty straightforward.
> 
> If that's the case, then wouldn't the 'observer.' call on 149 fail as it has
to
> derefence the iterator when the iterator points at the underlying storage
which
> now has a null?

We have a reference to the underlying object, not a reference to the pointer in
ObserverList's storage. It's equivalent to something like this:

vector<int*> v = { new int(5) };
int& x = **v.begin();
v.erase(v.begin());
printf("%d\n", x);

The final |x| access is fine, since the vector doesn't actually own the ints.

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

https://codereview.chromium.org/2414863002/diff/1/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/2414863002/diff/1/ui/aura/window.cc#newcode147
ui/aura/window.cc:147: for (auto& observer : observers_) {
On 2016/10/13 02:20:57, sky wrote:
> It's my understanding observer is a WindowObserver*. Assuming I'm right
doesn't
> that mean you should use auto* here? Same comment for the rest of the patch.
Of
> course I actually prefer being explicit and using WindowObserver*, but I
realize
> auto is allowed.
The range based for statement defined in C++ as:

{
  auto && __range = range-init;
  for ( auto __begin = begin-expr,
    __end = end-expr;
    __begin != __end;
    ++__begin ) {
      for-range-declaration = *__begin;
      statement
  }
}

Notice that the de-referencing (for-range-declaration = *__begin) is baked into
it. At the same time, there is a requirement for iterators that
operator-> returns an iterator's pointer type and operator* returns an
iterator's reference type.

As a result, we are forced to work with references (we don't want operator-> to
return a pointer to pointer)

I would rather change AddObserver/RemoveObserver signatures to consume a
reference maybe, but that's not allowed by our code style?

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

https://codereview.chromium.org/2414863002/diff/1/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/2414863002/diff/1/ui/aura/window.cc#newcode147
ui/aura/window.cc:147: for (auto& observer : observers_) {
On 2016/10/13 02:20:57, sky wrote:
> It's my understanding observer is a WindowObserver*. Assuming I'm right
doesn't
> that mean you should use auto* here? Same comment for the rest of the patch.
Of
> course I actually prefer being explicit and using WindowObserver*, but I
realize
> auto is allowed.
Also, in current implementation you are allowed to avoid the syntax sugar and
write directly:
for (auto i = observers_.begin(), end = observers.end(); i != end; ++i)
  i->Observe();

If you want operator* to return a pointer then:
1) you would be allowed to write auto* in the range-based form (notice that
pointer can't be nullptr in a loop invariant, that's the whole point)
2) operator-> would be forced to return a pointer to pointer, i.e. it would
become meaningless. You would be enforced to
use operator* instead:
for (auto i = obesrvers_.begin(), end = observers.end(); i != end; ++i)
  (*i)->Observe();

In both cases, a concept of iterators is far from "observer is a
WindowObserver*". That's an implementation detail of the ObserverList class. For
illustration purposes only: we could implement ObserverList with
std::reference_wrappers (and an std::optional indirection to enable nullability)

[sky, 2016-10-13 17:02:20]

loyso - thanks for the clarification.
LGTM

https://codereview.chromium.org/2414863002/diff/20001/ui/compositor/layer_ani...
File ui/compositor/layer_animator.cc (right):

https://codereview.chromium.org/2414863002/diff/20001/ui/compositor/layer_ani...
ui/compositor/layer_animator.cc:875: for (LayerAnimationObserver& observer :
observers_) {
no {}

[dcheng, 2016-10-13 17:25:59]

https://codereview.chromium.org/2414863002/diff/20001/ui/compositor/layer_ani...
File ui/compositor/layer_animator.cc (right):

https://codereview.chromium.org/2414863002/diff/20001/ui/compositor/layer_ani...
ui/compositor/layer_animator.cc:875: for (LayerAnimationObserver& observer :
observers_) {
On 2016/10/13 17:02:19, sky wrote:
> no {}

Done.

[dcheng, 2016-10-13 17:26:07]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-10-13 17:26:08]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2414863002/#ps40001
(title: "debrace")

[commit-bot: I haz the power, 2016-10-13 17:26:24]

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-13 18:22:01]

Description was changed from

==========
Remove usage of base::ObserverList<T>::Iter::GetNext() in //ui.

Observer lists now support range-based for loops.

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

to

==========
Remove usage of base::ObserverList<T>::Iter::GetNext() in //ui.

Observer lists now support range-based for loops.

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

[commit-bot: I haz the power, 2016-10-13 18:22:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-13 18:25:39]

Description was changed from

==========
Remove usage of base::ObserverList<T>::Iter::GetNext() in //ui.

Observer lists now support range-based for loops.

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

to

==========
Remove usage of base::ObserverList<T>::Iter::GetNext() in //ui.

Observer lists now support range-based for loops.

BUG=655021

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

[commit-bot: I haz the power, 2016-10-13 18:25:40]

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