Add observer interface to PrerenderContents.

chrome/browser/prerender/prerender_tracker.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 "chrome/browser/prerender/prerender_tracker.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/prerender/prerender_manager.h"
@@ skipping 30 matching lines @@
       : final_status(FINAL_STATUS_MAX),
         prerender_manager(prerender_manager->AsWeakPtr()) {
   }
   ~RenderViewInfo() {}
 
   FinalStatus final_status;
   base::WeakPtr<PrerenderManager> prerender_manager;
 };
 
 PrerenderTracker::PrerenderTracker() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
 PrerenderTracker::~PrerenderTracker() {
+  for (std::set<PrerenderContents*>::iterator it = observing_set_.begin();
+      it != observing_set_.end(); ++it) {
+    (*it)->RemoveObserver(this);
+  }

[mmenke, 2012/12/03 20:02:47]

Isn't the PrerenderTracker shut down after the last profile is destroyed, since
it's owned by the browser process?  If so, can just DCHECK the list is empty.

[gavinp, 2012/12/04 18:04:48]

Done.

 }
 
+void PrerenderTracker::AddPrerenderContents(
+    PrerenderContents* prerender_contents) {
+  DCHECK(CalledOnValidThread());
+  prerender_contents->AddObserver(this);
+  observing_set_.insert(prerender_contents);
+}
+
+

[mmenke, 2012/12/03 20:02:47]

nit:  Extra line break not needed.

[gavinp, 2012/12/04 18:04:48]

Done.

 bool PrerenderTracker::TryUse(int child_id, int route_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
+  DCHECK(CalledOnValidThread());
   return SetFinalStatus(child_id, route_id, FINAL_STATUS_USED, NULL);
 }
 
 bool PrerenderTracker::TryCancel(
     int child_id,
     int route_id,
     FinalStatus final_status) {
   DCHECK_NE(FINAL_STATUS_USED, final_status);
   DCHECK(final_status >= 0 && final_status < FINAL_STATUS_MAX);
 
   FinalStatus actual_final_status;
   SetFinalStatus(child_id, route_id, final_status, &actual_final_status);
   return actual_final_status != FINAL_STATUS_USED &&
          actual_final_status != FINAL_STATUS_MAX;
 }
 
 bool PrerenderTracker::TryCancelOnIOThread(
     int child_id,
     int route_id,
     FinalStatus final_status) {
   DCHECK_NE(FINAL_STATUS_USED, final_status);
-  DCHECK(final_status >= 0 && final_status < FINAL_STATUS_MAX);
+  DCHECK_LE(0, final_status);
+  DCHECK_GT(FINAL_STATUS_MAX, final_status);
 
   if (!IsPrerenderingOnIOThread(child_id, route_id))
     return false;
   return TryCancel(child_id, route_id, final_status);
 }
 
 bool PrerenderTracker::GetFinalStatus(int child_id, int route_id,
                                       FinalStatus* final_status) const {
   ChildRouteIdPair child_route_id_pair(child_id, route_id);
 
   base::AutoLock lock(final_status_map_lock_);
   FinalStatusMap::const_iterator final_status_it =
       final_status_map_.find(child_route_id_pair);
-  if (final_status_map_.end() == final_status_map_.find(child_route_id_pair))
+  if (final_status_map_.count(child_route_id_pair) == 0)
     return false;
   *final_status = final_status_it->second.final_status;
   return true;
 }
 
-void PrerenderTracker::OnPrerenderingStarted(
-    int child_id, int route_id, PrerenderManager* prerender_manager) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK_GE(child_id, 0);
-  DCHECK_GE(route_id, 0);
+void PrerenderTracker::OnPrerenderStart(
+    PrerenderContents* prerender_contents) {
+  DCHECK(CalledOnValidThread());
+  int child_id;
+  if (!prerender_contents->GetChildId(&child_id))
+    return;
+  int route_id;
+  if (!prerender_contents->GetRouteId(&route_id))
+    return;

[mmenke, 2012/12/03 20:02:47]

nit:  May be a little prettier if these if statements are merged, though doesn't
even save one line of code. :(

[gavinp, 2012/12/04 18:04:48]

I'm going to go ahead and ignore this nit from this review, since it doesn't
save lines, and I find this version prettier. But, if you repeat the nit, I'll
just follow it.

 
   ChildRouteIdPair child_route_id_pair(child_id, route_id);
 
   // The RenderView should not already be prerendering.
-  DCHECK(final_status_map_.end() ==
-         final_status_map_.find(child_route_id_pair));
+  DCHECK_EQ(0u, final_status_map_.count(child_route_id_pair));
 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&AddPrerenderOnIOThreadTask, child_route_id_pair));
 
   base::AutoLock lock(final_status_map_lock_);
 
   final_status_map_.insert(
-      std::make_pair(child_route_id_pair, RenderViewInfo(prerender_manager)));
+      std::make_pair(child_route_id_pair,
+                     RenderViewInfo(prerender_contents->prerender_manager())));
 }
 
-void PrerenderTracker::OnPrerenderingFinished(int child_id, int route_id) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DCHECK_GE(child_id, 0);
-  DCHECK_GE(route_id, 0);
+void PrerenderTracker::OnPrerenderStop(
+    PrerenderContents* prerender_contents) {
+  DCHECK(CalledOnValidThread());
+  prerender_contents->RemoveObserver(this);

[mmenke, 2012/12/03 20:02:47]

Wonder if we should just clear the observer list in NotifyPrerenderStop.

[gavinp, 2012/12/04 18:04:48]

That's a great idea; it avoids a ton of crashes.

Done.

+  observing_set_.erase(prerender_contents);

[mmenke, 2012/12/03 20:02:47]

Suggest a DCHECK_EQ(1, observer_set_.count(prerender_contents));

[gavinp, 2012/12/04 18:04:48]

Done. This turned out to require a significant rewrite of the unit test, and a
few changes in PrerenderContents() to make sure we don't OnPrerenderStop without
having earlier OnPrerenderStart().

+  int child_id;
+  if (!prerender_contents->GetChildId(&child_id))
+    return;
+  int route_id;
+  if (!prerender_contents->GetRouteId(&route_id))
+    return;

[mmenke, 2012/12/03 20:02:47]

nit:  May be a little prettier if these if statements are merged, though doesn't
even save one line of code. :(

 
   ChildRouteIdPair child_route_id_pair(child_id, route_id);
 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&RemovePrerenderOnIOThreadTask, child_route_id_pair));
 
   base::AutoLock lock(final_status_map_lock_);
   size_t num_erased = final_status_map_.erase(child_route_id_pair);
   DCHECK_EQ(1u, num_erased);
@@ skipping 35 matching lines @@
   if (actual_final_status)
     *actual_final_status = final_status_it->second.final_status;
   return false;
 }
 
 bool PrerenderTracker::IsPrerenderingOnIOThread(int child_id,
                                                 int route_id) const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   ChildRouteIdPair child_route_id_pair(child_id, route_id);
-  return possibly_prerendering_io_thread_set_.end() !=
-         possibly_prerendering_io_thread_set_.find(child_route_id_pair);
+  return possibly_prerendering_io_thread_set_.count(child_route_id_pair) > 0;
 }
 
 void PrerenderTracker::AddPrerenderOnIOThread(
     const ChildRouteIdPair& child_route_id_pair) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK(!IsPrerenderingOnIOThread(child_route_id_pair.first,
                                    child_route_id_pair.second));
 
   possibly_prerendering_io_thread_set_.insert(child_route_id_pair);
 }
@@ skipping 18 matching lines @@
   GetDefault()->AddPrerenderOnIOThread(child_route_id_pair);
 }
 
 // static
 void PrerenderTracker::RemovePrerenderOnIOThreadTask(
     const ChildRouteIdPair& child_route_id_pair) {
   GetDefault()->RemovePrerenderOnIOThread(child_route_id_pair);
 }
 
 }  // namespace prerender