Pull shell window stuff out of ExtensionHost and put in ShellWindow

Pull shell window stuff out of ExtensionHost and put in ShellWindow

This will let us iterate quicker on ShellWindow and platform apps and let them do things (like have TabContentsWrappers) that normal ExtensionHost windows shouldn't do.

BUG=None
TEST=Platform apps still work, browser tests.

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

[benwells, 2012-04-18 08:34:58]

This is a rough version of splitting out PlatformAppHost. It needs the changes
in http://codereview.chromium.org/10116017/ to not break stuff.

Is this in the right direction? I'm wondering if ExtensionView should also be
split up but it seems pretty light (the gtk one, anyway).

https://chromiumcodereview.appspot.com/10119003/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/base_extension_host.h (right):

https://chromiumcodereview.appspot.com/10119003/diff/1/chrome/browser/extensi...
chrome/browser/extensions/base_extension_host.h:31: // extension type specific
functionality delegated out.
Or maybe the  ExtensionView stuff should be split out as well, I think its the
only reason I created this class.

Then again, it is pretty cruddy stuff to have in two places with all the #ifs.

https://chromiumcodereview.appspot.com/10119003/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://chromiumcodereview.appspot.com/10119003/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_process_manager.cc:14: #include
"chrome/browser/extensions/platform_app_host.h"
Oops

https://chromiumcodereview.appspot.com/10119003/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_process_manager.h (right):

https://chromiumcodereview.appspot.com/10119003/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_process_manager.h:23: class PlatformAppHost;
Oops.

[benwells, 2012-04-18 12:08:44]

https://chromiumcodereview.appspot.com/10119003/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/base_extension_host.h (right):

https://chromiumcodereview.appspot.com/10119003/diff/1/chrome/browser/extensi...
chrome/browser/extensions/base_extension_host.h:32: class BaseExtensionHost {
If this class stays it could have more basic functionality pushed up from the
two subclasses (e.g. extension info and the implementations of the WebContents,
RVH and RPH accessors)

https://chromiumcodereview.appspot.com/10119003/diff/1/chrome/browser/ui/exte...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://chromiumcodereview.appspot.com/10119003/diff/1/chrome/browser/ui/exte...
chrome/browser/ui/extensions/shell_window.cc:94: new PlatformAppHost(profile,
extension, url));
I've tested this gets deleted when the shell window is closed, but I haven't yet
investigated the magic which makes this miracle of deletion happen.

[Matt Perry, 2012-04-18 19:00:00]

https://chromiumcodereview.appspot.com/10119003/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/base_extension_host.h (right):

https://chromiumcodereview.appspot.com/10119003/diff/1/chrome/browser/extensi...
chrome/browser/extensions/base_extension_host.h:31: // extension type specific
functionality delegated out.
The delegate approach actually sounds like a good idea to me... IMO the reason
ExtensionHost has stuck around so long is that it has a bunch of generic shared
code to implement various delegate/observer interfaces. If we went the delegate
approach, we could just pull out the extension/app/whatever specific stuff into
delegates and keep the common code in ExtensionHost.

https://chromiumcodereview.appspot.com/10119003/diff/1/chrome/browser/extensi...
chrome/browser/extensions/base_extension_host.h:71: virtual
content::WebContents* host_contents() const = 0;
underscore_style() is reserved for lightweight inline methods. These have to be
GetHostContents()

[Aaron Boodman, 2012-04-18 21:33:22]

I discussed this at length with Matt today at lunch.

I still think we should kill ExtensionHost. ExtensionHost is essentially a
"WebContentsDelegateExtensionHelper" - a common implementation of many parts of
WebContentsDelegate that is specialized for extensions. The problem is that
WebContentsDelegate implementations don't really vary by whether they host
extension content. They vary by what type of container they are (a tab, a panel,
an infobar, etc). So an extension-specialized implementation is not useful.

If the interface to WebContentsDelegate is becoming burdensome to implement, we
should fix that directly, but it's an orthogonal problem.

It seems like all that is left of BaseExtensionHost is the stuff around
ExtensionView. I think that PlatformAppShell itself should play the role
ExtensionView currently is. Take a look at the panel_view_aura.cc for an example
of what I'm talking about here.

https://chromiumcodereview.appspot.com/10119003/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/platform_app_host.cc (right):

https://chromiumcodereview.appspot.com/10119003/diff/1/chrome/browser/extensi...
chrome/browser/extensions/platform_app_host.cc:163: if (extension_ ==
Instead of comparing the extension that is unloaded to the one you stored, you
can just look at the URL you are displaying.

https://chromiumcodereview.appspot.com/10119003/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/platform_app_host.h (right):

https://chromiumcodereview.appspot.com/10119003/diff/1/chrome/browser/extensi...
chrome/browser/extensions/platform_app_host.h:95: const Extension* extension_;
Seems that you do not need to store extension_ or extension_id_.

[benwells, 2012-04-19 09:03:54]

Still WIP.

BaseExtensionHost is gone.

Platform apps don't use ExtensionView anymore but that probably won't fly for
anything but gtk.

http://codereview.chromium.org/10119003/diff/7001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/10119003/diff/7001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_process_manager.cc:8: #include
"chrome/browser/extensions/base_extension_host.h"
Woops.

http://codereview.chromium.org/10119003/diff/7001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_process_manager.cc:14: #include
"chrome/browser/extensions/platform_app_host.h"
Another woops.

http://codereview.chromium.org/10119003/diff/7001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_process_manager.h (right):

http://codereview.chromium.org/10119003/diff/7001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_process_manager.h:23: class PlatformAppHost;
Dang. This can go.

http://codereview.chromium.org/10119003/diff/7001/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_host.cc (right):

http://codereview.chromium.org/10119003/diff/7001/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_host.cc:114: void
PlatformAppHost::CreateRenderViewSoon() {
This can move into constructor.

http://codereview.chromium.org/10119003/diff/7001/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_host.cc:153: view_->DidStopLoading();
I think this means I need to bring some sort of view class back. Will check it
on Windows.

http://codereview.chromium.org/10119003/diff/7001/chrome/browser/extensions/p...
File chrome/browser/extensions/platform_app_host.h (right):

http://codereview.chromium.org/10119003/diff/7001/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_host.h:51: Profile* profile() const {
return profile_; }
Profile can probably go too.

http://codereview.chromium.org/10119003/diff/7001/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_host.h:94: scoped_ptr<PrefsTabHelper>
prefs_tab_helper_;
I'm not sure yet what the PrefsTabHelper does. Maybe this can go too.

http://codereview.chromium.org/10119003/diff/7001/chrome/browser/extensions/p...
chrome/browser/extensions/platform_app_host.h:105: GURL initial_url_;
This doesn't need to be stored here.

[Aaron Boodman, 2012-04-20 07:05:53]

https://chromiumcodereview.appspot.com/10119003/diff/7001/chrome/browser/exte...
File chrome/browser/extensions/platform_app_host.cc (right):

https://chromiumcodereview.appspot.com/10119003/diff/7001/chrome/browser/exte...
chrome/browser/extensions/platform_app_host.cc:77:
associated_web_contents_(NULL),
This is always NULL and can be removed, which means that ::Observe can also be
removed.

https://chromiumcodereview.appspot.com/10119003/diff/7001/chrome/browser/exte...
chrome/browser/extensions/platform_app_host.cc:90: render_view_host_ =
host_contents_->GetRenderViewHost();
Why do you need to store the rvh pointer?

https://chromiumcodereview.appspot.com/10119003/diff/7001/chrome/browser/exte...
chrome/browser/extensions/platform_app_host.cc:153: view_->DidStopLoading();
On 2012/04/19 09:03:55, benwells wrote:
> I think this means I need to bring some sort of view class back. Will check it
> on Windows.

ShellWindow is essentially that object. So that's a reason why it would be
convenient to combine the two objects.

But also, I'm not sure you even need this. This is here because of some specific
needs that the extension popup has which I'm not sure shell windows do. Do popup
windows shrinkwrap the size of their contents?

https://chromiumcodereview.appspot.com/10119003/diff/7001/chrome/browser/exte...
chrome/browser/extensions/platform_app_host.cc:180: void
PlatformAppHost::RenderViewDeleted(RenderViewHost* render_view_host) {
Don't think this is needed?

https://chromiumcodereview.appspot.com/10119003/diff/7001/chrome/browser/exte...
File chrome/browser/extensions/platform_app_host.h (right):

https://chromiumcodereview.appspot.com/10119003/diff/7001/chrome/browser/exte...
chrome/browser/extensions/platform_app_host.h:35: class PlatformAppHost : public
content::WebContentsDelegate,
Are you sure you don't want to just combine this with ShellWindow?

It isn't clear to me what the difference in responsibilities are between the two
classes. It seems like it would simplify many things to combine them.

[benwells, 2012-04-24 08:35:32]

Platform app host gone.

After EPM refactor lands I'll update this patch to create some sort of
per-profile PlatformShell registry to keep the platform app browser tests
working.

https://chromiumcodereview.appspot.com/10119003/diff/7001/chrome/browser/exte...
File chrome/browser/extensions/platform_app_host.cc (right):

https://chromiumcodereview.appspot.com/10119003/diff/7001/chrome/browser/exte...
chrome/browser/extensions/platform_app_host.cc:77:
associated_web_contents_(NULL),
On 2012/04/20 07:05:53, Aaron Boodman wrote:
> This is always NULL and can be removed, which means that ::Observe can also be
> removed.

Done.

https://chromiumcodereview.appspot.com/10119003/diff/7001/chrome/browser/exte...
chrome/browser/extensions/platform_app_host.cc:90: render_view_host_ =
host_contents_->GetRenderViewHost();
On 2012/04/20 07:05:53, Aaron Boodman wrote:
> Why do you need to store the rvh pointer?

Gone.

https://chromiumcodereview.appspot.com/10119003/diff/7001/chrome/browser/exte...
chrome/browser/extensions/platform_app_host.cc:180: void
PlatformAppHost::RenderViewDeleted(RenderViewHost* render_view_host) {
On 2012/04/20 07:05:53, Aaron Boodman wrote:
> Don't think this is needed?

Done.

https://chromiumcodereview.appspot.com/10119003/diff/7001/chrome/browser/exte...
File chrome/browser/extensions/platform_app_host.h (right):

https://chromiumcodereview.appspot.com/10119003/diff/7001/chrome/browser/exte...
chrome/browser/extensions/platform_app_host.h:35: class PlatformAppHost : public
content::WebContentsDelegate,
On 2012/04/20 07:05:53, Aaron Boodman wrote:
> Are you sure you don't want to just combine this with ShellWindow?
> 
> It isn't clear to me what the difference in responsibilities are between the
two
> classes. It seems like it would simplify many things to combine them.

Yes that sounds like a great approach. I misunderstood your suggestion before.

https://chromiumcodereview.appspot.com/10119003/diff/14001/chrome/browser/ui/...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.cc (right):

https://chromiumcodereview.appspot.com/10119003/diff/14001/chrome/browser/ui/...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:28: const Extension*
extension = host_->extension();
This was a merge error

[jeremya, 2012-04-25 02:21:37]

just fyi: when I did this refactor originally, I tried the 'put everything in
ShellWindow' approach first, but mihai suggested i create a PlatformAppHost
separately so that the one host might work for both platform app background
pages and shell windows.

[benwells, 2012-04-25 04:03:37]

On 2012/04/25 02:21:37, jeremya wrote:
> just fyi: when I did this refactor originally, I tried the 'put everything in
> ShellWindow' approach first, but mihai suggested i create a PlatformAppHost
> separately so that the one host might work for both platform app background
> pages and shell windows.

After cutting out all the stuff that wasn't needed the 'host' part of
ShellWindow is pretty small, almost trivial.

At the moment platform app background pages have normal ExtensionHosts, which
feels right as they are the same as normal background pages. If they diverge
they can get their own host class like PanelHost, BalloonHost etc.

[Matt Perry, 2012-04-25 19:31:58]

https://chromiumcodereview.appspot.com/10119003/diff/19001/chrome/browser/ui/...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://chromiumcodereview.appspot.com/10119003/diff/19001/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:152:
web_contents_->GetRenderViewHost()->SyncRendererPrefs();
This line is a good example of why I'm reluctant to pull apart ExtensionHost
into multiple things. This one line represents several hours of debugging to
figure out why selected text in file dialogs on chrome OS were white on white.
How many other lines are like that? Pulling apart ExtensionHost has 2 main
disadvantages:
1. It's not immediately obvious what bits are needed when breaking apart the
class. What if we had omitted this line when we first created ShellWindow? Would
we have noticed right away? If not, there goes another couple hours when
something subtle breaks.
2. If we end up with multiple copies of code that's basically "configure this
web contents to be sane", those copies are going to diverge as we change exactly
how best to configure it, creating more work.

I agree with the complaint that ExtensionHost is the wrong abstraction - these
"configure this web contents to be sane" bits are not really specific to
extensions. But I think splitting it apart into multiple duplicate copies is a
step in the wrong direction. We need some shared object or piece of code that is
extension agnostic.

[Matt Perry, 2012-04-25 22:34:59]

https://chromiumcodereview.appspot.com/10119003/diff/19001/chrome/browser/ui/...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://chromiumcodereview.appspot.com/10119003/diff/19001/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:152:
web_contents_->GetRenderViewHost()->SyncRendererPrefs();
On 2012/04/25 19:31:58, Matt Perry wrote:
> This line is a good example of why I'm reluctant to pull apart ExtensionHost
> into multiple things. This one line represents several hours of debugging to
> figure out why selected text in file dialogs on chrome OS were white on white.
> How many other lines are like that? Pulling apart ExtensionHost has 2 main
> disadvantages:
> 1. It's not immediately obvious what bits are needed when breaking apart the
> class. What if we had omitted this line when we first created ShellWindow?
Would
> we have noticed right away? If not, there goes another couple hours when
> something subtle breaks.
> 2. If we end up with multiple copies of code that's basically "configure this
> web contents to be sane", those copies are going to diverge as we change
exactly
> how best to configure it, creating more work.
> 
> I agree with the complaint that ExtensionHost is the wrong abstraction - these
> "configure this web contents to be sane" bits are not really specific to
> extensions. But I think splitting it apart into multiple duplicate copies is a
> step in the wrong direction. We need some shared object or piece of code that
is
> extension agnostic.

BTW I'm just voicing my grievances :). I realize Aaron suggested you go this
route, so I'll defer to him on this review.

[benwells, 2012-04-25 23:25:25]

http://codereview.chromium.org/10119003/diff/19001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.cc (right):

http://codereview.chromium.org/10119003/diff/19001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.cc:152:
web_contents_->GetRenderViewHost()->SyncRendererPrefs();
On 2012/04/25 22:35:00, Matt Perry wrote:
> On 2012/04/25 19:31:58, Matt Perry wrote:
> > This line is a good example of why I'm reluctant to pull apart ExtensionHost
> > into multiple things. This one line represents several hours of debugging to
> > figure out why selected text in file dialogs on chrome OS were white on
white.
> > How many other lines are like that? Pulling apart ExtensionHost has 2 main
> > disadvantages:
> > 1. It's not immediately obvious what bits are needed when breaking apart the
> > class. What if we had omitted this line when we first created ShellWindow?
> Would
> > we have noticed right away? If not, there goes another couple hours when
> > something subtle breaks.
> > 2. If we end up with multiple copies of code that's basically "configure
this
> > web contents to be sane", those copies are going to diverge as we change
> exactly
> > how best to configure it, creating more work.
> > 
> > I agree with the complaint that ExtensionHost is the wrong abstraction -
these
> > "configure this web contents to be sane" bits are not really specific to
> > extensions. But I think splitting it apart into multiple duplicate copies is
a
> > step in the wrong direction. We need some shared object or piece of code
that
> is
> > extension agnostic.
> 
> BTW I'm just voicing my grievances :). I realize Aaron suggested you go this
> route, so I'll defer to him on this review.

For context, this patch is why I want to split out ShellWindow:
http://codereview.chromium.org/10134026/. As platform apps get further refined I
think the hacks to keep them in ExtensionHost will get nastier.

I agree with the philosophy of "don't refactor unless there is a specific need,
as you can lose accumulated wisdom / bug fixes", but I think there is a specific
need here for platform app windows to be different from the other windows /
views covered by ExtensionHost. (sorry, I don't mean to start a philosophical
discussion :) ).

I don't have an opinion on whether ExtensionHost should be further split up, I
don't know enough about the other types of extensions.

[benwells, 2012-04-26 08:36:32]

This is getting closer - apart from the views and cocoa implementation, this is
now ready for review.

http://codereview.chromium.org/10119003/diff/31001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/10119003/diff/31001/chrome/browser/extensions/...
chrome/browser/extensions/extension_process_manager.cc:334:
content::Details<RenderViewHost>(render_view_host));
This is to keep the task manager from crashing if we close shell Windows while
the task manager is open.

http://codereview.chromium.org/10119003/diff/31001/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_browsertest.cc (left):

http://codereview.chromium.org/10119003/diff/31001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:81:
EXPECT_EQ(platform_app_count + 1, GetPlatformAppCount());
This check is no longer valid and I think was adding limited value.

[Aaron Boodman, 2012-04-27 16:20:31]

https://chromiumcodereview.appspot.com/10119003/diff/19001/chrome/browser/ui/...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://chromiumcodereview.appspot.com/10119003/diff/19001/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:152:
web_contents_->GetRenderViewHost()->SyncRendererPrefs();
On 2012/04/25 23:25:25, benwells wrote:
> On 2012/04/25 22:35:00, Matt Perry wrote:
> > On 2012/04/25 19:31:58, Matt Perry wrote:
> > > This line is a good example of why I'm reluctant to pull apart
ExtensionHost
> > > into multiple things. This one line represents several hours of debugging
to
> > > figure out why selected text in file dialogs on chrome OS were white on
> white.
> > > How many other lines are like that? Pulling apart ExtensionHost has 2 main
> > > disadvantages:
> > > 1. It's not immediately obvious what bits are needed when breaking apart
the
> > > class. What if we had omitted this line when we first created ShellWindow?
> > Would
> > > we have noticed right away? If not, there goes another couple hours when
> > > something subtle breaks.
> > > 2. If we end up with multiple copies of code that's basically "configure
> this
> > > web contents to be sane", those copies are going to diverge as we change
> > exactly
> > > how best to configure it, creating more work.
> > > 
> > > I agree with the complaint that ExtensionHost is the wrong abstraction -
> these
> > > "configure this web contents to be sane" bits are not really specific to
> > > extensions. But I think splitting it apart into multiple duplicate copies
is
> a
> > > step in the wrong direction. We need some shared object or piece of code
> that
> > is
> > > extension agnostic.
> > 
> > BTW I'm just voicing my grievances :). I realize Aaron suggested you go this
> > route, so I'll defer to him on this review.
> 
> For context, this patch is why I want to split out ShellWindow:
> http://codereview.chromium.org/10134026/. As platform apps get further refined
I
> think the hacks to keep them in ExtensionHost will get nastier.
> 
> I agree with the philosophy of "don't refactor unless there is a specific
need,
> as you can lose accumulated wisdom / bug fixes", but I think there is a
specific
> need here for platform app windows to be different from the other windows /
> views covered by ExtensionHost. (sorry, I don't mean to start a philosophical
> discussion :) ).
> 
> I don't have an opinion on whether ExtensionHost should be further split up, I
> don't know enough about the other types of extensions.

I hear you. We have many options when these things come up:
- If they are configuration, push them down into WebContents and give them
default values
- Implement isolated helpers (like EFD) for specific groups of behaviors
- Make the APIs WebContents exposes harder to mess up

In this case, it seems like the latter applies. GetMutableRendererPrefs() is a
trap. It doesn't actually update the renderer's prefs. It should be modified to
make it less easy to screw up, like:

WebContents::ScopedRenderPrefUpdate update;
update.render_prefs().browser_handles_all_requests = true;
// sync happens when update goes out of scope.

https://chromiumcodereview.appspot.com/10119003/diff/40004/chrome/browser/ext...
File chrome/browser/extensions/platform_app_registry.h (right):

https://chromiumcodereview.appspot.com/10119003/diff/40004/chrome/browser/ext...
chrome/browser/extensions/platform_app_registry.h:19: // The PlatformAppRegistry
tracks the ShellWindows for all platform apps for a
ShellWindowRegistry?

https://chromiumcodereview.appspot.com/10119003/diff/40004/chrome/browser/ui/...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://chromiumcodereview.appspot.com/10119003/diff/40004/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:62: namespace keys =
extension_tabs_module_constants;
This is only used in one place, just put it inline there.

https://chromiumcodereview.appspot.com/10119003/diff/40004/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:68: base::DictionaryValue*
ShellWindowController::CreateWindowValue() const {
Why override this if you're not adding behavior?

https://chromiumcodereview.appspot.com/10119003/diff/40004/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:92: ExtensionProcessManager*
manager = profile->GetExtensionProcessManager();
Unused code?

https://chromiumcodereview.appspot.com/10119003/diff/40004/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:100: return NULL;
EFD::Delegate could do this by default.

https://chromiumcodereview.appspot.com/10119003/diff/40004/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:116: bool
ShellWindow::ShouldSuppressDialogs() {
This seems like it is unnecessarily inversion of control. Consider modifying
WebContents to have set_should_surpress_dialogs(). That would make implementors
lives slightly easier. Can be separate cleanup CL.

https://chromiumcodereview.appspot.com/10119003/diff/40004/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:188: Browser*
ShellWindow::GetBrowser() {
EFD::Delegate could do this by default.

https://chromiumcodereview.appspot.com/10119003/diff/40004/chrome/browser/ui/...
File chrome/browser/ui/extensions/shell_window.h (right):

https://chromiumcodereview.appspot.com/10119003/diff/40004/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.h:49: //
ExtensionFunctionDispatcher::Delegate
None of these implemented interfaces need to be public unless you are going to
unit test them.

https://chromiumcodereview.appspot.com/10119003/diff/40004/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.h:71: // per platform). Public users
of ShellWindow should use ShellWindow::Create.
Name this CreateImpl()?

https://chromiumcodereview.appspot.com/10119003/diff/40004/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.h:72: static ShellWindow*
CreateShellWindow(Profile* profile,
Does all of this really need to be accessible to subclasses? I like it better to
only expose what is needed -- makes it easier to read code because you don't
have to wonder if things are being called/modified from other places.

[Aaron Boodman, 2012-04-27 16:21:53]

On 2012/04/27 16:20:31, Aaron Boodman wrote:
>
https://chromiumcodereview.appspot.com/10119003/diff/19001/chrome/browser/ui/...
> File chrome/browser/ui/extensions/shell_window.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10119003/diff/19001/chrome/browser/ui/...
> chrome/browser/ui/extensions/shell_window.cc:152:
> web_contents_->GetRenderViewHost()->SyncRendererPrefs();
> On 2012/04/25 23:25:25, benwells wrote:
> > On 2012/04/25 22:35:00, Matt Perry wrote:
> > > On 2012/04/25 19:31:58, Matt Perry wrote:
> > > > This line is a good example of why I'm reluctant to pull apart
> ExtensionHost
> > > > into multiple things. This one line represents several hours of
debugging
> to
> > > > figure out why selected text in file dialogs on chrome OS were white on
> > white.
> > > > How many other lines are like that? Pulling apart ExtensionHost has 2
main
> > > > disadvantages:
> > > > 1. It's not immediately obvious what bits are needed when breaking apart
> the
> > > > class. What if we had omitted this line when we first created
ShellWindow?
> > > Would
> > > > we have noticed right away? If not, there goes another couple hours when
> > > > something subtle breaks.
> > > > 2. If we end up with multiple copies of code that's basically "configure
> > this
> > > > web contents to be sane", those copies are going to diverge as we change
> > > exactly
> > > > how best to configure it, creating more work.
> > > > 
> > > > I agree with the complaint that ExtensionHost is the wrong abstraction -
> > these
> > > > "configure this web contents to be sane" bits are not really specific to
> > > > extensions. But I think splitting it apart into multiple duplicate
copies
> is
> > a
> > > > step in the wrong direction. We need some shared object or piece of code
> > that
> > > is
> > > > extension agnostic.
> > > 
> > > BTW I'm just voicing my grievances :). I realize Aaron suggested you go
this
> > > route, so I'll defer to him on this review.
> > 
> > For context, this patch is why I want to split out ShellWindow:
> > http://codereview.chromium.org/10134026/. As platform apps get further
refined
> I
> > think the hacks to keep them in ExtensionHost will get nastier.
> > 
> > I agree with the philosophy of "don't refactor unless there is a specific
> need,
> > as you can lose accumulated wisdom / bug fixes", but I think there is a
> specific
> > need here for platform app windows to be different from the other windows /
> > views covered by ExtensionHost. (sorry, I don't mean to start a
philosophical
> > discussion :) ).
> > 
> > I don't have an opinion on whether ExtensionHost should be further split up,
I
> > don't know enough about the other types of extensions.
> 
> I hear you. We have many options when these things come up:
> - If they are configuration, push them down into WebContents and give them
> default values

BTW, I've suggested several things along these lines in this review.

[Aaron Boodman, 2012-04-27 16:22:33]

One more thing I forgot to say:

Ben: Awesome change is awesome. I really like how this came together.

[benwells, 2012-05-03 09:22:40]

+mihaip as reviewer

http://codereview.chromium.org/10119003/diff/40004/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_registry.h (right):

http://codereview.chromium.org/10119003/diff/40004/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_registry.h:19: // The PlatformAppRegistry
tracks the ShellWindows for all platform apps for a
On 2012/04/27 16:20:31, Aaron Boodman wrote:
> ShellWindowRegistry?

Done.

http://codereview.chromium.org/10119003/diff/40004/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.cc (right):

http://codereview.chromium.org/10119003/diff/40004/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.cc:62: namespace keys =
extension_tabs_module_constants;
On 2012/04/27 16:20:31, Aaron Boodman wrote:
> This is only used in one place, just put it inline there.

Done.

http://codereview.chromium.org/10119003/diff/40004/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.cc:68: base::DictionaryValue*
ShellWindowController::CreateWindowValue() const {
On 2012/04/27 16:20:31, Aaron Boodman wrote:
> Why override this if you're not adding behavior?

Done.

http://codereview.chromium.org/10119003/diff/40004/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.cc:92: ExtensionProcessManager*
manager = profile->GetExtensionProcessManager();
On 2012/04/27 16:20:31, Aaron Boodman wrote:
> Unused code?

Done.

http://codereview.chromium.org/10119003/diff/40004/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.cc:100: return NULL;
On 2012/04/27 16:20:31, Aaron Boodman wrote:
> EFD::Delegate could do this by default.

Done.

http://codereview.chromium.org/10119003/diff/40004/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.cc:116: bool
ShellWindow::ShouldSuppressDialogs() {
On 2012/04/27 16:20:31, Aaron Boodman wrote:
> This seems like it is unnecessarily inversion of control. Consider modifying
> WebContents to have set_should_surpress_dialogs(). That would make
implementors
> lives slightly easier. Can be separate cleanup CL.

Added todo.

http://codereview.chromium.org/10119003/diff/40004/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.cc:188: Browser*
ShellWindow::GetBrowser() {
On 2012/04/27 16:20:31, Aaron Boodman wrote:
> EFD::Delegate could do this by default.

Done.

http://codereview.chromium.org/10119003/diff/40004/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.h (right):

http://codereview.chromium.org/10119003/diff/40004/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.h:49: //
ExtensionFunctionDispatcher::Delegate
On 2012/04/27 16:20:31, Aaron Boodman wrote:
> None of these implemented interfaces need to be public unless you are going to
> unit test them.

Done. I didn't even realize you could do this ...

http://codereview.chromium.org/10119003/diff/40004/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.h:71: // per platform). Public users
of ShellWindow should use ShellWindow::Create.
On 2012/04/27 16:20:31, Aaron Boodman wrote:
> Name this CreateImpl()?

Done.

http://codereview.chromium.org/10119003/diff/40004/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.h:72: static ShellWindow*
CreateShellWindow(Profile* profile,
On 2012/04/27 16:20:31, Aaron Boodman wrote:
> Does all of this really need to be accessible to subclasses? I like it better
to
> only expose what is needed -- makes it easier to read code because you don't
> have to wonder if things are being called/modified from other places.

Agreed. I moved other stuff in this class to be as low as visibility as it needs
to.

[benwells, 2012-05-03 20:39:33]

Try jobs are looking pretty good, there is still some suspicious failures on
chromeos but overall this is ready for review.

[benwells, 2012-05-04 18:38:53]

ChromeOS bots are now happy. Please review!

[Matt Perry, 2012-05-05 00:51:38]

lgtm

http://codereview.chromium.org/10119003/diff/67002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/10119003/diff/67002/chrome/browser/extensions/...
chrome/browser/extensions/extension_host.cc:530:
Browser::RunFileChooserHelper(tab, params);
Can ShellWindow apps call alert() or have a file picker? If so, I think you'll
need to implement these delegate methods too.

http://codereview.chromium.org/10119003/diff/67002/chrome/browser/extensions/...
chrome/browser/extensions/extension_host.cc:533: void
ExtensionHost::AddNewContents(WebContents* source,
Can apps hosted in a ShellWindow call window.open? Or have a right-click-link
"Open In New Tab" menu item? I think one of those is handled by this method, so
you might need to implement it there.

[jeremya, 2012-05-07 00:54:37]

I'd like the ShellWindowViews stuff to be structured a little differently (as
mentioned below), but I'm happy to do that myself after this lands, so lgtm.

http://codereview.chromium.org/10119003/diff/51027/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_dispatcher.h (right):

http://codereview.chromium.org/10119003/diff/51027/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_dispatcher.h:58: virtual Browser*
GetBrowser();
This is going to conflict with stevenjb's patch:
http://codereview.chromium.org/10021071/

http://codereview.chromium.org/10119003/diff/51027/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/10119003/diff/51027/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest.cc:121: }
I wish we could write a unit test for this, instead of a massive integration
test. Not really sure how that would work, though :/

http://codereview.chromium.org/10119003/diff/51027/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.h (right):

http://codereview.chromium.org/10119003/diff/51027/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.h:18: class Browser;
This doesn't seem to be used.

http://codereview.chromium.org/10119003/diff/51027/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_views.cc (left):

http://codereview.chromium.org/10119003/diff/51027/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_views.cc:117:
AddChildView(host_->view());
If you never add a view to the widget, how does the web contents get shown?
Unless I've missed something elsewhere, you probably need to return 'this' from
GetContentsView().

I had recently changed ShellWindowViews to be a WidgetDelegateView instead of
just a WidgetDelegate in order to prepare for the custom Chrome-style frame
work. It might make more sense to keep that structure and instead of SWV
inheriting from NativeViewHost, it could own one instead.

http://codereview.chromium.org/10119003/diff/51027/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_views.h (right):

http://codereview.chromium.org/10119003/diff/51027/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_views.h:10: #include
"chrome/browser/ui/views/extensions/extension_view.h"
I don't think we need this any more.

[Aaron Boodman, 2012-05-07 02:47:44]

LGTM

[benwells, 2012-05-07 08:46:15]

Merged with stevenjb's patch. Adding reviewers:

+ estade for chrome/browser/ui/gtk
+ sail for chrome/browser/ui/cocoa
+ sky for chrome/browser/ui/views

Try jobs are failing as there is no good revision which has stevenjb's patch

http://codereview.chromium.org/10119003/diff/51027/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_dispatcher.h (right):

http://codereview.chromium.org/10119003/diff/51027/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_dispatcher.h:58: virtual Browser*
GetBrowser();
On 2012/05/07 00:54:38, jeremya wrote:
> This is going to conflict with stevenjb's patch:
> http://codereview.chromium.org/10021071/

;_;

http://codereview.chromium.org/10119003/diff/51027/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.h (right):

http://codereview.chromium.org/10119003/diff/51027/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.h:18: class Browser;
On 2012/05/07 00:54:38, jeremya wrote:
> This doesn't seem to be used.

Done.

http://codereview.chromium.org/10119003/diff/51027/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_views.cc (left):

http://codereview.chromium.org/10119003/diff/51027/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_views.cc:117:
AddChildView(host_->view());
On 2012/05/07 00:54:38, jeremya wrote:
> If you never add a view to the widget, how does the web contents get shown?
> Unless I've missed something elsewhere, you probably need to return 'this'
from
> GetContentsView().
> 
> I had recently changed ShellWindowViews to be a WidgetDelegateView instead of
> just a WidgetDelegate in order to prepare for the custom Chrome-style frame
> work. It might make more sense to keep that structure and instead of SWV
> inheriting from NativeViewHost, it could own one instead.

There is no need for the ExtensionView to be a child view any more, as this guy
effectively is the ExtensionView now. GetContentsView does and did return this.

If you want to change the structure of this stuff go for it.

http://codereview.chromium.org/10119003/diff/51027/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_views.h (right):

http://codereview.chromium.org/10119003/diff/51027/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_views.h:10: #include
"chrome/browser/ui/views/extensions/extension_view.h"
On 2012/05/07 00:54:38, jeremya wrote:
> I don't think we need this any more.

Done.

http://codereview.chromium.org/10119003/diff/67002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/10119003/diff/67002/chrome/browser/extensions/...
chrome/browser/extensions/extension_host.cc:530:
Browser::RunFileChooserHelper(tab, params);
On 2012/05/05 00:51:38, Matt Perry wrote:
> Can ShellWindow apps call alert() or have a file picker? If so, I think you'll
> need to implement these delegate methods too.

Alert is deliberately disabled for ShellWindows, I'm not sure if we want a file
picker or not. We will but possibly not like other web contents (i.e. we'll do
it via intents).

http://codereview.chromium.org/10119003/diff/67002/chrome/browser/extensions/...
chrome/browser/extensions/extension_host.cc:533: void
ExtensionHost::AddNewContents(WebContents* source,
On 2012/05/05 00:51:38, Matt Perry wrote:
> Can apps hosted in a ShellWindow call window.open? Or have a right-click-link
> "Open In New Tab" menu item? I think one of those is handled by this method,
so
> you might need to implement it there.

This behaviour is disabled for shell windows.

[sky, 2012-05-07 15:48:28]

LGTM

http://codereview.chromium.org/10119003/diff/74012/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_views.h (right):

http://codereview.chromium.org/10119003/diff/74012/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_views.h:21: explicit
ShellWindowViews(Profile* profile,
remove explicit

[sail, 2012-05-07 16:32:09]

cocoa/* LGTM!

[Evan Stade, 2012-05-08 00:58:16]

rubber stamp lgtm on gtk

http://codereview.chromium.org/10119003/diff/74012/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.h (right):

http://codereview.chromium.org/10119003/diff/74012/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.h:26: 
please add a file level comment here as I have no idea what a "shell window" is
in this case.

http://codereview.chromium.org/10119003/diff/74012/chrome/browser/ui/gtk/exte...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.h (right):

http://codereview.chromium.org/10119003/diff/74012/chrome/browser/ui/gtk/exte...
chrome/browser/ui/gtk/extensions/shell_window_gtk.h:23: explicit
ShellWindowGtk(Profile* profile,
you only need explicit when there's exactly one parameter.

[commit-bot: I haz the power, 2012-05-10 00:22:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10119003/82005

[commit-bot: I haz the power, 2012-05-10 03:45:11]

Change committed as 136239