Abstract WebContents/NavigationController from core TabRestore code

Abstract WebContents/NavigationController from core TabRestore code

As the final piece of eliminating //content dependencies from core TabRestore
code to enable clean integration with iOS, this CL eliminates that code's
knowledge of WebContents and NavigationController.

To do so, it introduces an LiveTab interface that is an abstract representation
of an open tab from the tab restore POV. It also introduces a ContentLiveTab
implementation that is backed by WebContents and used by //chrome.

Core TabRestore code is moved from talking to WebContents/NavigationController
to talking to LiveTab. //chrome usage of this core code is changed to convert
back and forth between a (Content)LiveTab and WebContents as necessary, using
the fact that ContentLiveTab is a WebContentsUserData with a web_contents()
accessor.

BUG=530174
Committed: https://crrev.com/80c5b2a0321d38d4af5761c7078c8972828eaa31
Cr-Commit-Position: refs/heads/master@{#349454}

[blundell, 2015-09-16 16:16:55]

blundell@chromium.org changed reviewers:
+ sky@chromium.org

[blundell, 2015-09-16 16:20:36]

Hi Scott,

The core pieces of this CL are:

1. The introduction of OpenTab and ContentOpenTab in //components/sessions.
2. The change of TabRestoreServiceHelper from using //content-based classes to
using OpenTab.

All the other changes are plumbing to enable change 2.

Note that this is dependent on https://codereview.chromium.org/1343833002/.

(BTW, if you hate OpenTab as much as you hated LiveTab, I'm open to suggestions
;).

https://codereview.chromium.org/1321713005/diff/140001/components/sessions.gypi
File components/sessions.gypi (right):

https://codereview.chromium.org/1321713005/diff/140001/components/sessions.gy...
components/sessions.gypi:72: 'type': 'static_library',
You can ignore this; it's a temporary workaround for the WebContentsUserData
issue I described in my chromium-dev@ email. I'll fix it one way or another
before landing.

https://codereview.chromium.org/1321713005/diff/140001/components/sessions/se...
File components/sessions/sessions_export.h (right):

https://codereview.chromium.org/1321713005/diff/140001/components/sessions/se...
components/sessions/sessions_export.h:8: #if 0
These changes are also for the WebContentsUserData-in-component-build problem
and can also be ignored; I won't land with them present.

[sky, 2015-09-16 20:04:53]

https://codereview.chromium.org/1321713005/diff/140001/chrome/browser/ui/pane...
File chrome/browser/ui/panels/panel_host.cc (right):

https://codereview.chromium.org/1321713005/diff/140001/chrome/browser/ui/pane...
chrome/browser/ui/panels/panel_host.cc:70:
sessions::ContentOpenTab::CreateForWebContents(web_contents_.get());
It would be nice not to have to do this in a bunch of different place. How about
ContentOpenTab::Get() that does the creation if necessary?

https://codereview.chromium.org/1321713005/diff/140001/components/sessions/co...
File components/sessions/content/content_open_tab.h (right):

https://codereview.chromium.org/1321713005/diff/140001/components/sessions/co...
components/sessions/content/content_open_tab.h:46: friend class
content::WebContentsUserData<ContentOpenTab>;
nit: generally friend decls are before functions (including
constructor/destructor).

https://codereview.chromium.org/1321713005/diff/140001/components/sessions/co...
File components/sessions/core/open_tab.h (right):

https://codereview.chromium.org/1321713005/diff/140001/components/sessions/co...
components/sessions/core/open_tab.h:18: class SESSIONS_EXPORT OpenTab {
I'm not too thrilled about this name. It reads as if it's a verb, when that
isn't the case. LiveTab maybe?

https://codereview.chromium.org/1321713005/diff/140001/components/sessions/co...
components/sessions/core/open_tab.h:33: // Returs the user agent override, if
any.
Returns

[blundell, 2015-09-16 20:19:41]

blundell@chromium.org changed reviewers:
+ avi@chromium.org

[blundell, 2015-09-16 20:19:42]

Hi Avi,

Could you comment on the comment that sky@ left on panel_host.cc re: the idea of
lazy instantiation of a WebContentsUserData? Thanks!

https://codereview.chromium.org/1321713005/diff/140001/chrome/browser/ui/pane...
File chrome/browser/ui/panels/panel_host.cc (right):

https://codereview.chromium.org/1321713005/diff/140001/chrome/browser/ui/pane...
chrome/browser/ui/panels/panel_host.cc:70:
sessions::ContentOpenTab::CreateForWebContents(web_contents_.get());
On 2015/09/16 20:04:53, sky wrote:
> It would be nice not to have to do this in a bunch of different place. How
about
> ContentOpenTab::Get() that does the creation if necessary?

AFAIK lazy initialization isn't done for WebContentsUserData, although it is
what's done for KeyedService. Adding avi@ for his opinion.

[sky, 2015-09-16 20:48:16]

To be clear I'm not asking to change the policy of
WebContentsUserData. Rather ContentOpenTab has a static Get function
that does the creation (if necessary).

  -Scott

On Wed, Sep 16, 2015 at 1:19 PM,  <blundell@chromium.org> wrote:
> Hi Avi,
>
> Could you comment on the comment that sky@ left on panel_host.cc re: the
> idea of
> lazy instantiation of a WebContentsUserData? Thanks!
>
>
>
https://codereview.chromium.org/1321713005/diff/140001/chrome/browser/ui/pane...
> File chrome/browser/ui/panels/panel_host.cc (right):
>
>
https://codereview.chromium.org/1321713005/diff/140001/chrome/browser/ui/pane...
> chrome/browser/ui/panels/panel_host.cc:70:
> sessions::ContentOpenTab::CreateForWebContents(web_contents_.get());
> On 2015/09/16 20:04:53, sky wrote:
>>
>> It would be nice not to have to do this in a bunch of different place.
>
> How about
>>
>> ContentOpenTab::Get() that does the creation if necessary?
>
>
> AFAIK lazy initialization isn't done for WebContentsUserData, although
> it is what's done for KeyedService. Adding avi@ for his opinion.
>
> https://codereview.chromium.org/1321713005/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blundell, 2015-09-16 20:58:45]

On 2015/09/16 20:48:16, sky wrote:
> To be clear I'm not asking to change the policy of
> WebContentsUserData. Rather ContentOpenTab has a static Get function
> that does the creation (if necessary).
> 
>   -Scott
> 

Understood. What I meant is that AFAIK there is no WebContentsUserData subclass
that works this way. The only problem that I can see with it personally is that
WebContentsUserData defines the FromWebContents() method, which is the
conventional way of getting a WCUD from the WebContents and does *not*
instantiate the WCUD if it's not present. So if making this change the way to
make it IMO would be to eliminate ContentOpenTab being a WCUD entirely and
instead just have it be a base::SupportsUserData::Data on WebContents that is
essentially a WCUD, but has lazy instantiation. I'm fine with that, but I wonder
if there's some reason that WebContentsUserData doesn't work that way in the
first place that means it would also not be a good idea in this instance.

> On Wed, Sep 16, 2015 at 1:19 PM,  <mailto:blundell@chromium.org> wrote:
> > Hi Avi,
> >
> > Could you comment on the comment that sky@ left on panel_host.cc re: the
> > idea of
> > lazy instantiation of a WebContentsUserData? Thanks!
> >
> >
> >
>
https://codereview.chromium.org/1321713005/diff/140001/chrome/browser/ui/pane...
> > File chrome/browser/ui/panels/panel_host.cc (right):
> >
> >
>
https://codereview.chromium.org/1321713005/diff/140001/chrome/browser/ui/pane...
> > chrome/browser/ui/panels/panel_host.cc:70:
> > sessions::ContentOpenTab::CreateForWebContents(web_contents_.get());
> > On 2015/09/16 20:04:53, sky wrote:
> >>
> >> It would be nice not to have to do this in a bunch of different place.
> >
> > How about
> >>
> >> ContentOpenTab::Get() that does the creation if necessary?
> >
> >
> > AFAIK lazy initialization isn't done for WebContentsUserData, although
> > it is what's done for KeyedService. Adding avi@ for his opinion.
> >
> > https://codereview.chromium.org/1321713005/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[blundell, 2015-09-17 09:40:29]

On 2015/09/16 20:58:45, blundell wrote:
> On 2015/09/16 20:48:16, sky wrote:
> > To be clear I'm not asking to change the policy of
> > WebContentsUserData. Rather ContentOpenTab has a static Get function
> > that does the creation (if necessary).
> > 
> >   -Scott
> > 
> 
> Understood. What I meant is that AFAIK there is no WebContentsUserData
subclass
> that works this way. The only problem that I can see with it personally is
that
> WebContentsUserData defines the FromWebContents() method, which is the
> conventional way of getting a WCUD from the WebContents and does *not*
> instantiate the WCUD if it's not present. So if making this change the way to
> make it IMO would be to eliminate ContentOpenTab being a WCUD entirely and
> instead just have it be a base::SupportsUserData::Data on WebContents that is
> essentially a WCUD, but has lazy instantiation. I'm fine with that, but I
wonder
> if there's some reason that WebContentsUserData doesn't work that way in the
> first place that means it would also not be a good idea in this instance.
> 

Actually, I have to apologize: I think that this whole discussion is moot due to
a careful analysis of which WebContents actually need ContentOpenTab (spoiler:
it's only those for which AttachTabHelpers() is called). I had naively assumed
that it would be needed everywhere that SessionTabHelper is needed, but that
looks to be not at all the case:

- The only callers of ContentOpenTab::FromWebContents() are TabAndroid,
BrowserTabStripModelDelegate, and BrowserTabRestoreServiceDelegate.
- All WebContents that go through TabAndroid and BrowserTabStripModelDelegate
have had AttachTabHelpers() called on them, based on my understanding of the
comment at the top of tab_helpers.h.
- The WebContents instances in BrowserTabRestoreServiceDelegate have been
created via calls to the functions in browser_tabrestore.*, which call
TabStripModel::InsertWebContentsAt(), which also ends up calling
AttachTabHelpers() via the tab strip model delegate.
- Hence, the only place that ContentOpenTab::CreateForWebContents() actually
needs to be called is in AttachTabHelpers(); all the other calls to that method
that I added are unnecessary and can be removed.

Avi, does this reasoning look correct to you? Thanks!

> > On Wed, Sep 16, 2015 at 1:19 PM,  <mailto:blundell@chromium.org> wrote:
> > > Hi Avi,
> > >
> > > Could you comment on the comment that sky@ left on panel_host.cc re: the
> > > idea of
> > > lazy instantiation of a WebContentsUserData? Thanks!
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/1321713005/diff/140001/chrome/browser/ui/pane...
> > > File chrome/browser/ui/panels/panel_host.cc (right):
> > >
> > >
> >
>
https://codereview.chromium.org/1321713005/diff/140001/chrome/browser/ui/pane...
> > > chrome/browser/ui/panels/panel_host.cc:70:
> > > sessions::ContentOpenTab::CreateForWebContents(web_contents_.get());
> > > On 2015/09/16 20:04:53, sky wrote:
> > >>
> > >> It would be nice not to have to do this in a bunch of different place.
> > >
> > > How about
> > >>
> > >> ContentOpenTab::Get() that does the creation if necessary?
> > >
> > >
> > > AFAIK lazy initialization isn't done for WebContentsUserData, although
> > > it is what's done for KeyedService. Adding avi@ for his opinion.
> > >
> > > https://codereview.chromium.org/1321713005/
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[blundell, 2015-09-17 10:10:29]

Patchset #10 (id:170001) has been deleted

[blundell, 2015-09-17 10:14:48]

Patchset #10 (id:190001) has been deleted

[blundell, 2015-09-17 11:28:10]

Patchset #10 (id:210001) has been deleted

[blundell, 2015-09-17 11:52:25]

Ready for re-review

https://codereview.chromium.org/1321713005/diff/140001/chrome/browser/ui/pane...
File chrome/browser/ui/panels/panel_host.cc (right):

https://codereview.chromium.org/1321713005/diff/140001/chrome/browser/ui/pane...
chrome/browser/ui/panels/panel_host.cc:70:
sessions::ContentOpenTab::CreateForWebContents(web_contents_.get());
On 2015/09/16 20:19:42, blundell wrote:
> On 2015/09/16 20:04:53, sky wrote:
> > It would be nice not to have to do this in a bunch of different place. How
> about
> > ContentOpenTab::Get() that does the creation if necessary?
> 
> AFAIK lazy initialization isn't done for WebContentsUserData, although it is
> what's done for KeyedService. Adding avi@ for his opinion.

Removed all the unnecessary callsites, leaving only one in tab_helpers.cc. Does
that eliminate this concern?

https://codereview.chromium.org/1321713005/diff/140001/components/sessions/co...
File components/sessions/content/content_open_tab.h (right):

https://codereview.chromium.org/1321713005/diff/140001/components/sessions/co...
components/sessions/content/content_open_tab.h:46: friend class
content::WebContentsUserData<ContentOpenTab>;
On 2015/09/16 20:04:53, sky wrote:
> nit: generally friend decls are before functions (including
> constructor/destructor).

Done.

https://codereview.chromium.org/1321713005/diff/140001/components/sessions/co...
File components/sessions/core/open_tab.h (right):

https://codereview.chromium.org/1321713005/diff/140001/components/sessions/co...
components/sessions/core/open_tab.h:18: class SESSIONS_EXPORT OpenTab {
On 2015/09/16 20:04:53, sky wrote:
> I'm not too thrilled about this name. It reads as if it's a verb, when that
> isn't the case. LiveTab maybe?

Done.

https://codereview.chromium.org/1321713005/diff/140001/components/sessions/co...
components/sessions/core/open_tab.h:33: // Returs the user agent override, if
any.
On 2015/09/16 20:04:53, sky wrote:
> Returns

Done.

[Avi (use Gerrit), 2015-09-17 15:08:25]

Comments:

- It's completely OK to not use WCUD and to fall back to UserData directly.
There already are tab helpers that do that.

- The way to think about "AttachTabHelpers" is this: it is called for tabs in a
browser window:

(A WebContents that is used as a tab in a browser window) == (A WebContents that
has AttachTabHelpers called on it)

It seems to me that tab restoration only is meaningful for WebContents used as
tabs in a browser window (almost tautologically), so that is where you want to
CreateForWebContents and you should create it nowhere else.

[blundell, 2015-09-17 15:32:03]

On 2015/09/17 15:08:25, Avi wrote:
> Comments:
> 
> - It's completely OK to not use WCUD and to fall back to UserData directly.
> There already are tab helpers that do that.
> 
> - The way to think about "AttachTabHelpers" is this: it is called for tabs in
a
> browser window:
> 
> (A WebContents that is used as a tab in a browser window) == (A WebContents
that
> has AttachTabHelpers called on it)
> 
> It seems to me that tab restoration only is meaningful for WebContents used as
> tabs in a browser window (almost tautologically), so that is where you want to
> CreateForWebContents and you should create it nowhere else.

Thanks, Avi!

[sky, 2015-09-17 16:51:10]

LGTM

[blundell, 2015-09-17 18:15:18]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2015-09-17 18:16:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1321713005/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1321713005/230001

[commit-bot: I haz the power, 2015-09-17 18:22:19]

Committed patchset #10 (id:230001)

[commit-bot: I haz the power, 2015-09-17 18:24:00]

Patchset 10 (id:??) landed as
https://crrev.com/80c5b2a0321d38d4af5761c7078c8972828eaa31
Cr-Commit-Position: refs/heads/master@{#349454}