Refactor ExtensionView to support a UI-less extension instance....

Refactor ExtensionView to support a UI-less extension instance.
- Introduce ExtensionHost, which is the guy that talks to the RenderViewHost.
- ExtensionView holds an ExtensionHost, and also renders its contents in an
  HWND.
- Added code to load a page optionally specified in the manifest as a
  background process whenever it exists.

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

[Erik does not do reviews, 2009-04-22 23:28:31]

http://codereview.chromium.org/92043/diff/1/14
File chrome/browser/browser.vcproj (right):

http://codereview.chromium.org/92043/diff/1/14#newcode1949
Line 1949: RelativePath=".\extensions\extension_host.cc"
missing .gyp file

http://codereview.chromium.org/92043/diff/1/12
File chrome/browser/extensions/extension.cc (right):

http://codereview.chromium.org/92043/diff/1/12#newcode400
Line 400: FilePath plugins_dir = path_.AppendASCII(kPluginsDirname);
so far, we've resisted having a hard-coded directory structure.  The only
requirement is a toplevel manifest.json.  Why add more structure now?

http://codereview.chromium.org/92043/diff/1/12#newcode405
Line 405: FilePath master_path = path_.AppendASCII(kMasterFilename);
same here

http://codereview.chromium.org/92043/diff/1/10
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/92043/diff/1/10#newcode158
Line 158: return view_ ? view_->browser() : BrowserList::GetLastActive();
I don't think this is safe.  You might get a browser for a different profile. 
We need a method like GetLastActiveForProfile().

http://codereview.chromium.org/92043/diff/1/10#newcode159
Line 159: }
lint says missing file at end of file

http://codereview.chromium.org/92043/diff/1/11
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/92043/diff/1/11#newcode73
Line 73: private:
indent 1 char

http://codereview.chromium.org/92043/diff/1/11#newcode94
Line 94: #endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_HOST_H_
lint says missing line at end of file

http://codereview.chromium.org/92043/diff/1/6
File chrome/browser/extensions/extension_view.cc (right):

http://codereview.chromium.org/92043/diff/1/6#newcode66
Line 66: // RenderWidgetHostHWND supports windowed plugins, but if we ever also
wanted
80 cols

http://codereview.chromium.org/92043/diff/1/6#newcode75
Line 75: #endif
add #else NOTIMPLEMENTED();

http://codereview.chromium.org/92043/diff/1/8
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/92043/diff/1/8#newcode93
Line 93: for (ExtensionHostList::iterator iter = running_extensions_.begin();
It feels a little weird to delete the host *after* the extension.  Doesn't it
hold a reference to the extension?  One way to solve this would be to have the
Extension own the ExtensionHost, so it could deal with shutdown order issues
with the resources it owns.

http://codereview.chromium.org/92043/diff/1/8#newcode211
Line 211: ExtensionHost* host =
whitespace at end of line

http://codereview.chromium.org/92043/diff/1/8#newcode212
Line 212: new ExtensionHost(extension, GetSiteInstanceForURL(url));
what if the URL doesn't exist?  Or there was an exception at startup?  Should we
have error handling here?

http://codereview.chromium.org/92043/diff/1/19
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/92043/diff/1/19#newcode97
Line 97: typedef std::list<ExtensionHost*> ExtensionHostList;
usually, I've seen this declared next to where it's used for clarity.

http://codereview.chromium.org/92043/diff/1/19#newcode123
Line 123: ExtensionHostList running_extensions_;
The name "running_extensions_" doesn't seem accurate to me (the extension could
have a view, a content script, but no host - isn't it running?).   Maybe just
extension_hosts_?

http://codereview.chromium.org/92043/diff/1/15
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/92043/diff/1/15#newcode178
Line 178: Send(new ViewMsg_New(GetPluginNativeViewId(),
what does it mean to send this message when the view id is NULL?

http://codereview.chromium.org/92043/diff/1/3
File chrome/test/data/extensions/good/extension1/1/toolstrip2.html (left):

http://codereview.chromium.org/92043/diff/1/3#oldcode18
Line 18: alert(window.extension.getTestString());
I think this alert is used by the unit test even though it does nothing in real
use.

http://codereview.chromium.org/92043/diff/1/2
File chrome/test/data/extensions/test/TabsAPI/1/manifest.json (right):

http://codereview.chromium.org/92043/diff/1/2#newcode7
Line 7: "toolstrips": ["ts.html"]
I don't see this in the CL

[Matt Perry, 2009-04-22 23:56:00]

http://codereview.chromium.org/92043/diff/1/14
File chrome/browser/browser.vcproj (right):

http://codereview.chromium.org/92043/diff/1/14#newcode1949
Line 1949: RelativePath=".\extensions\extension_host.cc"
On 2009/04/22 23:28:31, Erik Kay wrote:
> missing .gyp file

Done.

http://codereview.chromium.org/92043/diff/1/12
File chrome/browser/extensions/extension.cc (right):

http://codereview.chromium.org/92043/diff/1/12#newcode400
Line 400: FilePath plugins_dir = path_.AppendASCII(kPluginsDirname);
On 2009/04/22 23:28:31, Erik Kay wrote:
> so far, we've resisted having a hard-coded directory structure.  The only
> requirement is a toplevel manifest.json.  Why add more structure now?

I was talking to Aaron about this a while ago.  He initially suggested putting
plugins_dir in the manifest, but later decided it's pointless.  I agree; it just
seems like an extra unnecessary step.

http://codereview.chromium.org/92043/diff/1/10
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/92043/diff/1/10#newcode158
Line 158: return view_ ? view_->browser() : BrowserList::GetLastActive();
On 2009/04/22 23:28:31, Erik Kay wrote:
> I don't think this is safe.  You might get a browser for a different profile. 
> We need a method like GetLastActiveForProfile().

I added a method similar to FindBrowserWithType, but for choose-by-Profile only.

http://codereview.chromium.org/92043/diff/1/11
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/92043/diff/1/11#newcode73
Line 73: private:
On 2009/04/22 23:28:31, Erik Kay wrote:
> indent 1 char

Done.

http://codereview.chromium.org/92043/diff/1/11#newcode94
Line 94: #endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_HOST_H_
On 2009/04/22 23:28:31, Erik Kay wrote:
> lint says missing line at end of file

weird... none of the other files in this CL seem to have a trailing newline.

http://codereview.chromium.org/92043/diff/1/6
File chrome/browser/extensions/extension_view.cc (right):

http://codereview.chromium.org/92043/diff/1/6#newcode66
Line 66: // RenderWidgetHostHWND supports windowed plugins, but if we ever also
wanted
On 2009/04/22 23:28:31, Erik Kay wrote:
> 80 cols

Done.

http://codereview.chromium.org/92043/diff/1/6#newcode75
Line 75: #endif
On 2009/04/22 23:28:31, Erik Kay wrote:
> add #else NOTIMPLEMENTED();

Done.

http://codereview.chromium.org/92043/diff/1/8
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/92043/diff/1/8#newcode93
Line 93: for (ExtensionHostList::iterator iter = running_extensions_.begin();
On 2009/04/22 23:28:31, Erik Kay wrote:
> It feels a little weird to delete the host *after* the extension.  Doesn't it
> hold a reference to the extension?  One way to solve this would be to have the
> Extension own the ExtensionHost, so it could deal with shutdown order issues
> with the resources it owns.

True.  I just moved it so they're deleted first.  I don't really like having
Extension own the ExtensionHost, because
- ExtensionView owns its host (but these have no View)
- Extension represents static info about an extension, not running instances.

http://codereview.chromium.org/92043/diff/1/19
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/92043/diff/1/19#newcode97
Line 97: typedef std::list<ExtensionHost*> ExtensionHostList;
On 2009/04/22 23:28:31, Erik Kay wrote:
> usually, I've seen this declared next to where it's used for clarity.

Done.

http://codereview.chromium.org/92043/diff/1/19#newcode123
Line 123: ExtensionHostList running_extensions_;
On 2009/04/22 23:28:31, Erik Kay wrote:
> The name "running_extensions_" doesn't seem accurate to me (the extension
could
> have a view, a content script, but no host - isn't it running?).   Maybe just
> extension_hosts_?

Done.

http://codereview.chromium.org/92043/diff/1/15
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/92043/diff/1/15#newcode178
Line 178: Send(new ViewMsg_New(GetPluginNativeViewId(),
On 2009/04/22 23:28:31, Erik Kay wrote:
> what does it mean to send this message when the view id is NULL?

From what I can tell, the renderer only uses the view ID to talk back to the
browser, and we ignore it whenever view_ is NULL.

http://codereview.chromium.org/92043/diff/1/3
File chrome/test/data/extensions/good/extension1/1/toolstrip2.html (left):

http://codereview.chromium.org/92043/diff/1/3#oldcode18
Line 18: alert(window.extension.getTestString());
On 2009/04/22 23:28:31, Erik Kay wrote:
> I think this alert is used by the unit test even though it does nothing in
real
> use.

Didn't mean to include this change.

http://codereview.chromium.org/92043/diff/1/2
File chrome/test/data/extensions/test/TabsAPI/1/manifest.json (right):

http://codereview.chromium.org/92043/diff/1/2#newcode7
Line 7: "toolstrips": ["ts.html"]
On 2009/04/22 23:28:31, Erik Kay wrote:
> I don't see this in the CL

Didn't mean to include this change.

[Erik does not do reviews, 2009-04-23 00:10:13]

http://codereview.chromium.org/92043/diff/1/12
File chrome/browser/extensions/extension.cc (right):

http://codereview.chromium.org/92043/diff/1/12#newcode400
Line 400: FilePath plugins_dir = path_.AppendASCII(kPluginsDirname);
On 2009/04/22 23:56:00, Matt Perry wrote:
> On 2009/04/22 23:28:31, Erik Kay wrote:
> > so far, we've resisted having a hard-coded directory structure.  The only
> > requirement is a toplevel manifest.json.  Why add more structure now?
> 
> I was talking to Aaron about this a while ago.  He initially suggested putting
> plugins_dir in the manifest, but later decided it's pointless.  I agree; it
just
> seems like an extra unnecessary step.

I like the indirection because it gives us flexibility.  For example, I could
imagine a developer with multiple plugin dirs.  Or what about the multiplatform
case?  Or maybe we actually just want to specify the plugin file rather than a
dir.  When it's defined by the manifest, then it's clear that the version of the
manifest defines the feature set.

http://codereview.chromium.org/92043/diff/1/11
File chrome/browser/extensions/extension_host.h (right):

http://codereview.chromium.org/92043/diff/1/11#newcode94
Line 94: #endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_HOST_H_
On 2009/04/22 23:56:00, Matt Perry wrote:
> On 2009/04/22 23:28:31, Erik Kay wrote:
> > lint says missing line at end of file
> 
> weird... none of the other files in this CL seem to have a trailing newline.

That's an annoying bug in Rietveld.  It trims trailing newlines from the
display, so you can't tell just by looking at the file in the diff.

http://codereview.chromium.org/92043/diff/2001/3007
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/92043/diff/2001/3007#newcode160
Line 160: render_view_host()->process()->profile());
So what should happen if the user closes the last browser window in a profile? 
Should the ExtensionHost go away?  
In any case, this can now return NULL, which seems like a bad state for
ExtensionHost to be in.  We should either CHECK() here, or do NULL checks from
the callers. (I think the CHECK is better since this feels invalid)

[Matt Perry, 2009-04-23 21:32:36]

ready for another look

http://codereview.chromium.org/92043/diff/1/12
File chrome/browser/extensions/extension.cc (right):

http://codereview.chromium.org/92043/diff/1/12#newcode400
Line 400: FilePath plugins_dir = path_.AppendASCII(kPluginsDirname);
On 2009/04/23 00:10:13, Erik Kay wrote:
> On 2009/04/22 23:56:00, Matt Perry wrote:
> > On 2009/04/22 23:28:31, Erik Kay wrote:
> > > so far, we've resisted having a hard-coded directory structure.  The only
> > > requirement is a toplevel manifest.json.  Why add more structure now?
> > 
> > I was talking to Aaron about this a while ago.  He initially suggested
putting
> > plugins_dir in the manifest, but later decided it's pointless.  I agree; it
> just
> > seems like an extra unnecessary step.
> 
> I like the indirection because it gives us flexibility.  For example, I could
> imagine a developer with multiple plugin dirs.  Or what about the
multiplatform
> case?  Or maybe we actually just want to specify the plugin file rather than a
> dir.  When it's defined by the manifest, then it's clear that the version of
the
> manifest defines the feature set.

OK, I took that part out of this CL.  We can always change it if we come to
agreement later.

http://codereview.chromium.org/92043/diff/2001/3007
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/92043/diff/2001/3007#newcode160
Line 160: render_view_host()->process()->profile());
On 2009/04/23 00:10:14, Erik Kay wrote:
> So what should happen if the user closes the last browser window in a profile?

> Should the ExtensionHost go away?  
> In any case, this can now return NULL, which seems like a bad state for
> ExtensionHost to be in.  We should either CHECK() here, or do NULL checks from
> the callers. (I think the CHECK is better since this feels invalid)

Added a CHECK.  We should destroy the hosts when all Browsers for that profile
close (though we don't yet).

[Erik does not do reviews, 2009-04-23 21:43:33]

LGTM

http://codereview.chromium.org/92043/diff/2001/3007
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/92043/diff/2001/3007#newcode160
Line 160: render_view_host()->process()->profile());
On 2009/04/23 21:32:36, Matt Perry wrote:
> On 2009/04/23 00:10:14, Erik Kay wrote:
> > So what should happen if the user closes the last browser window in a
profile?
> 
> > Should the ExtensionHost go away?  
> > In any case, this can now return NULL, which seems like a bad state for
> > ExtensionHost to be in.  We should either CHECK() here, or do NULL checks
from
> > the callers. (I think the CHECK is better since this feels invalid)
> 
> Added a CHECK.  We should destroy the hosts when all Browsers for that profile
> close (though we don't yet).

OK.  Add a TODO/bug for that work so we don't lose it.

[Matt Perry, 2009-04-29 20:08:00]

FYI, I uploaded a new snapshot that merged in the changes from the past couple
days.

[Matt Perry, 2009-04-29 20:08:21]

On 2009/04/29 20:08:00, Matt Perry wrote:
> FYI, I uploaded a new snapshot that merged in the changes from the past couple
> days.

(changes to extension_view.* that is.  That's the only place there were
conflicts.)

[Matt Perry, 2009-04-29 20:08:38]

FYI, I uploaded a new snapshot that merged in the changes from the past couple
days.

[Erik does not do reviews, 2009-04-29 21:13:24]

from my standpoint, it still LGTM, but aa should really look at this now