Add TabWebContentsTracker

Add TabWebContentsTracker

BUG=234231

[Adrian Kuegel, 2013-04-26 15:52:26]

Bernhard, can you please take a look if this CL is going in the right direction?

https://codereview.chromium.org/14518005/diff/1/chrome/browser/managed_mode/m...
File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right):

https://codereview.chromium.org/14518005/diff/1/chrome/browser/managed_mode/m...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:289: // applies
if the ClearObserverState function was not called on the new
Reassigning the ManagedModeNavigationObserver to another WebContents does not
work. When removing from WebContentsUserData it would already call the
destructor.

https://codereview.chromium.org/14518005/diff/1/chrome/browser/ui/tab_webcont...
File chrome/browser/ui/tab_webcontents_tracker.h (right):

https://codereview.chromium.org/14518005/diff/1/chrome/browser/ui/tab_webcont...
chrome/browser/ui/tab_webcontents_tracker.h:25: // TabStripModelObserver:
Should I add all methods from TabStripModelObserver?

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

A couple of style issues, but please read the bigger comments first:

https://codereview.chromium.org/14518005/diff/1/chrome/browser/ui/tab_webcont...

https://codereview.chromium.org/14518005/diff/1/chrome/browser/managed_mode/m...
File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right):

https://codereview.chromium.org/14518005/diff/1/chrome/browser/managed_mode/m...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:287: if
(new_contents == web_contents()) {
Early-return otherwise?

https://codereview.chromium.org/14518005/diff/1/chrome/browser/managed_mode/m...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:288: // First,
copy over some data of the old navigation observer. This only
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.

https://codereview.chromium.org/14518005/diff/1/chrome/browser/managed_mode/m...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:305:
preview_infobar_delegate_ = ManagedModePreviewInfobarDelegate::Create(
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.

https://codereview.chromium.org/14518005/diff/1/chrome/browser/managed_mode/m...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:425:
observer_state_cleared_ = true;
So you set this to true when we have cleared observer state at some point, and
then never set it to false again?

https://codereview.chromium.org/14518005/diff/1/chrome/browser/ui/tab_webcont...
File chrome/browser/ui/tab_webcontents_tracker.cc (right):

https://codereview.chromium.org/14518005/diff/1/chrome/browser/ui/tab_webcont...
chrome/browser/ui/tab_webcontents_tracker.cc:15:
DefaultSingletonTraits<TabWebContentsTracker> >::get();
I think DefaultSingletonTraits is the default, so it can be omitted, right?

https://codereview.chromium.org/14518005/diff/1/chrome/browser/ui/tab_webcont...
chrome/browser/ui/tab_webcontents_tracker.cc:23: BrowserList::const_iterator it
= browser_list->begin();
Could you put this into a for loop? That's slightly more idiomatic.

https://codereview.chromium.org/14518005/diff/1/chrome/browser/ui/tab_webcont...
chrome/browser/ui/tab_webcontents_tracker.cc:46: FOR_EACH_OBSERVER(
Hmm... Instead of forwarding all these methods from TabStripModelObserver
(assuming that we're using TabStripModelObserver in place of
TabWebContentsTracker::Observer), couldn't we directly add the observers as
observers of the tab strip model? And going a bit further, then we could make
this class a member of every ManagedModeNavigationObserver instead of a
singleton with a list of observers. And *then* we could even optimize a bit by
only registering ourselves as observers of TabStripModels in our own profile
(because we might get moved between browsers, but not between profiles).

https://codereview.chromium.org/14518005/diff/1/chrome/browser/ui/tab_webcont...
File chrome/browser/ui/tab_webcontents_tracker.h (right):

https://codereview.chromium.org/14518005/diff/1/chrome/browser/ui/tab_webcont...
chrome/browser/ui/tab_webcontents_tracker.h:16: namespace content { class
WebContents; }
Forward declarations go on a separate line.

https://codereview.chromium.org/14518005/diff/1/chrome/browser/ui/tab_webcont...
chrome/browser/ui/tab_webcontents_tracker.h:23: static void Init();
This method isn't implemented.

https://codereview.chromium.org/14518005/diff/1/chrome/browser/ui/tab_webcont...
chrome/browser/ui/tab_webcontents_tracker.h:25: // TabStripModelObserver:
On 2013/04/26 15:52:27, Adrian Kuegel wrote:
> Should I add all methods from TabStripModelObserver?

Hm. Is there even a reason we need a separate interface for this? Why don't we
just use TabStripModelObserver as the interface?

https://codereview.chromium.org/14518005/diff/1/chrome/browser/ui/tab_webcont...
chrome/browser/ui/tab_webcontents_tracker.h:47: class Observer {
Inner class declarations go to the beginning of the (outer) class.

https://codereview.chromium.org/14518005/diff/1/chrome/browser/ui/tab_webcont...
chrome/browser/ui/tab_webcontents_tracker.h:83: ObserverList<Observer>
observer_list_;
Add a newline between method and member declarations?

[Bernhard Bauer, 2013-04-26 16:34:16]

Hit Publish too soon. What I meant was, please read these comments first: 

1.
https://codereview.chromium.org/14518005/diff/1/chrome/browser/ui/tab_webcont...

2.
https://codereview.chromium.org/14518005/diff/1/chrome/browser/ui/tab_webcont...

Because some of the other things might become irrelevant then.

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

I didn't address most of the style comments, because I guess I need to do
everything differently, anyway. See my comments below.

https://codereview.chromium.org/14518005/diff/1/chrome/browser/managed_mode/m...
File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right):

https://codereview.chromium.org/14518005/diff/1/chrome/browser/managed_mode/m...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:288: // First,
copy over some data of the old navigation observer. This only
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).

https://codereview.chromium.org/14518005/diff/1/chrome/browser/managed_mode/m...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:305:
preview_infobar_delegate_ = ManagedModePreviewInfobarDelegate::Create(
Yes, that is possible. I guess I can remove that code part then.

https://codereview.chromium.org/14518005/diff/1/chrome/browser/managed_mode/m...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:425:
observer_state_cleared_ = true;
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.

https://codereview.chromium.org/14518005/diff/1/chrome/browser/ui/tab_webcont...
File chrome/browser/ui/tab_webcontents_tracker.h (right):

https://codereview.chromium.org/14518005/diff/1/chrome/browser/ui/tab_webcont...
chrome/browser/ui/tab_webcontents_tracker.h:25: // TabStripModelObserver:
I didn't think about it, but you are right, that should be better.

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

https://codereview.chromium.org/14518005/diff/1/chrome/browser/managed_mode/m...
File chrome/browser/managed_mode/managed_mode_navigation_observer.cc (right):

https://codereview.chromium.org/14518005/diff/1/chrome/browser/managed_mode/m...
chrome/browser/managed_mode/managed_mode_navigation_observer.cc:288: // First,
copy over some data of the old navigation observer. This only
On 2013/04/29 08:11:15, Adrian Kuegel wrote:
> 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).

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.)?