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

ui/aura/window.cc

Index: ui/aura/window.cc
diff --git a/ui/aura/window.cc b/ui/aura/window.cc
index d92078768d12c4d114c7178ac53db7372da53c33..58fc5d8fc93c38a1e2317f6f5a62c2e4a319bf38 100644
--- a/ui/aura/window.cc
+++ b/ui/aura/window.cc
@@ -144,11 +144,9 @@ Window::~Window() {
 
   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