Add WebstoreInlineInstaller (downloads store data, shows the install UI, and starts the install).

Add WebstoreInlineInstaller (downloads store data, shows the install UI, and starts the install).

The flow is:
1. Fetch store metadata as JSON (using URLFetcher)
2. Parse response in utility process (via SafeWebstoreResponseParser)
3. Parse manifest and get icon data using WebstoreInstallHelper
4. Show install UI
5. Whitelist extension ID for download and start download

Still missing are a way of informing the page that the inline install succeeded
or failed.

Also removes ExtensionTabHelper::GetCustomFrameNativeWindow, since it wasn't
actually overriding anything (the method was removed from 
ExtensionFunctionDispatcher::Delegate by r74835).

R=asargent@chromium.org

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

[Mihai Parparita -not on Chrome, 2011-08-25 23:49:34]

Let me know if this has too many TODOs to land, though I figured the CL was
big enough as it is.

Mihai

On Thu, Aug 25, 2011 at 4:48 PM, <mihaip@chromium.org> wrote:

> Reviewers: Antony Sargent,
>
> Description:
> Add WebstoreInlineInstaller (downloads store data, shows the install UI,
> and
> starts the install).
>
> The flow is:
> 1. Fetch store metadata as JSON (using URLFetcher)
> 2. Parse response in utility process (via SafeWebstoreResponseParser)
> 3. Parse manifest and get icon data using WebstoreInstallHelper
> 4. Show install UI
> 5. Whitelist extension ID for download and start download
>
> Still missing are a way of informing the page that the inline install
> succeeded
> or failed.
>
> R=asargent@chromium.org
>
>
> Please review this at
http://codereview.chromium.**org/7741037/<http://codereview.chromium.org/7741...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M chrome/browser/extensions/**extension_tab_helper.h
>  M chrome/browser/extensions/**extension_tab_helper.cc
>  M chrome/browser/extensions/**webstore_inline_install_**browsertest.cc
>  A chrome/browser/extensions/**webstore_inline_installer.h
>  A chrome/browser/extensions/**webstore_inline_installer.cc
>  M chrome/chrome_browser.gypi
>  M chrome/common/extensions/**extension_constants.h
>  M chrome/common/extensions/**extension_constants.cc
>  M chrome/common/extensions/**extension_messages.h
>  M chrome/renderer/extensions/**extension_helper.h
>  M chrome/renderer/extensions/**extension_helper.cc
>  A chrome/test/data/extensions/**api_test/webstore_inline_**
> install/detail/**ecglahbcnmdpdciemllbhojghbkagd**je
>  A chrome/test/data/extensions/**api_test/webstore_inline_**
> install/extension.crx
>  A chrome/test/data/extensions/**api_test/webstore_inline_**
> install/extension.pem
>  A chrome/test/data/extensions/**api_test/webstore_inline_**
> install/extension/icon.png
>  A chrome/test/data/extensions/**api_test/webstore_inline_**
> install/extension/manifest.**json
>  M chrome/test/data/extensions/**api_test/webstore_inline_**
> install/find_link.html
>  M chrome/test/data/extensions/**api_test/webstore_inline_**
> install/install.html
>
>
>

[asargent_no_longer_on_chrome, 2011-08-26 17:50:40]

LGTM w/ some nits & suggestions

http://codereview.chromium.org/7741037/diff/1/chrome/browser/extensions/webst...
File chrome/browser/extensions/webstore_inline_install_browsertest.cc (right):

http://codereview.chromium.org/7741037/diff/1/chrome/browser/extensions/webst...
chrome/browser/extensions/webstore_inline_install_browsertest.cc:57: GURL
GetTestUrl(const std::string& domain, const std::string& page_filename) {
There's already a GetTestUrl in ui_test_utils.h that does something similar to
this, minus the domain replacement. Consider whether it would make sense to use
that here, and perhaps change the name of yours to reduce confusion.

http://codereview.chromium.org/7741037/diff/1/chrome/browser/extensions/webst...
File chrome/browser/extensions/webstore_inline_installer.cc (right):

http://codereview.chromium.org/7741037/diff/1/chrome/browser/extensions/webst...
chrome/browser/extensions/webstore_inline_installer.cc:46: ,
webstore_data_(webstore_data)
commas in weird place here and line below

http://codereview.chromium.org/7741037/diff/1/chrome/browser/extensions/webst...
chrome/browser/extensions/webstore_inline_installer.cc:136: ,
id_(webstore_item_id)
commas in weird place here too

http://codereview.chromium.org/7741037/diff/1/chrome/browser/extensions/webst...
chrome/browser/extensions/webstore_inline_installer.cc:143: GURL
webstore_data_url(extension_urls::GetWebstoreItemJsonDataURL(id_));
It might be good to add a check of Extension::IdIsValid(webstore_item_id) and
bail out early if it returns false.

http://codereview.chromium.org/7741037/diff/1/chrome/browser/extensions/webst...
chrome/browser/extensions/webstore_inline_installer.cc:156: void
WebstoreInlineInstaller::SetAutoConfirmForTests(
add comment with "// static" here

http://codereview.chromium.org/7741037/diff/1/chrome/browser/extensions/webst...
chrome/browser/extensions/webstore_inline_installer.cc:277:
ExtensionInstallUI::INSTALL_PROMPT);
Do you think it's worth it to extract this code to create the dummy extension
and put up the install dialog into a common class and share it with the webstore
private api? I think we could just pass the Profile, localized name std::string,
manifest DictionaryValue, icon SkBitmap and auto_prompt boolean and then get
called back in InstallUIProceed or InstallUIAbort.

http://codereview.chromium.org/7741037/diff/1/chrome/browser/extensions/webst...
chrome/browser/extensions/webstore_inline_installer.cc:307: // is actually
downloaded and installed.
Yeah, for the case where the download succeeds you can just listen for
NOTIFICATION_EXTENSION_INSTALLED and NOTIFICATION_EXTENSION_INSTALL_ERROR. There
might be a notification for the download failure too.

http://codereview.chromium.org/7741037/diff/1/chrome/renderer/extensions/exte...
File chrome/renderer/extensions/extension_helper.cc (right):

http://codereview.chromium.org/7741037/diff/1/chrome/renderer/extensions/exte...
chrome/renderer/extensions/extension_helper.cc:116: LOG(INFO) << "Inline install
succeeded.";
nit: LOG(INFO) considered harmful (except in cases where you're actively
tracking down a bug in the field). Prefer VLOG instead.

... although I suppose you're going to eventually replace this code anyway based
on above TODO.

[Mihai Parparita -not on Chrome, 2011-08-29 19:00:56]

http://codereview.chromium.org/7741037/diff/1/chrome/browser/extensions/webst...
File chrome/browser/extensions/webstore_inline_install_browsertest.cc (right):

http://codereview.chromium.org/7741037/diff/1/chrome/browser/extensions/webst...
chrome/browser/extensions/webstore_inline_install_browsertest.cc:57: GURL
GetTestUrl(const std::string& domain, const std::string& page_filename) {
On 2011/08/26 17:50:40, Antony Sargent wrote:
> There's already a GetTestUrl in ui_test_utils.h that does something similar to
> this, minus the domain replacement. Consider whether it would make sense to
use
> that here, and perhaps change the name of yours to reduce confusion.

GetTestUrl in ui_test_utils returns a file:/// URL for a test file, so it's not
quite appropriate to use here. I've renamed this to GenerateTestServerUrl.

http://codereview.chromium.org/7741037/diff/1/chrome/browser/extensions/webst...
File chrome/browser/extensions/webstore_inline_installer.cc (right):

http://codereview.chromium.org/7741037/diff/1/chrome/browser/extensions/webst...
chrome/browser/extensions/webstore_inline_installer.cc:46: ,
webstore_data_(webstore_data)
On 2011/08/26 17:50:40, Antony Sargent wrote:
> commas in weird place here and line below

Fixed.

http://codereview.chromium.org/7741037/diff/1/chrome/browser/extensions/webst...
chrome/browser/extensions/webstore_inline_installer.cc:136: ,
id_(webstore_item_id)
On 2011/08/26 17:50:40, Antony Sargent wrote:
> commas in weird place here too

Fixed.

http://codereview.chromium.org/7741037/diff/1/chrome/browser/extensions/webst...
chrome/browser/extensions/webstore_inline_installer.cc:143: GURL
webstore_data_url(extension_urls::GetWebstoreItemJsonDataURL(id_));
On 2011/08/26 17:50:40, Antony Sargent wrote:
> It might be good to add a check of Extension::IdIsValid(webstore_item_id) and
> bail out early if it returns false.
>  

Done.

http://codereview.chromium.org/7741037/diff/1/chrome/browser/extensions/webst...
chrome/browser/extensions/webstore_inline_installer.cc:156: void
WebstoreInlineInstaller::SetAutoConfirmForTests(
On 2011/08/26 17:50:40, Antony Sargent wrote:
> add comment with "// static" here

Now that I've refactored the auto-confirm code to be shared with the webstore
private API, this method is no longer needed.

http://codereview.chromium.org/7741037/diff/1/chrome/browser/extensions/webst...
chrome/browser/extensions/webstore_inline_installer.cc:277:
ExtensionInstallUI::INSTALL_PROMPT);
On 2011/08/26 17:50:40, Antony Sargent wrote:
> Do you think it's worth it to extract this code to create the dummy extension
> and put up the install dialog into a common class and share it with the
webstore
> private api? I think we could just pass the Profile, localized name
std::string,
> manifest DictionaryValue, icon SkBitmap and auto_prompt boolean and then get
> called back in InstallUIProceed or InstallUIAbort.

Added ShowExtensionInstallDialogForManifest (the signature wasn't quite the
same, since BeginInstallWithManifestFunction needs to get at the dummy extension
to record some histograms about permissions).

http://codereview.chromium.org/7741037/diff/1/chrome/renderer/extensions/exte...
File chrome/renderer/extensions/extension_helper.cc (right):

http://codereview.chromium.org/7741037/diff/1/chrome/renderer/extensions/exte...
chrome/renderer/extensions/extension_helper.cc:116: LOG(INFO) << "Inline install
succeeded.";
On 2011/08/26 17:50:40, Antony Sargent wrote:
> nit: LOG(INFO) considered harmful (except in cases where you're actively
> tracking down a bug in the field). Prefer VLOG instead.
> 
> ... although I suppose you're going to eventually replace this code anyway
based
> on above TODO. 

Switched to VLOG.