Preferring the content:: types except where necessary

[Ben Goodger (Google)]

With the content split, we've split various types (WebContents vs TabContents, BrowserContext vs Profile).

In some cases this has created some additional API overhead, e.g. in Browser, where we have stuff like this:

  TabContents* GetActiveTabContents() const;

  content::WebContents* GetActiveWebContents() const;

  TabContents* GetTabContentsAt(int index) const;

  content::WebContents* GetWebContentsAt(int index) const;

My proposal is that we always prefer the content:: types, except where we need to obtain the underlying wrapper to pull something off it.

So in this case, Browser would only present content::WebContents*, and someone that wanted to access a helper would have to do TabContents::FromWebContent(wc)->foo_helper()->Bar();

This keeps the API/code nice and simple, and makes it easier to pull stuff out of src/chrome into components as needed.

Beyond WebContents, I'd also like to see this happen for Profile. The ProfileKeyedService refactor has helped a great deal, but we're reaching a point where I think we could potentially take one step further and just use content::BrowserContext* in most places, only grabbing the profile where absolutely necessary.

WDYT?

-Ben

[John Abd-El-Malek]

sgtm

[Avi Drissman]

That seem to be to be a reversal from the stance we were taking.

(I'll use modern terminology here, re WebContents/TabContents.)

When we wrapped WebContents at first, I thought the plan was to make Browser own TabContentses which owned WebContentses (which we did), keeping the accessors to WebContentses only as a convenience. That was the whole point of the ugliness of GetCurrentWrapperForTabContents, that TabContentses should be what's being passed around.

On Mon, Jun 18, 2012 at 7:26 PM, Ben Goodger (Google) <b...@chromium.org> wrote:

My proposal is that we always prefer the content:: types, except where we need to obtain the underlying wrapper to pull something off it.

I'll assume "underlying wrapper" is a typo.

Yes, we need to distinguish between what's a tab (thus TabContents) and what's just a raw WebContents, but blurring things doesn't look to me to be that helpful. It smacks of bad RTTI: "Is this a WebContents or a TabContents? Let's try dynamic_cast<>ing it: FromWebContents(). Oh! It's really a TabContents."

As opposed to using TabContents where the has-a relationship is clear.

In a different email you said something that broadens the conversation:

TabContents is just something that manages access to and lifetime of WebContents-scoped helpers.

That's not what TabContents is. That's a possible future direction, but that's not what it is now. Let me rephrase it to have it match how I see reality:

TabContents is just something that manages access to and lifetime of the WebContents-scoped helpers needed for hosting the WebContents in a tab.

The point of a TabContents is that it holds a collection of WebContents-scoped helper classes that are needed to support the WebContents used in a browser tab.

Why is it used all over the place outside the use of tabs? Background printing uses TabContents. Does it need all the random support TabHelpers that come with TabContents? The print preview tab apparently uses TabContents. Same question.

When we split TabContents/WebContents, we went too far. We made too many things own TabContentses, and now we need to pay the price and get them to working with just a WebContents.

Avi

[Ben Goodger (Google)]

On Mon, Jun 18, 2012 at 7:22 PM, Avi Drissman <a...@chromium.org> wrote:

That seem to be to be a reversal from the stance we were taking.

(I'll use modern terminology here, re WebContents/TabContents.)

When we wrapped WebContents at first, I thought the plan was to make Browser own TabContentses which owned WebContentses (which we did), keeping the accessors to WebContentses only as a convenience. That was the whole point of the ugliness of GetCurrentWrapperForTabContents, that TabContentses should be what's being passed around.

To me it comes down to what API is the most often used. In my observation, it's the WC API that is most often used, and the WC type that is most often expected. Where functions take TC, my experience has been that that has been a hindrance (e.g. places like WebDialog*, which were converted to use just WC). 

 

TabContents is just something that manages access to and lifetime of WebContents-scoped helpers.

That's not what TabContents is. That's a possible future direction, but that's not what it is now. Let me rephrase it to have it match how I see reality:

TabContents is just something that manages access to and lifetime of the WebContents-scoped helpers needed for hosting the WebContents in a tab.

Sure.

 

Why is it used all over the place outside the use of tabs? Background printing uses TabContents. Does it need all the random support TabHelpers that come with TabContents? The print preview tab apparently uses TabContents. Same question.

When we split TabContents/WebContents, we went too far. We made too many things own TabContentses, and now we need to pay the price and get them to working with just a WebContents.

I agree that TC is overused, and that's what I'm reacting to. It is pervasive in too many APIs. It's another example of a proliferation of wrapper types in Chrome (Profile, a variety of Views classes etc being other examples) serve to make our system convoluted.

-Ben

[Avi Drissman]

The overuse of TabContents is something I find problematic too, but reducing everything to using WebContents with the occasional down-cast to TabContents is something I can't stomach.

The problem as I see it is that our code, all over the place, is still written with the assumption that a WebContents is in a tab. So it constantly needs to switch to TabContents, check if it's null, crash if it forgets to, etc. Code shouldn't be listening to Notifications::AllSources. It should be dealing with the types that it is: TabContents if it really is a tab, WebContents if it isn't. If it needs just one helper (like that login code the other week that needed a password manager) it should make its own wrapper. Even better, it should subclass PropertyBag::Prop, and attach all the random crap that it needs to that.

I don't see treating everything as WebContents as the right balance.

Avi