Add a webstore API for installing bundles of extensions.

Add a webstore API for installing bundles of extensions.

This does not include any of the UI.

Re-landing due to trouble on debug bots (parameters section was missing from the callback spec).

BUG=112096
TEST=*InstallBundle*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=123253

[Aaron Boodman, 2012-02-17 01:59:33]

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/webstore/bundle_installer.cc (right):

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.cc:32:
AutoApproveForTest auto_approve_for_test = DO_NOT_SKIP;
g_auto_approve...

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.cc:49: return
Extension::Create(
You can move the arguments up one line.

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.cc:59: :
profile_(profile),
You need to initialize approved_ too.

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.cc:67:
STLDeleteContainerPairSecondPointers(
You can just make the map be: std::map<int, linked_ptr<base::DictionaryValue> >
and it will do the deleting automatically.

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.cc:178:
pending_items_.erase(id);
Instead of moving things between lists this way, could you just have a state
field on each item that indicates where it is in the process?

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.cc:207: void
BundleInstaller::ParseManifests() {
I think it would be easier to read if you put the methods in roughly their call
order. So this would go right after Prompt...().

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.cc:255: Browser* browser
= BrowserList::GetLastActiveWithProfile(profile_);
It seems that it would be better to pass a Browser (or even a TabContents) in.
That way the dialog will show up associated with the correct window. And if the
browser/tab goes away then the dialog will never occur, which is what you'd
expect.

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/webstore/bundle_installer.h (right):

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.h:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
2012

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.h:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
Typically the contents of the api/foo/ directories are the classes that
implement the ExtensionFunction for those APIs.

I think this should probably be in chrome/browser/extensions.

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.h:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
Is this class unit-testable? It seems like it almost could be. Not too many
dependencies.

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.h:20: namespace content
{
add blank lines separating these namespaces.

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.h:40: typedef
std::vector<Item> ItemList;
Having this all alone in the namespace looks a little odd to me. Can it be a
member of BundleInstaller?

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.h:55: static void
SetAutoApproveForTesting(bool approve);
You can enforce this in the implementation by checking if the --test switch is
present in the command line. Then no need for scary comment.

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.h:90: //
WebstoreInstallHelper::Delegate implementation:
If these are not part of the class's public interface, you can put them in the
private section.

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.h:123: // Parses the all
the manifests using WebstoreInstallHelper.
.remove(" all");

[jstritar, 2012-02-17 22:29:09]

https://chromiumcodereview.appspot.com/9414013/diff/27/chrome/browser/extensi...
File chrome/browser/extensions/api/webstore/bundle_installer.cc (right):

https://chromiumcodereview.appspot.com/9414013/diff/27/chrome/browser/extensi...
chrome/browser/extensions/api/webstore/bundle_installer.cc:32:
AutoApproveForTest auto_approve_for_test = DO_NOT_SKIP;
On 2012/02/17 01:59:33, Aaron Boodman wrote:
> g_auto_approve...

Done.

https://chromiumcodereview.appspot.com/9414013/diff/27/chrome/browser/extensi...
chrome/browser/extensions/api/webstore/bundle_installer.cc:49: return
Extension::Create(
On 2012/02/17 01:59:33, Aaron Boodman wrote:
> You can move the arguments up one line.

Done.

https://chromiumcodereview.appspot.com/9414013/diff/27/chrome/browser/extensi...
chrome/browser/extensions/api/webstore/bundle_installer.cc:59: :
profile_(profile),
On 2012/02/17 01:59:33, Aaron Boodman wrote:
> You need to initialize approved_ too.

Done.

https://chromiumcodereview.appspot.com/9414013/diff/27/chrome/browser/extensi...
chrome/browser/extensions/api/webstore/bundle_installer.cc:67:
STLDeleteContainerPairSecondPointers(
On 2012/02/17 01:59:33, Aaron Boodman wrote:
> You can just make the map be: std::map<int, linked_ptr<base::DictionaryValue>
>
> and it will do the deleting automatically.

Nice, do you pretty much only use linked_ptrs in STL containers?
Done.

https://chromiumcodereview.appspot.com/9414013/diff/27/chrome/browser/extensi...
chrome/browser/extensions/api/webstore/bundle_installer.cc:178:
pending_items_.erase(id);
On 2012/02/17 01:59:33, Aaron Boodman wrote:
> Instead of moving things between lists this way, could you just have a state
> field on each item that indicates where it is in the process?

Done.

https://chromiumcodereview.appspot.com/9414013/diff/27/chrome/browser/extensi...
chrome/browser/extensions/api/webstore/bundle_installer.cc:207: void
BundleInstaller::ParseManifests() {
On 2012/02/17 01:59:33, Aaron Boodman wrote:
> I think it would be easier to read if you put the methods in roughly their
call
> order. So this would go right after Prompt...().

Done.

https://chromiumcodereview.appspot.com/9414013/diff/27/chrome/browser/extensi...
chrome/browser/extensions/api/webstore/bundle_installer.cc:255: Browser* browser
= BrowserList::GetLastActiveWithProfile(profile_);
On 2012/02/17 01:59:33, Aaron Boodman wrote:
> It seems that it would be better to pass a Browser (or even a TabContents) in.
> That way the dialog will show up associated with the correct window. And if
the
> browser/tab goes away then the dialog will never occur, which is what you'd
> expect.

Done. Went with a Browser (rather than TabContents) since the bubble is shown
near the browser actions.

https://chromiumcodereview.appspot.com/9414013/diff/27/chrome/browser/extensi...
File chrome/browser/extensions/api/webstore/bundle_installer.h (right):

https://chromiumcodereview.appspot.com/9414013/diff/27/chrome/browser/extensi...
chrome/browser/extensions/api/webstore/bundle_installer.h:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
On 2012/02/17 01:59:33, Aaron Boodman wrote:
> 2012

Done.

https://chromiumcodereview.appspot.com/9414013/diff/27/chrome/browser/extensi...
chrome/browser/extensions/api/webstore/bundle_installer.h:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
On 2012/02/17 01:59:33, Aaron Boodman wrote:
> Typically the contents of the api/foo/ directories are the classes that
> implement the ExtensionFunction for those APIs.
> 
> I think this should probably be in chrome/browser/extensions.

Okay, I was thinking of moving all the webstore things to api/webstore since
they're only around to implement the webstore private API. 

I moved this back to extensions. Can I keep the namespace?

https://chromiumcodereview.appspot.com/9414013/diff/27/chrome/browser/extensi...
chrome/browser/extensions/api/webstore/bundle_installer.h:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
On 2012/02/17 01:59:33, Aaron Boodman wrote:
> Is this class unit-testable? It seems like it almost could be. Not too many
> dependencies.

I tried to do this, but it's not easy given how WebstoreInstaller and
WebstoreInstallHelper work. The test wouldn't do much if we mock those out. I
say we stick with the API tests -- they have good coverage of BundleInstaller --
until those support unit tests better.

https://chromiumcodereview.appspot.com/9414013/diff/27/chrome/browser/extensi...
chrome/browser/extensions/api/webstore/bundle_installer.h:20: namespace content
{
On 2012/02/17 01:59:33, Aaron Boodman wrote:
> add blank lines separating these namespaces.

Done.

https://chromiumcodereview.appspot.com/9414013/diff/27/chrome/browser/extensi...
chrome/browser/extensions/api/webstore/bundle_installer.h:40: typedef
std::vector<Item> ItemList;
On 2012/02/17 01:59:33, Aaron Boodman wrote:
> Having this all alone in the namespace looks a little odd to me. Can it be a
> member of BundleInstaller?

Done (also moved Item).

https://chromiumcodereview.appspot.com/9414013/diff/27/chrome/browser/extensi...
chrome/browser/extensions/api/webstore/bundle_installer.h:55: static void
SetAutoApproveForTesting(bool approve);
On 2012/02/17 01:59:33, Aaron Boodman wrote:
> You can enforce this in the implementation by checking if the --test switch is
> present in the command line. Then no need for scary comment.

Nice, done.

https://chromiumcodereview.appspot.com/9414013/diff/27/chrome/browser/extensi...
chrome/browser/extensions/api/webstore/bundle_installer.h:90: //
WebstoreInstallHelper::Delegate implementation:
On 2012/02/17 01:59:33, Aaron Boodman wrote:
> If these are not part of the class's public interface, you can put them in the
> private section.

Done. Does this only work because the Delegates are declared as inner classes?

https://chromiumcodereview.appspot.com/9414013/diff/27/chrome/browser/extensi...
chrome/browser/extensions/api/webstore/bundle_installer.h:123: // Parses the all
the manifests using WebstoreInstallHelper.
On 2012/02/17 01:59:33, Aaron Boodman wrote:
> .remove(" all");

Done.

[Aaron Boodman, 2012-02-18 11:11:54]

redirecting to yoz..

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/webstore/bundle_installer.cc (right):

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.cc:67:
STLDeleteContainerPairSecondPointers(
On 2012/02/17 22:29:09, jstritar wrote:
> On 2012/02/17 01:59:33, Aaron Boodman wrote:
> > You can just make the map be: std::map<int,
linked_ptr<base::DictionaryValue>
> >
> > and it will do the deleting automatically.
> 
> Nice, do you pretty much only use linked_ptrs in STL containers?
> Done.

Yeah, I don't really know of any use for them other than "I wish I could put a
scoped_ptr inside a container".

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/webstore/bundle_installer.h (right):

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.h:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
On 2012/02/17 22:29:09, jstritar wrote:
> I moved this back to extensions. Can I keep the namespace?

Is there anything else in the 'webstore' namespace? I find it surprising that
this would be there. I'd expect it to be in 'extensions' or '' just like
everything else in chrome/browser/extensions.

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.h:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
On 2012/02/17 22:29:09, jstritar wrote:
> On 2012/02/17 01:59:33, Aaron Boodman wrote:
> > Is this class unit-testable? It seems like it almost could be. Not too many
> > dependencies.
> 
> I tried to do this, but it's not easy given how WebstoreInstaller and
> WebstoreInstallHelper work. The test wouldn't do much if we mock those out. I
> say we stick with the API tests -- they have good coverage of BundleInstaller
--
> until those support unit tests better.

OK, thanks for looking.

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.h:90: //
WebstoreInstallHelper::Delegate implementation:
On 2012/02/17 22:29:09, jstritar wrote:
> On 2012/02/17 01:59:33, Aaron Boodman wrote:
> > If these are not part of the class's public interface, you can put them in
the
> > private section.
> 
> Done. Does this only work because the Delegates are declared as inner classes?

No, it's a general feature of C++ that you can inherit privately. So awesome.

http://codereview.chromium.org/9414013/diff/10001/chrome/browser/extensions/b...
File chrome/browser/extensions/bundle_installer.cc (right):

http://codereview.chromium.org/9414013/diff/10001/chrome/browser/extensions/b...
chrome/browser/extensions/bundle_installer.cc:199: if (browser_)
You don't ever change the value of browser_. I didn't make this exactly clear in
the last review, but I meant that you could listen to notifications related to
it from BrowserList. And when it is closed, then null it out so that you don't
bother showing the bubble.

Right now, if the browser closes between CompleteInstall() and
ShowInstalledBubbleIfDone(), you will crash.

[jstritar, 2012-02-21 15:35:28]

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/webstore/bundle_installer.h (right):

http://codereview.chromium.org/9414013/diff/27/chrome/browser/extensions/api/...
chrome/browser/extensions/api/webstore/bundle_installer.h:1: // Copyright (c)
2011 The Chromium Authors. All rights reserved.
On 2012/02/18 11:11:54, Aaron Boodman wrote:
>
> Is there anything else in the 'webstore' namespace? I find it surprising that
> this would be there. I'd expect it to be in 'extensions' or '' just like
> everything else in chrome/browser/extensions.

Oh, yeah, using the 'extensions' namespace now.

http://codereview.chromium.org/9414013/diff/10001/chrome/browser/extensions/b...
File chrome/browser/extensions/bundle_installer.cc (right):

http://codereview.chromium.org/9414013/diff/10001/chrome/browser/extensions/b...
chrome/browser/extensions/bundle_installer.cc:199: if (browser_)
On 2012/02/18 11:11:54, Aaron Boodman wrote:
> You don't ever change the value of browser_. I didn't make this exactly clear
in
> the last review, but I meant that you could listen to notifications related to
> it from BrowserList. And when it is closed, then null it out so that you don't
> bother showing the bubble.
> 
> Right now, if the browser closes between CompleteInstall() and
> ShowInstalledBubbleIfDone(), you will crash.

Done.

[Yoyo Zhou, 2012-02-22 00:20:26]

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/b...
File chrome/browser/extensions/bundle_installer.cc (right):

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/b...
chrome/browser/extensions/bundle_installer.cc:45: DictionaryValue* manifest) {
It's slightly confusing that this argument has the same name as the string
manifest member of Item.

What do you think about putting this function in the anonymous namespace as
CreateDummyExtension(Item, DictionaryValue*)?

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/b...
chrome/browser/extensions/bundle_installer.cc:102: delegate_ = delegate;
Is this delegate argument necessary? It looks like it's going to be the same
delegate_.

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/b...
chrome/browser/extensions/bundle_installer.cc:106: if (!approved_ ||
GetItemsByState(Item::STATE_PENDING).empty()) {
How do we get here if !approved_; could that be a CHECK? Otherwise, shouldn't we
report canceled in that case?

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/b...
chrome/browser/extensions/bundle_installer.cc:149: Release();  // Balanced in
ParseManifests().
in PromptForApproval

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/b...
chrome/browser/extensions/bundle_installer.cc:156: Release();  // Balanced in
ParseManifests().
ditto

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/b...
File chrome/browser/extensions/bundle_installer.h (right):

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/b...
chrome/browser/extensions/bundle_installer.h:86: ItemList
GetItemsByState(Item::State state) const;
GetItemsWithState, perhaps?

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webstore_private_api.cc (right):

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.cc:154: AddRef();  //
Balanced in OnBundleInstallApproved and OnBundleInstallCanceled.
in OnBundleInstallCompleted

[jstritar, 2012-02-22 15:45:48]

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/b...
File chrome/browser/extensions/bundle_installer.cc (right):

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/b...
chrome/browser/extensions/bundle_installer.cc:45: DictionaryValue* manifest) {
On 2012/02/22 00:20:26, Yoyo Zhou wrote:
> It's slightly confusing that this argument has the same name as the string
> manifest member of Item.
> 
> What do you think about putting this function in the anonymous namespace as
> CreateDummyExtension(Item, DictionaryValue*)?

Done.

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/b...
chrome/browser/extensions/bundle_installer.cc:102: delegate_ = delegate;
On 2012/02/22 00:20:26, Yoyo Zhou wrote:
> Is this delegate argument necessary? It looks like it's going to be the same
> delegate_.

Yeah, PromptForApproval and CompleteInstall will be called by different
ExtensionFunctions when we want to support payments (we might need to do this
for M19):

1. PromptForApproval to accept permissions
2. store performs checkout flow
3. CompleteInstall to actually install the extensions

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/b...
chrome/browser/extensions/bundle_installer.cc:106: if (!approved_ ||
GetItemsByState(Item::STATE_PENDING).empty()) {
On 2012/02/22 00:20:26, Yoyo Zhou wrote:
> How do we get here if !approved_; could that be a CHECK? Otherwise, shouldn't
we
> report canceled in that case?

Good point -- even if these methods are called by different delegates, they
should still check if the bundle is approved().

Done.

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/b...
chrome/browser/extensions/bundle_installer.cc:149: Release();  // Balanced in
ParseManifests().
On 2012/02/22 00:20:26, Yoyo Zhou wrote:
> in PromptForApproval

Done.

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/b...
chrome/browser/extensions/bundle_installer.cc:156: Release();  // Balanced in
ParseManifests().
On 2012/02/22 00:20:26, Yoyo Zhou wrote:
> ditto

Done.

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/b...
File chrome/browser/extensions/bundle_installer.h (right):

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/b...
chrome/browser/extensions/bundle_installer.h:86: ItemList
GetItemsByState(Item::State state) const;
On 2012/02/22 00:20:26, Yoyo Zhou wrote:
> GetItemsWithState, perhaps?

Done.

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_webstore_private_api.cc (right):

http://codereview.chromium.org/9414013/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_webstore_private_api.cc:154: AddRef();  //
Balanced in OnBundleInstallApproved and OnBundleInstallCanceled.
On 2012/02/22 00:20:26, Yoyo Zhou wrote:
> in OnBundleInstallCompleted 

Thanks for catching all those... forgot to change those after refractoring.

Done.

[Yoyo Zhou, 2012-02-22 17:41:25]

LGTM

[commit-bot: I haz the power, 2012-02-22 17:42:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/9414013/19001

[commit-bot: I haz the power, 2012-02-22 20:12:39]

Change committed as 123094

[commit-bot: I haz the power, 2012-02-23 16:21:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/9414013/20001