Initial inline web store install bindings.

Initial inline web store install bindings.

Adds chrome.webstore.install() which looks for a <link> tag with the right relation
and href (that points to a CWS item). Does not actually show an inline install UI,
just opens the CWS page in a new tab.

R=aa@chromium.org


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

[Aaron Boodman, 2011-08-17 04:53:03]

http://codereview.chromium.org/7621032/diff/1/chrome/browser/chrome_content_b...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/7621032/diff/1/chrome/browser/chrome_content_b...
chrome/browser/chrome_content_browser_client.cc:273:
switches::kEnableInlineWebstoreInstall,
It looks like you aren't using this in the renderer.

http://codereview.chromium.org/7621032/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_tab_helper.cc (right):

http://codereview.chromium.org/7621032/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_tab_helper.cc:152:
switches::kEnableInlineWebstoreInstall))
You should use curly braces when the condition wraps.

http://codereview.chromium.org/7621032/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/7621032/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.h:369: static std::string
ChromeStoreItemDetailURLPrefix();
I realize ChromeStoreLaunchURL() is already an example of this, but this method
has nothing to do with the Extension class. Maybe this should go in
extension_constants?

Also, the naming should be GetChromeStore...

http://codereview.chromium.org/7621032/diff/1/chrome/renderer/extensions/chro...
File chrome/renderer/extensions/chrome_webstore_bindings.cc (right):

http://codereview.chromium.org/7621032/diff/1/chrome/renderer/extensions/chro...
chrome/renderer/extensions/chrome_webstore_bindings.cc:7: #include
"base/string16.h"
I don't see any instance of 'string16' in this file. Is it needed?

http://codereview.chromium.org/7621032/diff/1/chrome/renderer/extensions/chro...
chrome/renderer/extensions/chrome_webstore_bindings.cc:76: ExtensionHelper*
helper = ExtensionHelper::Get(render_view);
I don't see this method defined on ExtensionHelper. Other v8 extensions have an
internal Impl class that stores a pointer to ExtensionHelper. Do the same here?

http://codereview.chromium.org/7621032/diff/1/chrome/renderer/extensions/chro...
File chrome/renderer/extensions/chrome_webstore_bindings.h (right):

http://codereview.chromium.org/7621032/diff/1/chrome/renderer/extensions/chro...
chrome/renderer/extensions/chrome_webstore_bindings.h:6: // at
window.chrome.webstore.  This object allows JavaScript to initiate inline
"at" fits on the previous line.

http://codereview.chromium.org/7621032/diff/1/chrome/test/data/extensions/api...
File chrome/test/data/extensions/api_test/webstore_inline_install/install.html
(right):

http://codereview.chromium.org/7621032/diff/1/chrome/test/data/extensions/api...
chrome/test/data/extensions/api_test/webstore_inline_install/install.html:4:
<link rel="chrome-webstore-item" href="http://cws.com/detail/abc">
Why specify the URL just to take it apart in the renderer, and send it up to the
browser. Why not just specify the ID only?

http://codereview.chromium.org/7621032/diff/1/chrome/test/data/extensions/api...
chrome/test/data/extensions/api_test/webstore_inline_install/install.html:9:
chrome.webstore.install();
Do we need both chrome.webstore.install() and chrome.app.install()? Did you all
discuss combining them? Also, did you discuss passing the URL to install()
directly?

[Mihai Parparita -not on Chrome, 2011-08-17 23:00:16]

http://codereview.chromium.org/7621032/diff/1/chrome/browser/chrome_content_b...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/7621032/diff/1/chrome/browser/chrome_content_b...
chrome/browser/chrome_content_browser_client.cc:273:
switches::kEnableInlineWebstoreInstall,
On 2011/08/17 04:53:03, Aaron Boodman wrote:
> It looks like you aren't using this in the renderer.

Actually, the check in ExtensionDispatcher::WebKitInitialized lives in the
renderer.

http://codereview.chromium.org/7621032/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_tab_helper.cc (right):

http://codereview.chromium.org/7621032/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_tab_helper.cc:152:
switches::kEnableInlineWebstoreInstall))
On 2011/08/17 04:53:03, Aaron Boodman wrote:
> You should use curly braces when the condition wraps.

Done.

http://codereview.chromium.org/7621032/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/7621032/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.h:369: static std::string
ChromeStoreItemDetailURLPrefix();
On 2011/08/17 04:53:03, Aaron Boodman wrote:
> I realize ChromeStoreLaunchURL() is already an example of this, but this
method
> has nothing to do with the Extension class. Maybe this should go in
> extension_constants?
> 
> Also, the naming should be GetChromeStore...

Moved to extension_constants and renamed to
GetWebstoreLaunchURL/GetWebstoreItemDetailURLPrefix.

http://codereview.chromium.org/7621032/diff/1/chrome/renderer/extensions/chro...
File chrome/renderer/extensions/chrome_webstore_bindings.cc (right):

http://codereview.chromium.org/7621032/diff/1/chrome/renderer/extensions/chro...
chrome/renderer/extensions/chrome_webstore_bindings.cc:7: #include
"base/string16.h"
On 2011/08/17 04:53:03, Aaron Boodman wrote:
> I don't see any instance of 'string16' in this file. Is it needed?

Removed this and a couple of other unneeded #includes.

http://codereview.chromium.org/7621032/diff/1/chrome/renderer/extensions/chro...
chrome/renderer/extensions/chrome_webstore_bindings.cc:76: ExtensionHelper*
helper = ExtensionHelper::Get(render_view);
On 2011/08/17 04:53:03, Aaron Boodman wrote:
> I don't see this method defined on ExtensionHelper. Other v8 extensions have
an
> internal Impl class that stores a pointer to ExtensionHelper. Do the same
here?

chrome_app_bindings.cc and extension_process_bindings.cc do this too. The ::Get
methods comes from the RenderViewObserverTracker class that ExtensionHelper
inherits from
(http://www.google.com/codesearch#OAMlx_jo-ck/src/content/renderer/render_view...).

http://codereview.chromium.org/7621032/diff/1/chrome/renderer/extensions/chro...
File chrome/renderer/extensions/chrome_webstore_bindings.h (right):

http://codereview.chromium.org/7621032/diff/1/chrome/renderer/extensions/chro...
chrome/renderer/extensions/chrome_webstore_bindings.h:6: // at
window.chrome.webstore.  This object allows JavaScript to initiate inline
On 2011/08/17 04:53:03, Aaron Boodman wrote:
> "at" fits on the previous line.

Done.

http://codereview.chromium.org/7621032/diff/1/chrome/test/data/extensions/api...
File chrome/test/data/extensions/api_test/webstore_inline_install/install.html
(right):

http://codereview.chromium.org/7621032/diff/1/chrome/test/data/extensions/api...
chrome/test/data/extensions/api_test/webstore_inline_install/install.html:4:
<link rel="chrome-webstore-item" href="http://cws.com/detail/abc">
On 2011/08/17 04:53:03, Aaron Boodman wrote:
> Why specify the URL just to take it apart in the renderer, and send it up to
the
> browser. Why not just specify the ID only?

Having a full URL seems cleaner from a web standards perspective
(http://www.whatwg.org/specs/web-apps/current-work/multipage/semantics.html#at...).

http://codereview.chromium.org/7621032/diff/1/chrome/test/data/extensions/api...
chrome/test/data/extensions/api_test/webstore_inline_install/install.html:9:
chrome.webstore.install();
On 2011/08/17 04:53:03, Aaron Boodman wrote:
> Do we need both chrome.webstore.install() and chrome.app.install()? Did you
all
> discuss combining them? 

I don't seem much overlap in the implementation as things currently stand
(chrome.app.install has to fetch the CRX-less manifest, whereas
chrome.webstore.install will need to talk to CWS). I also didn't want to have
"app" in the name, since the intent is to allow this to prompt for installation
of extensions and themes too.

> Also, did you discuss passing the URL to install() directly?

I think there's some value in having the <link> tag for crawl reasons. For cases
where the page has multiple <link> tags,
https://docs.google.com/a/google.com/document/d/1dlADtMCDK-i1EMIw0ECvvAoCtw9h...
mentions allowing an optional ID parameter to specify which should be installed.

[Aaron Boodman, 2011-08-17 23:15:41]

ok

http://codereview.chromium.org/7621032/diff/1/chrome/browser/chrome_content_b...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/7621032/diff/1/chrome/browser/chrome_content_b...
chrome/browser/chrome_content_browser_client.cc:273:
switches::kEnableInlineWebstoreInstall,
On 2011/08/17 23:00:16, Mihai Parparita wrote:
> On 2011/08/17 04:53:03, Aaron Boodman wrote:
> > It looks like you aren't using this in the renderer.
> 
> Actually, the check in ExtensionDispatcher::WebKitInitialized lives in the
> renderer.

whoops.

http://codereview.chromium.org/7621032/diff/1/chrome/renderer/extensions/chro...
File chrome/renderer/extensions/chrome_webstore_bindings.cc (right):

http://codereview.chromium.org/7621032/diff/1/chrome/renderer/extensions/chro...
chrome/renderer/extensions/chrome_webstore_bindings.cc:76: ExtensionHelper*
helper = ExtensionHelper::Get(render_view);
On 2011/08/17 23:00:16, Mihai Parparita wrote:
> On 2011/08/17 04:53:03, Aaron Boodman wrote:
> > I don't see this method defined on ExtensionHelper. Other v8 extensions have
> an
> > internal Impl class that stores a pointer to ExtensionHelper. Do the same
> here?
> 
> chrome_app_bindings.cc and extension_process_bindings.cc do this too. The
::Get
> methods comes from the RenderViewObserverTracker class that ExtensionHelper
> inherits from
>
(http://www.google.com/codesearch#OAMlx_jo-ck/src/content/renderer/render_view...).

Um, ok. I think this changed since I last saw it.

[Aaron Boodman, 2011-08-17 23:25:18]

lgtm

[commit-bot: I haz the power, 2011-08-18 03:09:07]

Change committed as 97274