Add TabWebContentsTracker

chrome/browser/managed_mode/managed_mode_navigation_observer.cc

Index: chrome/browser/managed_mode/managed_mode_navigation_observer.cc
diff --git a/chrome/browser/managed_mode/managed_mode_navigation_observer.cc b/chrome/browser/managed_mode/managed_mode_navigation_observer.cc
index 91c3ac445fbb95052be8d710802a1584b9f2b91e..2d192f8ff51b1f64a1b8082f0f20426dea7326db 100644
--- a/chrome/browser/managed_mode/managed_mode_navigation_observer.cc
+++ b/chrome/browser/managed_mode/managed_mode_navigation_observer.cc
@@ -256,6 +256,7 @@ void ManagedModePreviewInfobarDelegate::InfoBarDismissed() {
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(ManagedModeNavigationObserver);
 
 ManagedModeNavigationObserver::~ManagedModeNavigationObserver() {
+  TabWebContentsTracker::RemoveObserver(this);
   RemoveTemporaryException();
 }
 
@@ -267,6 +268,7 @@ ManagedModeNavigationObserver::ManagedModeNavigationObserver(
       state_(RECORDING_URLS_BEFORE_PREVIEW),
       is_elevated_(false),
       last_allowed_page_(-1),
+      observer_state_cleared_(false),
       finished_redirects_(false) {
   Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
@@ -274,6 +276,39 @@ ManagedModeNavigationObserver::ManagedModeNavigationObserver(
   if (!managed_user_service_->ProfileIsManaged())
     is_elevated_ = true;
   url_filter_ = managed_user_service_->GetURLFilterForUIThread();
+  TabWebContentsTracker::AddObserver(this);
+}
+
+void ManagedModeNavigationObserver::OnTabReplacedAt(
+    TabStripModel* tab_strip_model,
+    content::WebContents* old_contents,
+    content::WebContents* new_contents,
+    int index) {
+  if (new_contents == web_contents()) {

[Bernhard Bauer, 2013/04/26 16:30:10]

Early-return otherwise?

+    // First, copy over some data of the old navigation observer. This only

[Bernhard Bauer, 2013/04/26 16:30:10]

This seems...brittle. I'm okay with it, but I could imagine running into subtle
bugs down the line later (we already need special treatment when
ClearObserverState has been called, for example). I think an easier way would be
to add a ReleaseUserData() method to base::SupportsUserData and move the whole
object over.

[Adrian Kuegel, 2013/04/29 08:11:15]

But if I copy the whole object over, it would still not contain the right data,
and I would have to do the same thing. This replace of WebContents seems to
happen mostly (if not always) in the case where a page is prerendered. In that
case, we will be notified after the page has been already prerendered, and then
the ManagedModeNavigationObserver attached to the prerendered WebContents
contains some data we may need to copy over (or contains the information that we
have to clear some data). If we really just want to copy the object over, we
would have to make sure that we do that when creating the WebContents that is
used for the prerendering. I already thought that this would probably be the
best thing, but I am not sure if that is feasible (we would have to clone the
object, because I think we cannot be sure if the prerendered WebContents will
actually be used later).

[Bernhard Bauer, 2013/04/29 08:39:38]

Like I said, I'm okay with this, I just want to keep us from having to add more
special cases later. Seeing as the prerender-swap-in happens instead of a
navigation, could we get the desired behavior by treating it as a navigation
event (recording URLs etc.)?

+    // applies if the ClearObserverState function was not called on the new

[Adrian Kuegel, 2013/04/26 15:52:27]

Reassigning the ManagedModeNavigationObserver to another WebContents does not
work. When removing from WebContentsUserData it would already call the
destructor.

+    // navigation observer, because that would reset the data we are about to
+    // copy.
+    if (!observer_state_cleared_) {
+      ManagedModeNavigationObserver* observer =
+          ManagedModeNavigationObserver::FromWebContents(old_contents);
+      navigated_urls_.insert(observer->navigated_urls_.begin(),
+                             observer->navigated_urls_.end());
+      if (last_allowed_page_ < 0)
+        last_allowed_page_ = observer->last_allowed_page_;
+      if (observer->warn_infobar_delegate_ && !warn_infobar_delegate_) {
+        warn_infobar_delegate_ = ManagedModeWarningInfobarDelegate::Create(
+            InfoBarService::FromWebContents(web_contents()),
+            last_allowed_page_);
+      }
+      if (observer->preview_infobar_delegate_ && !preview_infobar_delegate_) {
+        preview_infobar_delegate_ = ManagedModePreviewInfobarDelegate::Create(

[Bernhard Bauer, 2013/04/26 16:30:10]

Doesn't InfobarTabHelper or something similar take care of this? At least I've
always seen Infobars come over even when a WebContents is replaced.

[Adrian Kuegel, 2013/04/29 08:11:15]

Yes, that is possible. I guess I can remove that code part then.

+            InfoBarService::FromWebContents(web_contents()));
+      }
+      // We don't need to copy |state_| and |finished_redirects_|, because that
+      // should be up-to-date.
+    }
+  }
 }
 
 void ManagedModeNavigationObserver::AddTemporaryException() {
@@ -387,6 +422,7 @@ void ManagedModeNavigationObserver::ClearObserverState() {
     infobar_service->RemoveInfoBar(preview_infobar_delegate_);
     preview_infobar_delegate_ = NULL;
   }
+  observer_state_cleared_ = true;

[Bernhard Bauer, 2013/04/26 16:30:10]

So you set this to true when we have cleared observer state at some point, and
then never set it to false again?

[Adrian Kuegel, 2013/04/29 08:11:15]

Yes, but that is exactly as expected. This variable is only used for deciding if
to copy data from the old ManagedModeNavigationObserver. We only need to copy
data from the old ManagedModeNavigationObserver, if _never_ ClearObserverState()
was called in the new Observer. Imagine we had copied the data over when
initializing the new Observer (which I think we should actually do). Then this
data would have been gone now.

   navigated_urls_.clear();
   last_url_ = GURL();
   state_ = RECORDING_URLS_BEFORE_PREVIEW;