Remove TabContents creation from ShellWindow.

Remove TabContents creation from ShellWindow.

BUG=107201, 151493
TEST=no visible change


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163322

[benwells, 2012-10-19 01:51:24]

https://codereview.chromium.org/11222003/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/shell_window.cc (left):

https://codereview.chromium.org/11222003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.cc:176:
infobar_helper->set_infobars_enabled(false);
Will this re-enable infobars? Or does not having a TabContents disable infobars?

[Avi (use Gerrit), 2012-10-19 01:59:01]

https://codereview.chromium.org/11222003/diff/1/chrome/browser/ui/extensions/...
File chrome/browser/ui/extensions/shell_window.cc (left):

https://codereview.chromium.org/11222003/diff/1/chrome/browser/ui/extensions/...
chrome/browser/ui/extensions/shell_window.cc:176:
infobar_helper->set_infobars_enabled(false);
Look at it this way.

A TabContents just is a WebContents + a zillion TabHelpers. Rather than creating
a TabContents (creating a WebContents plus lots of TabHelpers you don't want)
I'm now creating a WebContents plus exactly two helpers.

There is no InfoBarTabHelper on this WebContents, because I didn't attach one.
Therefore, there won't be infobars.

You refer to "disabling" infobars. You can't disable them; for this WebContents
they never existed in the first place.

[benwells, 2012-10-19 02:03:54]

Cool. lgtm then!

Thanks for doing this!

BTW, it will be good to see passing tryjobs, I'm not sure if that WebIntents
helper thing will expect a TabContents somewhere or other helpers associated
with the WebContents.

[Avi (use Gerrit), 2012-10-19 02:12:29]

Trybots indeed.

This won't even compile until the media gallery CL lands
(http://codereview.chromium.org/11138010/). Once that does, I'll run trybots and
let you know if you need to take a look again.

[Avi (use Gerrit), 2012-10-19 23:18:08]

Ben, please look again. Two changes from the trybot runs.

1. ShellWindowViews::GetWindowIcon expects there to be a FaviconTabHelper on the
shell window's WebContents, so I added one.
2. SelectFileDialogExtension::SelectFileImpl expects there to be a
SessionTabHelper on the shell window's WebContents, but I switched over to
SessionID::IdForTab which doesn't make that assumption.

[benwells, 2012-10-21 23:19:54]

lgtm

[commit-bot: I haz the power, 2012-10-22 01:15:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11222003/8002

[commit-bot: I haz the power, 2012-10-22 01:15:04]

Presubmit check for 11222003-8002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/views
    chrome/browser/ui/extensions

Presubmit checks took 1.1s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Avi (use Gerrit), 2012-10-22 01:21:18]

OtherBen: ui stuff

[Ben Goodger (Google), 2012-10-22 16:09:50]

lgtm for avi, comments for apps folk below

https://codereview.chromium.org/11222003/diff/8002/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://codereview.chromium.org/11222003/diff/8002/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.cc:105:
ConstrainedWindowTabHelper::CreateForWebContents(web_contents_.get());
qq for apps folk: why is ShellWindow using ConstrainedWindows? this is exactly
the kind of browser feature creep that I wanted to avoid by having the app
frames live outside chrome.

https://codereview.chromium.org/11222003/diff/8002/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.cc:107:
WebIntentPickerController::CreateForWebContents(web_contents_.get());
ditto all of this other stuff.

i'd like to see all of this stuff refactored instead of having the incestuous
pile grow.

[Avi (use Gerrit), 2012-10-22 16:22:09]

I can answer this partially.

There is an Apps feature called "media gallery". The UI for the media gallery is
presented in a constrained window. The creation of the
ConstrainedWindowTabHelper manages the fact that ShellWindows can host
constrained windows.

That's the proximate reason why I added those helpers. The appsv2 peeps can
answer further.

[commit-bot: I haz the power, 2012-10-22 16:23:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11222003/8002

[commit-bot: I haz the power, 2012-10-22 18:17:24]

Change committed as 163322

[sail, 2012-11-01 22:22:20]

https://chromiumcodereview.appspot.com/11222003/diff/8002/chrome/browser/ui/e...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://chromiumcodereview.appspot.com/11222003/diff/8002/chrome/browser/ui/e...
chrome/browser/ui/extensions/shell_window.cc:105:
ConstrainedWindowTabHelper::CreateForWebContents(web_contents_.get());
On 2012/10/22 16:09:50, Ben Goodger (Google) wrote:
> qq for apps folk: why is ShellWindow using ConstrainedWindows? this is exactly
> the kind of browser feature creep that I wanted to avoid by having the app
> frames live outside chrome.

Hi Ben. I think of ConstrainedWindows as a way to show a modal dialog on top of
web contents.

One way this is used today is that an extension API will make a call that needs
to have a dialog shown. The API is usually associated with a web contents. We
use this web contents to parent the dialog (either on a tab or a shell window).

Does this seem reasonable?

[Ben Goodger (Google), 2012-11-01 22:26:04]

I think showing a modal dialog for a WebContents is a reasonable feature
but should not be chrome-specific. I proposed a fix for this here:
http://code.google.com/p/chromium/issues/detail?id=157161

We've just about reached the limit of how much chrome dependencies I'm
willing to tolerate this apps stuff having in chrome. You should start
planning on fixing this stuff soon.

-Ben


On Thu, Nov 1, 2012 at 3:22 PM, <sail@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/11222003/diff/**
>
8002/chrome/browser/ui/**extensions/shell_window.cc<https://chromiumcodereview.appspot.com/11222003/diff/8002/chrome/browser/ui/extensions/shell_window.cc>
> File chrome/browser/ui/extensions/**shell_window.cc (right):
>
> https://chromiumcodereview.**appspot.com/11222003/diff/**
>
8002/chrome/browser/ui/**extensions/shell_window.cc#**newcode105<https://chromiumcodereview.appspot.com/11222003/diff/8002/chrome/browser/ui/extensions/shell_window.cc#newcode105>
> chrome/browser/ui/extensions/**shell_window.cc:105:
> ConstrainedWindowTabHelper::**CreateForWebContents(web_**contents_.get());
> On 2012/10/22 16:09:50, Ben Goodger (Google) wrote:
>
>> qq for apps folk: why is ShellWindow using ConstrainedWindows? this is
>>
> exactly
>
>> the kind of browser feature creep that I wanted to avoid by having the
>>
> app
>
>> frames live outside chrome.
>>
>
> Hi Ben. I think of ConstrainedWindows as a way to show a modal dialog on
> top of web contents.
>
> One way this is used today is that an extension API will make a call
> that needs to have a dialog shown. The API is usually associated with a
> web contents. We use this web contents to parent the dialog (either on a
> tab or a shell window).
>
> Does this seem reasonable?
>
>
https://chromiumcodereview.**appspot.com/11222003/<https://chromiumcodereview...
>