Add/resurrect support for bundles of WebStore items.

Add/resurrect support for bundles of WebStore items.

PRD: https://docs.google.com/document/d/1ISbZTZzHhQv-A49uuY4gy_moJf6agBCvNktPEOYb3Sk
DD: https://docs.google.com/document/d/1f8b4jALZ3Z6VcmbzCJnpfZkc2pl8hLdlnkmtWIGiBXU

This CL adds a new webstorePrivate API to install a bundle. (A similar API existed in the past, but was removed in https://codereview.chromium.org/673263003/. Tests were removed even earlier in https://chromiumcodereview.appspot.com/15732016/.)
It also updates the bundle install logic (which had rotted somewhat).

BUG=448724
Committed: https://crrev.com/a21dcd92c7f0f0ffa39e6b0508934aae2875eb17
Cr-Commit-Position: refs/heads/master@{#323627}

[Marc Treib, 2015-03-02 13:13:39]

treib@chromium.org changed reviewers:
+ kalman@chromium.org

[Marc Treib, 2015-03-02 13:13:40]

Another new (or rather, resurrected) webstorePrivate API. PTAL!

[not at google - send to devlin, 2015-03-02 17:48:56]

kalman@chromium.org changed reviewers:
+ asargent@chromium.org

[not at google - send to devlin, 2015-03-02 17:48:57]

It's probably better for Antony to look at this.

[asargent_no_longer_on_chrome, 2015-03-03 23:08:54]

Sorry to not respond immediately - I'm taking some time to read through the PRD
and think carefully through the requirements, since this is a lot of extra
complexity for us to have in the extension install pipeline.

[asargent_no_longer_on_chrome, 2015-03-07 00:59:39]

On 2015/03/03 23:08:54, Antony Sargent wrote:
> Sorry to not respond immediately - I'm taking some time to read through the
PRD
> and think carefully through the requirements, since this is a lot of extra
> complexity for us to have in the extension install pipeline.

Ok, sorry for continuing delay. I've read through the PRD/Design Doc and it
looks like you've been thinking about the right questions. I have thought of a
couple of potential issues and am now looking through the code, so some of them
may already be addressed.

[asargent_no_longer_on_chrome, 2015-03-16 17:48:22]

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:612:
return RespondNow(Error(kCannotSpecifyIconDataAndUrlError));
FYI, we're phasing out the icon_data parameter from the webstore install APIs
since the webstore has only ever actually used the icon_url, and continuing to
have support for the base64-encoded icon_data was making a refactor of image
decoding more complicated than it needed to be. See this in-progress CL for
details:

 https://codereview.chromium.org/931993002/

I'm going to be coming through here and doing any remaining needed cleanup, but
in the meantime wherever it is convenient you can write your code assuming
icon_data will never be passed.

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:722: 
Quite a bit of the code here looks similar to what we already have in the
templatized WebstorePrivateFunctionWithPermissionPrompt methods above - could we
just modify that to include virtual functions to specialize in whatever few ways
your code here is different?

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.cc (right):

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.cc:85: if (!manifest_.empty())
Why would the manifest be empty?

[Marc Treib, 2015-03-17 12:18:27]

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:612:
return RespondNow(Error(kCannotSpecifyIconDataAndUrlError));
On 2015/03/16 17:48:22, Antony Sargent wrote:
> FYI, we're phasing out the icon_data parameter from the webstore install APIs
> since the webstore has only ever actually used the icon_url, and continuing to
> have support for the base64-encoded icon_data was making a refactor of image
> decoding more complicated than it needed to be. See this in-progress CL for
> details:
> 
>  https://codereview.chromium.org/931993002/
> 
> I'm going to be coming through here and doing any remaining needed cleanup,
but
> in the meantime wherever it is convenient you can write your code assuming
> icon_data will never be passed. 

Thanks for the heads-up!
If you'll be doing a cleanup pass anyway, then I will leave this as is for now.
(OTOH, if the cleanup lands before this, then I'll clean it up before landing!)

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:722: 
On 2015/03/16 17:48:22, Antony Sargent wrote:
> Quite a bit of the code here looks similar to what we already have in the
> templatized WebstorePrivateFunctionWithPermissionPrompt methods above - could
we
> just modify that to include virtual functions to specialize in whatever few
ways
> your code here is different?

I've looked at that, but the code is actually not that similar. Much of
WebstorePrivateFunctionWithPermissionPrompt deals with the
ExtensionInstallPrompt, which InstallBundle doesn't use. The only really similar
part is the parsing/validation of the Params. That could maybe be extracted into
a helper function, but InstallBundle uses a different format for the return
values, so that's also awkward... (InstallBundle has either an error or a return
value (like CompleteInstall), but WithPermissionPrompt has both. Which you're
not supposed to do, see the comment here:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...)

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.cc (right):

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.cc:85: if (!manifest_.empty())
On 2015/03/16 17:48:22, Antony Sargent wrote:
> Why would the manifest be empty?

It is empty in the bundle install case. I'm (ab)using the WebstoreInstallHelper
to get an actual icon from the icon URL, but the bundle doesn't have a manifest.

[Marc Treib, 2015-03-24 14:09:20]

On 2015/03/17 12:18:27, Marc Treib wrote:
>
https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
> File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
> (right):
> 
>
https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
> chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:612:
> return RespondNow(Error(kCannotSpecifyIconDataAndUrlError));
> On 2015/03/16 17:48:22, Antony Sargent wrote:
> > FYI, we're phasing out the icon_data parameter from the webstore install
APIs
> > since the webstore has only ever actually used the icon_url, and continuing
to
> > have support for the base64-encoded icon_data was making a refactor of image
> > decoding more complicated than it needed to be. See this in-progress CL for
> > details:
> > 
> >  https://codereview.chromium.org/931993002/
> > 
> > I'm going to be coming through here and doing any remaining needed cleanup,
> but
> > in the meantime wherever it is convenient you can write your code assuming
> > icon_data will never be passed. 
> 
> Thanks for the heads-up!
> If you'll be doing a cleanup pass anyway, then I will leave this as is for
now.
> (OTOH, if the cleanup lands before this, then I'll clean it up before
landing!)
> 
>
https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
> chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:722: 
> On 2015/03/16 17:48:22, Antony Sargent wrote:
> > Quite a bit of the code here looks similar to what we already have in the
> > templatized WebstorePrivateFunctionWithPermissionPrompt methods above -
could
> we
> > just modify that to include virtual functions to specialize in whatever few
> ways
> > your code here is different?
> 
> I've looked at that, but the code is actually not that similar. Much of
> WebstorePrivateFunctionWithPermissionPrompt deals with the
> ExtensionInstallPrompt, which InstallBundle doesn't use. The only really
similar
> part is the parsing/validation of the Params. That could maybe be extracted
into
> a helper function, but InstallBundle uses a different format for the return
> values, so that's also awkward... (InstallBundle has either an error or a
return
> value (like CompleteInstall), but WithPermissionPrompt has both. Which you're
> not supposed to do, see the comment here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...)
> 
>
https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
> File chrome/browser/extensions/webstore_install_helper.cc (right):
> 
>
https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
> chrome/browser/extensions/webstore_install_helper.cc:85: if
(!manifest_.empty())
> On 2015/03/16 17:48:22, Antony Sargent wrote:
> > Why would the manifest be empty?
> 
> It is empty in the bundle install case. I'm (ab)using the
WebstoreInstallHelper
> to get an actual icon from the icon URL, but the bundle doesn't have a
manifest.

Ping! :)

[asargent_no_longer_on_chrome, 2015-03-24 23:34:00]

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:722: 
On 2015/03/17 12:18:26, Marc Treib wrote:
> On 2015/03/16 17:48:22, Antony Sargent wrote:
> > Quite a bit of the code here looks similar to what we already have in the
> > templatized WebstorePrivateFunctionWithPermissionPrompt methods above -
could
> we
> > just modify that to include virtual functions to specialize in whatever few
> ways
> > your code here is different?
> 
> I've looked at that, but the code is actually not that similar. Much of
> WebstorePrivateFunctionWithPermissionPrompt deals with the
> ExtensionInstallPrompt, which InstallBundle doesn't use. The only really
similar
> part is the parsing/validation of the Params. That could maybe be extracted
into
> a helper function, but InstallBundle uses a different format for the return
> values, so that's also awkward... (InstallBundle has either an error or a
return
> value (like CompleteInstall), but WithPermissionPrompt has both. Which you're
> not supposed to do, see the comment here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...)

Sorry, I guess I was referring to the ::Run method most - it looks like 80% of
the lines in it are identical to
WebstorePrivateFunctionWithPermissionPrompt<Params>::Run

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.cc (right):

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.cc:85: if (!manifest_.empty())
On 2015/03/17 12:18:27, Marc Treib wrote:
> On 2015/03/16 17:48:22, Antony Sargent wrote:
> > Why would the manifest be empty?
> 
> It is empty in the bundle install case. I'm (ab)using the
WebstoreInstallHelper
> to get an actual icon from the icon URL, but the bundle doesn't have a
manifest.

Hmm, this seems like a risky approach, since all the other code which uses this
class passes a real manifest, so it seems like it would be easy for future
changes to accidentally assume manifest is non-empty and break your usage. 

When https://codereview.chromium.org/931993002/ lands, we'll have a class which
abstracts the work of sandboxed image processing (see the
chrome/browser/image_decoder.h class in that CL). At that point it might make
sense to just have the URLFetcher in your class, and use a ImageDecoder with the
data you fetch. 

In the mean time, it might be best to create a new class named something like
"SafeImageFetcher" with a method something like this:

typedef base::Callback<const SkBitmap*, std::string error> GetImageCallback;

// Tries to fetch and parse the image at |image_url|, returning either a valid
// image to the callback or an error describing what went wrong.
void GetImage(const GURL& image_url, const GetImageCallback& callback);

Then you can use this inside the implementation of WebstoreInstallHelper, and
directory in your API implementation.

[Marc Treib, 2015-03-25 11:21:59]

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:722: 
On 2015/03/24 23:34:00, Antony Sargent wrote:
> On 2015/03/17 12:18:26, Marc Treib wrote:
> > On 2015/03/16 17:48:22, Antony Sargent wrote:
> > > Quite a bit of the code here looks similar to what we already have in the
> > > templatized WebstorePrivateFunctionWithPermissionPrompt methods above -
> could
> > we
> > > just modify that to include virtual functions to specialize in whatever
few
> > ways
> > > your code here is different?
> > 
> > I've looked at that, but the code is actually not that similar. Much of
> > WebstorePrivateFunctionWithPermissionPrompt deals with the
> > ExtensionInstallPrompt, which InstallBundle doesn't use. The only really
> similar
> > part is the parsing/validation of the Params. That could maybe be extracted
> into
> > a helper function, but InstallBundle uses a different format for the return
> > values, so that's also awkward... (InstallBundle has either an error or a
> return
> > value (like CompleteInstall), but WithPermissionPrompt has both. Which
you're
> > not supposed to do, see the comment here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...)
> 
> Sorry, I guess I was referring to the ::Run method most - it looks like 80% of
> the lines in it are identical to
> WebstorePrivateFunctionWithPermissionPrompt<Params>::Run

I just removed the bundle ID (since it wasn't used for anything). The icon_data
will also be removed soon, and after http://crrev.com/931993002 I won't be using
a WebstoreInstallHelper anymore here. Then the only (mostly) common code will be
the few lines for getting and resolving the icon_url. I don't think those
warrant a helper function.

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.cc (right):

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.cc:85: if (!manifest_.empty())
On 2015/03/24 23:34:00, Antony Sargent wrote:
> On 2015/03/17 12:18:27, Marc Treib wrote:
> > On 2015/03/16 17:48:22, Antony Sargent wrote:
> > > Why would the manifest be empty?
> > 
> > It is empty in the bundle install case. I'm (ab)using the
> WebstoreInstallHelper
> > to get an actual icon from the icon URL, but the bundle doesn't have a
> manifest.
> 
> Hmm, this seems like a risky approach, since all the other code which uses
this
> class passes a real manifest, so it seems like it would be easy for future
> changes to accidentally assume manifest is non-empty and break your usage. 
> 
> When https://codereview.chromium.org/931993002/ lands, we'll have a class
which
> abstracts the work of sandboxed image processing (see the
> chrome/browser/image_decoder.h class in that CL). At that point it might make
> sense to just have the URLFetcher in your class, and use a ImageDecoder with
the
> data you fetch. 
> 
> In the mean time, it might be best to create a new class named something like
> "SafeImageFetcher" with a method something like this:
> 
> typedef base::Callback<const SkBitmap*, std::string error> GetImageCallback;
> 
> // Tries to fetch and parse the image at |image_url|, returning either a valid
> // image to the callback or an error describing what went wrong.
> void GetImage(const GURL& image_url, const GetImageCallback& callback);
> 
> Then you can use this inside the implementation of WebstoreInstallHelper, and
> directory in your API implementation. 
> 

Ack re risky.
Actually, http://crrev.com/931993002 seems almost ready to land. So rather than
messing with WebstoreInstallHelper here (and causing lots of merge conflicts), I
think I'll just wait for it to land and rebase on top of it.

[Marc Treib, 2015-03-26 15:49:26]

Next try :)
PTAL again!

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.cc (right):

https://codereview.chromium.org/855513002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.cc:85: if (!manifest_.empty())
On 2015/03/25 11:21:59, Marc Treib wrote:
> On 2015/03/24 23:34:00, Antony Sargent wrote:
> > On 2015/03/17 12:18:27, Marc Treib wrote:
> > > On 2015/03/16 17:48:22, Antony Sargent wrote:
> > > > Why would the manifest be empty?
> > > 
> > > It is empty in the bundle install case. I'm (ab)using the
> > WebstoreInstallHelper
> > > to get an actual icon from the icon URL, but the bundle doesn't have a
> > manifest.
> > 
> > Hmm, this seems like a risky approach, since all the other code which uses
> this
> > class passes a real manifest, so it seems like it would be easy for future
> > changes to accidentally assume manifest is non-empty and break your usage. 
> > 
> > When https://codereview.chromium.org/931993002/ lands, we'll have a class
> which
> > abstracts the work of sandboxed image processing (see the
> > chrome/browser/image_decoder.h class in that CL). At that point it might
make
> > sense to just have the URLFetcher in your class, and use a ImageDecoder with
> the
> > data you fetch. 
> > 
> > In the mean time, it might be best to create a new class named something
like
> > "SafeImageFetcher" with a method something like this:
> > 
> > typedef base::Callback<const SkBitmap*, std::string error> GetImageCallback;
> > 
> > // Tries to fetch and parse the image at |image_url|, returning either a
valid
> > // image to the callback or an error describing what went wrong.
> > void GetImage(const GURL& image_url, const GetImageCallback& callback);
> > 
> > Then you can use this inside the implementation of WebstoreInstallHelper,
and
> > directory in your API implementation. 
> > 
> 
> Ack re risky.
> Actually, http://crrev.com/931993002 seems almost ready to land. So rather
than
> messing with WebstoreInstallHelper here (and causing lots of merge conflicts),
I
> think I'll just wait for it to land and rebase on top of it.

931993002 has landed (yay!) and I've rebased this on top of it. I've also
created a SafeImageFetcher class as suggested, which combines fetching the URL
and decoding the image. It's used by WebstoreInstallHelper and by the new
InstallBundle function. Consequently, WebstoreInstallHelper requires a non-empty
manifest again.

[asargent_no_longer_on_chrome, 2015-03-26 18:12:51]

Ok, getting close! Just a few nits and some possible object lifetime issues to
fix.

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:608:
icon_url = source_url().Resolve(*params_->details.icon_url);
FYI, I think the icon_url we're passed is probably always an absolute URL, so we
don't really need to resolve it as a relative URL. But I suppose having this
doesn't hurt anything, as the Resolve method handles absolute URLs just fine,
and perhaps someday could come in handy if we did want to use relative URLs.

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:624:
AddRef();
Instead of doing manual AddRef/Release calls and using base::Unretained for the
various callbacks we pass to the SafeImageFetcher above here and BundleInstaller
calls below, would it make more sense to just let the callbacks take care of the
refcounting for us?

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
(right):

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:454:
ASSERT_TRUE(base::DeleteFile(test_crx_[i], false));
nit: instead of creating the |test_crx_| files in the test_data_dir/ in the
source tree, it would be better to write all of them into a ScopedTempDir. That
accomplishes 2 things:

-You no longer need to manually delete files, as you'll get that for free from
the ScopedTempDir deleting itself when it gets destructed. This might let you
get away with not having a TearDownInProcessBrowserTestFixture override at all. 

-If the test happens to crash or fail in a way that doesn't pass through this
function, the source tree on the bot won't be left with changes in its source
tree

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/bundle_installer.cc (right):

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/bundle_installer.cc:140: [&state] (const
std::pair<std::string, Item>& entry) {
Is there any advantage to capturing state by reference [&state] instead of by
value [state] ? Since it's an enum type with only 3 possible values I'd guess
there would be no copy savings.

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.cc (right):

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.cc:55:
base::Unretained(this))));
I don't think you want base::Unretained here, right? If we do that I think there
could be a race condition that the refcount could reach 0 and the object be
deleted if the image fetcher took a long time but the work on the IO thread
finishes quickly.

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/safe_ima...
File chrome/browser/safe_image_fetcher.cc (right):

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/safe_ima...
chrome/browser/safe_image_fetcher.cc:34: base::Unretained(this)));
What if this object is deleted by its owner before this task gets a chance to
run? I think we should either make this class vend a weak ptr to itself here,
become fully ref-counted, or explicitly state in the class overview comment that
it's unsafe to delete until after the callback it was passed has been called
back.

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/safe_ima...
File chrome/browser/safe_image_fetcher.h (right):

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/safe_ima...
chrome/browser/safe_image_fetcher.h:21: class SafeImageFetcher : public
ImageDecoder::ImageRequest,
nit: 

"Every class definition should have an accompanying comment that describes what
it is for and how it should be used."

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Class_Comments

[Marc Treib, 2015-03-27 11:52:13]

Thanks for your comments - looks like I still have lots to learn about object
lifetime management!

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:608:
icon_url = source_url().Resolve(*params_->details.icon_url);
On 2015/03/26 18:12:50, Antony Sargent wrote:
> FYI, I think the icon_url we're passed is probably always an absolute URL, so
we
> don't really need to resolve it as a relative URL. But I suppose having this
> doesn't hurt anything, as the Resolve method handles absolute URLs just fine,
> and perhaps someday could come in handy if we did want to use relative URLs. 
>  

Acknowledged.

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:624:
AddRef();
On 2015/03/26 18:12:50, Antony Sargent wrote:
> Instead of doing manual AddRef/Release calls and using base::Unretained for
the
> various callbacks we pass to the SafeImageFetcher above here and
BundleInstaller
> calls below, would it make more sense to just let the callbacks take care of
the
> refcounting for us?

Indeed, that's much nicer, thanks! Done.

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
(right):

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:454:
ASSERT_TRUE(base::DeleteFile(test_crx_[i], false));
On 2015/03/26 18:12:50, Antony Sargent wrote:
> nit: instead of creating the |test_crx_| files in the test_data_dir/ in the
> source tree, it would be better to write all of them into a ScopedTempDir.
That
> accomplishes 2 things:
> 
> -You no longer need to manually delete files, as you'll get that for free from
> the ScopedTempDir deleting itself when it gets destructed. This might let you
> get away with not having a TearDownInProcessBrowserTestFixture override at
all. 
> 
> -If the test happens to crash or fail in a way that doesn't pass through this
> function, the source tree on the bot won't be left with changes in its source
> tree

I've switched to using a ScopedTempDir, so now I don't need to cleanup manually
and don't need a TearDown anymore.
However, the temp dir still resides in the source tree, because the test server
needs to find it, and is pointed to chrome/test/data (see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/i...).
So if a test crashes, the temp files will still remain in the source tree :(

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/bundle_installer.cc (right):

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/bundle_installer.cc:140: [&state] (const
std::pair<std::string, Item>& entry) {
On 2015/03/26 18:12:50, Antony Sargent wrote:
> Is there any advantage to capturing state by reference [&state] instead of by
> value [state] ? Since it's an enum type with only 3 possible values I'd guess
> there would be no copy savings. 

You're right, there's really no point to capturing by ref here. Done.

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/webstore_install_helper.cc (right):

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/webstore_install_helper.cc:55:
base::Unretained(this))));
On 2015/03/26 18:12:50, Antony Sargent wrote:
> I don't think you want base::Unretained here, right? If we do that I think
there
> could be a race condition that the refcount could reach 0 and the object be
> deleted if the image fetcher took a long time but the work on the IO thread
> finishes quickly. 

Done.

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/safe_ima...
File chrome/browser/safe_image_fetcher.cc (right):

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/safe_ima...
chrome/browser/safe_image_fetcher.cc:34: base::Unretained(this)));
On 2015/03/26 18:12:50, Antony Sargent wrote:
> What if this object is deleted by its owner before this task gets a chance to
> run? I think we should either make this class vend a weak ptr to itself here,
> become fully ref-counted, or explicitly state in the class overview comment
that
> it's unsafe to delete until after the callback it was passed has been called
> back. 

Hm, just adding a comment feels like asking for trouble, and it's not what other
similar classes do. Full refcounting seems like overkill. So I've gone the
weak_ptr route.

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/safe_ima...
File chrome/browser/safe_image_fetcher.h (right):

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/safe_ima...
chrome/browser/safe_image_fetcher.h:21: class SafeImageFetcher : public
ImageDecoder::ImageRequest,
On 2015/03/26 18:12:51, Antony Sargent wrote:
> nit: 
> 
> "Every class definition should have an accompanying comment that describes
what
> it is for and how it should be used."
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Class_Comments

Done.

[asargent_no_longer_on_chrome, 2015-03-27 23:38:27]

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
(right):

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:454:
ASSERT_TRUE(base::DeleteFile(test_crx_[i], false));
On 2015/03/27 11:52:12, Marc Treib wrote:
> On 2015/03/26 18:12:50, Antony Sargent wrote:
> > nit: instead of creating the |test_crx_| files in the test_data_dir/ in the
> > source tree, it would be better to write all of them into a ScopedTempDir.
> That
> > accomplishes 2 things:
> > 
> > -You no longer need to manually delete files, as you'll get that for free
from
> > the ScopedTempDir deleting itself when it gets destructed. This might let
you
> > get away with not having a TearDownInProcessBrowserTestFixture override at
> all. 
> > 
> > -If the test happens to crash or fail in a way that doesn't pass through
this
> > function, the source tree on the bot won't be left with changes in its
source
> > tree
> 
> I've switched to using a ScopedTempDir, so now I don't need to cleanup
manually
> and don't need a TearDown anymore.
> However, the temp dir still resides in the source tree, because the test
server
> needs to find it, and is pointed to chrome/test/data (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/i...).
> So if a test crashes, the temp files will still remain in the source tree :(

Hmm, stepping back for a moment, do you really need to dynamically generate
these .crx files during the test? Can you just generate them once and check them
in to the source tree?

If not, I think we really don't want to be writing into the source directory. It
looks like you can tell the embedded test server to serve files out of an
arbitrary directory by calling
embedded_test_server()->ServeFilesFromDirectory(some_path). The only trick is
that you'll need to arrange in your test to change how we connect up the
webstore requests to the test server, which you currently get for free via the
inherited ExtensionWebstorePrivateApiTest::SetUpCommandLine.

[Marc Treib, 2015-03-30 08:52:17]

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
(right):

https://codereview.chromium.org/855513002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:454:
ASSERT_TRUE(base::DeleteFile(test_crx_[i], false));
On 2015/03/27 23:38:27, Antony Sargent wrote:
> On 2015/03/27 11:52:12, Marc Treib wrote:
> > On 2015/03/26 18:12:50, Antony Sargent wrote:
> > > nit: instead of creating the |test_crx_| files in the test_data_dir/ in
the
> > > source tree, it would be better to write all of them into a ScopedTempDir.
> > That
> > > accomplishes 2 things:
> > > 
> > > -You no longer need to manually delete files, as you'll get that for free
> from
> > > the ScopedTempDir deleting itself when it gets destructed. This might let
> you
> > > get away with not having a TearDownInProcessBrowserTestFixture override at
> > all. 
> > > 
> > > -If the test happens to crash or fail in a way that doesn't pass through
> this
> > > function, the source tree on the bot won't be left with changes in its
> source
> > > tree
> > 
> > I've switched to using a ScopedTempDir, so now I don't need to cleanup
> manually
> > and don't need a TearDown anymore.
> > However, the temp dir still resides in the source tree, because the test
> server
> > needs to find it, and is pointed to chrome/test/data (see
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/i...).
> > So if a test crashes, the temp files will still remain in the source tree :(
> 
> Hmm, stepping back for a moment, do you really need to dynamically generate
> these .crx files during the test? Can you just generate them once and check
them
> in to the source tree?

Yep, done!

> If not, I think we really don't want to be writing into the source directory.
It
> looks like you can tell the embedded test server to serve files out of an
> arbitrary directory by calling
> embedded_test_server()->ServeFilesFromDirectory(some_path). The only trick is
> that you'll need to arrange in your test to change how we connect up the
> webstore requests to the test server, which you currently get for free via the
> inherited ExtensionWebstorePrivateApiTest::SetUpCommandLine.

[asargent_no_longer_on_chrome, 2015-03-30 17:47:30]

Ok, glad it worked out with the test files!

lgtm

[asargent_no_longer_on_chrome, 2015-03-30 17:49:13]

On 2015/03/30 17:47:30, Antony Sargent wrote:
> Ok, glad it worked out with the test files!
> 
> lgtm

BTW, in the past the try servers / CQ have had trouble with patches that add
binary test files. I'm not sure if that problem is fixed or not, but if you get
errors that look like the .crx files can't be found when the tests run, what you
can do is extract the test files out into another CL and just commit that first
(feel free to TBR it).

[Marc Treib, 2015-03-31 09:13:20]

treib@chromium.org changed reviewers:
+ thestig@chromium.org

[Marc Treib, 2015-03-31 09:13:21]

+thestig for OWNERS approval for the new c/b/safe_image_fetcher.* (reviewed by
asargent already), as well as small changes in
c/b/ui/cocoa/extensions/extension_installed_bubble_controller.mm
PTAL, thanks!

[Lei Zhang, 2015-03-31 09:39:09]

I put the review extension_installed_bubble_controller.mm back to the extensions
folks.

https://codereview.chromium.org/855513002/diff/260001/chrome/browser/safe_ima...
File chrome/browser/safe_image_fetcher.cc (right):

https://codereview.chromium.org/855513002/diff/260001/chrome/browser/safe_ima...
chrome/browser/safe_image_fetcher.cc:47: response_code / 100 == 4 ||
response_code / 100 == 5) {
So if we somehow get respond code 900, that counts as a success?

https://codereview.chromium.org/855513002/diff/260001/chrome/browser/safe_ima...
File chrome/browser/safe_image_fetcher.h (right):

https://codereview.chromium.org/855513002/diff/260001/chrome/browser/safe_ima...
chrome/browser/safe_image_fetcher.h:22: // Helper class to fetch an image from a
given URL and decode it into a SkBitmap
This class sounds a lot like BitmapFetcher.

https://codereview.chromium.org/855513002/diff/260001/chrome/browser/safe_ima...
chrome/browser/safe_image_fetcher.h:22: // Helper class to fetch an image from a
given URL and decode it into a SkBitmap
You may want to document the thread(s) this class live on.

https://codereview.chromium.org/855513002/diff/260001/chrome/browser/safe_ima...
chrome/browser/safe_image_fetcher.h:47: };
DISALLOW_COPY_AND_ASSIGN() ?

[Lei Zhang, 2015-03-31 09:39:22]

On 2015/03/31 09:39:09, Lei Zhang wrote:
> I put the review extension_installed_bubble_controller.mm back to the
extensions
> folks.

s/put/punt/

[Marc Treib, 2015-03-31 12:48:22]

re extension_installed_bubble_controller.mm: Turns out the extension folks don't
own the c/b/ui/cocoa/extensions/ folder. asargent, have you looked at that file
anyway?

Also, by using the existing BitmapFetcher, I had to introduce some explicit
AddRef/Release again in WebstorePrivateInstallBundleFunction :-/

https://codereview.chromium.org/855513002/diff/260001/chrome/browser/safe_ima...
File chrome/browser/safe_image_fetcher.cc (right):

https://codereview.chromium.org/855513002/diff/260001/chrome/browser/safe_ima...
chrome/browser/safe_image_fetcher.cc:47: response_code / 100 == 4 ||
response_code / 100 == 5) {
On 2015/03/31 09:39:08, Lei Zhang wrote:
> So if we somehow get respond code 900, that counts as a success?

Hm, I'm guessing is_success() wouldn't be true in this case? I just copied this
code from webstore_install_helper.cc. Anyway, obsolete now that the class is
removed again. :)

https://codereview.chromium.org/855513002/diff/260001/chrome/browser/safe_ima...
File chrome/browser/safe_image_fetcher.h (right):

https://codereview.chromium.org/855513002/diff/260001/chrome/browser/safe_ima...
chrome/browser/safe_image_fetcher.h:22: // Helper class to fetch an image from a
given URL and decode it into a SkBitmap
On 2015/03/31 09:39:09, Lei Zhang wrote:
> This class sounds a lot like BitmapFetcher.

Indeed it does! I didn't know about BitmapFetcher. I've removed the new class
again, and updated its clients to use BitmapFetcher instead. Thanks for the
hint, and sorry about the noise!

[Lei Zhang, 2015-04-01 03:10:01]

I think asargent should take a look again. Happy to rubber stamp the .grd file
and CQ for you afterwards.

[asargent_no_longer_on_chrome, 2015-04-01 17:43:14]

new version lgtm

for the file
"chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm"

I guess you technically should get a review from someone listed in 

chrome/browser/ui/cocoa/OWNERS

[Marc Treib, 2015-04-02 08:54:57]

On 2015/04/01 17:43:14, Antony Sargent wrote:
> new version lgtm
> 
> for the file
> "chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm"
> 
> I guess you technically should get a review from someone listed in 
> 
> chrome/browser/ui/cocoa/OWNERS

Yup, but since thestig is already here and has offered to rubber-stamp, no need
to involve anyone else :)

[Lei Zhang, 2015-04-02 18:06:13]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2015-04-02 18:06:14]

lgtm

[commit-bot: I haz the power, 2015-04-02 18:07:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/855513002/300001

[commit-bot: I haz the power, 2015-04-03 05:27:16]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-04-03 20:32:23]

Patchset 16 (id:??) landed as
https://crrev.com/a21dcd92c7f0f0ffa39e6b0508934aae2875eb17
Cr-Commit-Position: refs/heads/master@{#323627}