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

ui/aura/window.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/aura/window.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <utility>
@@ skipping 126 matching lines @@
   // Then destroy the children.
   RemoveOrDestroyChildren();
 
   // The window needs to be removed from the parent before calling the
   // WindowDestroyed callbacks of delegate and the observers.
   if (parent_)
     parent_->RemoveChild(this);
 
   if (delegate_)
     delegate_->OnWindowDestroyed(this);
-  base::ObserverListBase<WindowObserver>::Iterator iter(&observers_);
-  for (WindowObserver* observer = iter.GetNext(); observer;
-       observer = iter.GetNext()) {
-    RemoveObserver(observer);
-    observer->OnWindowDestroyed(this);
+  for (auto& observer : observers_) {

[sky, 2016/10/13 02:20:57]

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.

[dcheng, 2016/10/13 02:39:22]

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.

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

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]

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]

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]

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)

+    RemoveObserver(&observer);
+    observer.OnWindowDestroyed(this);
   }
 
   // Delete the LayoutManager before properties. This way if the LayoutManager
   // depends upon properties existing the properties are still valid.
   layout_manager_.reset();
 
   // Clear properties.
   for (std::map<const void*, Value>::const_iterator iter = prop_map_.begin();
        iter != prop_map_.end();
        ++iter) {
@@ skipping 956 matching lines @@
     layer_name = "Unnamed Window";
 
   if (id_ != -1)
     layer_name += " " + base::IntToString(id_);
 
   layer()->set_name(layer_name);
 #endif
 }
 
 }  // namespace aura