Use scoped_ptr to document ownership in InstantUnloadHandler.

And some cosmetic changes in other Instant files.

BUG=none
R=jered@chromium.org,sky@chromium.org
TEST=none; no change in functionality.


Review URL: https://chromiumcodereview.appspot.com/12256029

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@182397 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/instant/instant_commit_type.h

@@ -5,20 +5,20 @@
 #ifndef CHROME_BROWSER_INSTANT_INSTANT_COMMIT_TYPE_H_
 #define CHROME_BROWSER_INSTANT_INSTANT_COMMIT_TYPE_H_
 
-// Reason why the Instant preview is committed (merged into a tab).
+// Reason why the Instant overlay is committed (merged into a tab).
 enum InstantCommitType {
   // The commit is due to the user pressing Enter from the omnibox.
   INSTANT_COMMIT_PRESSED_ENTER,
 
   // The commit is due to the user pressing Alt-Enter from the omnibox (which
-  // causes the preview to be committed on to a new tab).
+  // causes the overlay to be committed onto a new tab).
   INSTANT_COMMIT_PRESSED_ALT_ENTER,
 
   // The commit is due to the omnibox losing focus, usually due to the user
-  // clicking on the preview.
+  // clicking on the overlay.
   INSTANT_COMMIT_FOCUS_LOST,
 
-  // The commit is due to the instant page navigating.
+  // The commit is due to the Instant overlay navigating to a non-Instant URL.
   INSTANT_COMMIT_NAVIGATED,
 
   // The commit is due to the user clicking on a query suggestion.

chrome/browser/instant/instant_service.cc

@@ -22,11 +22,7 @@ void InstantService::AddInstantProcess(int process_id) {
 }
 
 bool InstantService::IsInstantProcess(int process_id) const {
-  return process_ids_.count(process_id) != 0;
-}
-
-int InstantService::GetInstantProcessCount() const {
-  return process_ids_.size();
+  return process_ids_.find(process_id) != process_ids_.end();
 }
 
 void InstantService::Shutdown() {

chrome/browser/instant/instant_service.h

@@ -13,7 +13,7 @@
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 
-// Tracks render process host ids that are associated with Instant.
+// Tracks render process host IDs that are associated with Instant.
 class InstantService : public ProfileKeyedService,
                        public content::NotificationObserver {
  public:
@@ -25,13 +25,17 @@ class InstantService : public ProfileKeyedService,
   void AddInstantProcess(int process_id);
   bool IsInstantProcess(int process_id) const;
 
-  // For testing.
-  int GetInstantProcessCount() const;
+#if defined(UNIT_TEST)
+  int GetInstantProcessCount() const {
+    return process_ids_.size();
+  }
+#endif
 
  private:
-  // ProfileKeyedService:
+  // Overridden from ProfileKeyedService:
   virtual void Shutdown() OVERRIDE;
 
+  // Overridden from content::NotificationObserver:
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;

chrome/browser/instant/instant_service_factory.cc

@@ -18,11 +18,6 @@ InstantServiceFactory* InstantServiceFactory::GetInstance() {
   return Singleton<InstantServiceFactory>::get();
 }
 
-// static
-ProfileKeyedService* InstantServiceFactory::BuildInstanceFor(Profile* profile) {
-  return new InstantService;
-}
-
 InstantServiceFactory::InstantServiceFactory()
     : ProfileKeyedServiceFactory("InstantService",
                                  ProfileDependencyManager::GetInstance()) {
@@ -38,5 +33,5 @@ bool InstantServiceFactory::ServiceRedirectedInIncognito() const {
 
 ProfileKeyedService* InstantServiceFactory::BuildServiceInstanceFor(
     Profile* profile) const {
-  return BuildInstanceFor(profile);
+  return new InstantService;
 }

chrome/browser/instant/instant_service_factory.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_INSTANT_INSTANT_SERVICE_FACTORY_H_
 
 #include "base/basictypes.h"
+#include "base/compiler_specific.h"
 #include "base/memory/singleton.h"
 #include "chrome/browser/profiles/profile_keyed_service_factory.h"
 
@@ -13,8 +14,7 @@ class InstantService;
 class Profile;
 class ProfileKeyedService;
 
-// Singleton that owns all InstantServices and associates them with
-// Profiles.
+// Singleton that owns all InstantServices and associates them with Profiles.
 class InstantServiceFactory : public ProfileKeyedServiceFactory {
  public:
   // Returns the InstantService for |profile|.
@@ -22,15 +22,13 @@ class InstantServiceFactory : public ProfileKeyedServiceFactory {
 
   static InstantServiceFactory* GetInstance();
 
-  static ProfileKeyedService* BuildInstanceFor(Profile* profile);
-
  private:
   friend struct DefaultSingletonTraits<InstantServiceFactory>;
 
   InstantServiceFactory();
   virtual ~InstantServiceFactory();
 
-  // ProfileKeyedServiceFactory:
+  // Overridden from ProfileKeyedServiceFactory:
   virtual bool ServiceRedirectedInIncognito() const OVERRIDE;
   virtual ProfileKeyedService* BuildServiceInstanceFor(
       Profile* profile) const OVERRIDE;

chrome/browser/instant/instant_unload_handler.cc

@@ -12,42 +12,43 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_delegate.h"
 
-// WebContentsDelegate implementation. This owns the WebContents supplied to the
-// constructor.
 class InstantUnloadHandler::WebContentsDelegateImpl
     : public content::WebContentsDelegate {
  public:
   WebContentsDelegateImpl(InstantUnloadHandler* handler,
-                          content::WebContents* contents,
+                          scoped_ptr<content::WebContents> contents,
                           int index)
       : handler_(handler),
-        contents_(contents),
+        contents_(contents.Pass()),
         index_(index) {
-    contents->SetDelegate(this);
+    contents_->SetDelegate(this);
+    contents_->GetRenderViewHost()->FirePageBeforeUnload(false);
   }
 
-  // content::WebContentsDelegate overrides:
-  virtual void WillRunBeforeUnloadConfirm() OVERRIDE {
-    content::WebContents* contents = contents_.release();
-    contents->SetDelegate(NULL);
-    handler_->Activate(this, contents, index_);
+  // Overridden from content::WebContentsDelegate:
+  virtual void CloseContents(content::WebContents* source) OVERRIDE {
+    DCHECK_EQ(contents_, source);
+    // Remove ourselves as the delegate, so that CloseContents() won't be
+    // called twice, leading to double deletion (http://crbug.com/155848).
+    contents_->SetDelegate(NULL);
+    handler_->Destroy(this);
   }
 
-  virtual bool ShouldSuppressDialogs() OVERRIDE {
-    return true;  // Return true so dialogs are suppressed.
+  virtual void WillRunBeforeUnloadConfirm() OVERRIDE {
+    contents_->SetDelegate(NULL);
+    handler_->Activate(this, contents_.Pass(), index_);
   }
 
-  virtual void CloseContents(content::WebContents* source) OVERRIDE {
-    contents_->SetDelegate(NULL);
-    handler_->Destroy(this);
+  virtual bool ShouldSuppressDialogs() OVERRIDE {
+    return true;
   }
 
  private:
   InstantUnloadHandler* const handler_;
   scoped_ptr<content::WebContents> contents_;
 
-  // The index |contents_| was originally at. If we add the tab back we add it
-  // at this index.
+  // The tab strip index |contents_| was originally at. If we add the tab back
+  // to the tabstrip, we add it at this index.
   const int index_;
 
   DISALLOW_COPY_AND_ASSIGN(WebContentsDelegateImpl);
@@ -61,7 +62,7 @@ InstantUnloadHandler::~InstantUnloadHandler() {
 }
 
 void InstantUnloadHandler::RunUnloadListenersOrDestroy(
-    content::WebContents* contents,
+    scoped_ptr<content::WebContents> contents,
     int index) {
   DCHECK(!contents->GetDelegate());
 
@@ -71,27 +72,25 @@ void InstantUnloadHandler::RunUnloadListenersOrDestroy(
     // get here from BrowserInstantController::TabDeactivated, other tab
     // observers may still expect to interact with the tab before the event has
     // finished propagating.
-    MessageLoop::current()->DeleteSoon(FROM_HERE, contents);
+    MessageLoop::current()->DeleteSoon(FROM_HERE, contents.release());
     return;
   }
 
-  // Tab has before unload listener. Install a delegate and fire the before
-  // unload listener.
-  delegates_.push_back(new WebContentsDelegateImpl(this, contents, index));
-  contents->GetRenderViewHost()->FirePageBeforeUnload(false);
+  // Tab has beforeunload listeners. Install a delegate to run them.
+  delegates_.push_back(
+      new WebContentsDelegateImpl(this, contents.Pass(), index));
 }
 
 void InstantUnloadHandler::Activate(WebContentsDelegateImpl* delegate,
-                                    content::WebContents* contents,
+                                    scoped_ptr<content::WebContents> contents,
                                     int index) {
-  chrome::NavigateParams params(browser_, contents);
-  params.disposition = NEW_FOREGROUND_TAB;
-  params.tabstrip_index = index;
-
   // Remove (and delete) the delegate.
   Destroy(delegate);
 
   // Add the tab back in.
+  chrome::NavigateParams params(browser_, contents.release());
+  params.disposition = NEW_FOREGROUND_TAB;
+  params.tabstrip_index = index;
   chrome::Navigate(&params);
 }
 

chrome/browser/instant/instant_unload_handler.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_INSTANT_INSTANT_UNLOAD_HANDLER_H_
 
 #include "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 
 class Browser;
@@ -14,29 +15,39 @@ namespace content {
 class WebContents;
 }
 
-// InstantUnloadHandler ensures that the beforeunload and unload handlers are
-// run when using Instant. When the user commits the Instant preview the
-// existing WebContents is passed to RunUnloadListenersOrDestroy(). If the tab
-// has no beforeunload or unload listeners, the tab is deleted; otherwise the
-// beforeunload and unload listeners are executed. If the beforeunload listener
-// shows a dialog the tab is added back to the tabstrip at its original location
-// next to the Instant page.
+// InstantUnloadHandler ensures that it runs the BeforeUnload/Unload Handlers
+// (BUH) of a page if the page is replaced by an Instant overlay.
+//
+// Why is this needed? Say the user is looking at a page P. They then try to
+// navigate to another page Q. Consider what happens with and without Instant:
+//
+// Without Instant: Before the navigation is committed, P's BUH are run. If P's
+// BUH return a string (instead of the default null), the user is prompted to
+// "Stay or Leave?". If the user clicks "Stay", the navigation is cancelled,
+// and the user remains on P.
+//
+// With Instant: The navigation to Q has already happened, since Q is being
+// shown as a preview (overlay). When the user "commits" the overlay, it's too
+// late to cancel Q based on P's BUH. So, Instant just replaces P with Q and
+// passes P to InstantUnloadHandler::RunUnloadListenersOrDestroy(). This class
+// runs P's BUH in the background. If the "Stay or Leave?" dialog needs to be
+// shown, it adds P back onto the tabstrip, next to Q. Otherwise, P is deleted.
 class InstantUnloadHandler {
  public:
   explicit InstantUnloadHandler(Browser* browser);
   ~InstantUnloadHandler();
 
   // See class description for details on what this does.
-  void RunUnloadListenersOrDestroy(content::WebContents* contents, int index);
+  void RunUnloadListenersOrDestroy(scoped_ptr<content::WebContents> contents,
+                                   int index);
 
  private:
   class WebContentsDelegateImpl;
 
   // Invoked if the tab is to be shown, at |index| on the tab strip. This
-  // happens if the before unload listener returns a string. Takes ownership of
-  // |delegate| and |contents|.
+  // happens if the beforeunload listener returns a string.
   void Activate(WebContentsDelegateImpl* delegate,
-                content::WebContents* contents,
+                scoped_ptr<content::WebContents> contents,
                 int index);
 
   // Destroys the old tab. This is invoked if script tries to close the page.

chrome/browser/ui/browser_instant_controller.cc

@@ -169,15 +169,10 @@ void BrowserInstantController::ReplaceWebContentsAt(
     int index,
     scoped_ptr<content::WebContents> new_contents) {
   DCHECK_NE(TabStripModel::kNoTab, index);
-  content::WebContents* old_contents =
-      browser_->tab_strip_model()->GetWebContentsAt(index);
-  // TabStripModel takes ownership of |new_contents|.
-  browser_->tab_strip_model()->ReplaceWebContentsAt(
-      index, new_contents.release());
-  // TODO(samarth): use scoped_ptr instead of comments to document ownership
-  // transfer.
-  // InstantUnloadHandler takes ownership of |old_contents|.
-  instant_unload_handler_.RunUnloadListenersOrDestroy(old_contents, index);
+  scoped_ptr<content::WebContents> old_contents(browser_->tab_strip_model()->
+      ReplaceWebContentsAt(index, new_contents.release()));
+  instant_unload_handler_.RunUnloadListenersOrDestroy(old_contents.Pass(),
+                                                      index);
 }
 
 void BrowserInstantController::SetInstantSuggestion(