Added support for pending extensions to ExtensionsService and

Added support for pending extensions to ExtensionsService and
ExtensionUpdater.  This is needed for theme syncing.

Basically a pending extension is an (id, update_url) pair.  This change
makes it so that one can pass pending extensions to the extension service
and they will be installed if necessary on the next auto-update cycle.

BUG=32414
TEST=unittests, trybots, in-progress theme syncing change

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

[akalin, 2010-03-24 22:30:32]

+cnygaard for review

[akalin, 2010-03-24 22:39:08]

On 2010/03/24 22:30:32, akalin wrote:
> +cnygaard for review

-cnygaard, +asargent

[asargent_no_longer_on_chrome, 2010-03-25 00:23:01]

Seems reasonable in general - I have a few questions and noted a few nits.

http://codereview.chromium.org/1232003/diff/4002/20002
File chrome/browser/extensions/extension_updater.cc (right):

http://codereview.chromium.org/1232003/diff/4002/20002#newcode675
chrome/browser/extensions/extension_updater.cc:675: int no_url_count_,
google_url_count_, other_url_count_, theme_count_;
Would it read better if these were put into a struct? (Or maybe even a little
class that had a reportStats() method that did the UMA_HISTOGRAM_COUNTS calls
for you?). Your call.

http://codereview.chromium.org/1232003/diff/4002/20002#newcode727
chrome/browser/extensions/extension_updater.cc:727: fetches.begin(); it !=
fetches.end(); ++it) {
horizontal whitespace nit: "fetches" here should line up underneath the
std::vector one line above.

http://codereview.chromium.org/1232003/diff/4002/20002#newcode732
chrome/browser/extensions/extension_updater.cc:732: }
Maybe put in a call to fetches.clear() here so that no one adds code below this
that tries to do something with the pointers in it (which are no longer owned).

http://codereview.chromium.org/1232003/diff/4002/20002#newcode743
chrome/browser/extensions/extension_updater.cc:743:
service_->pending_extensions().end()) {
You could use the ContainsKey helper from base/stl_util-inl.h here (and a few
other places in the CL).

e.g. ContainsKey(service_->pending_extensions(), id)

http://codereview.chromium.org/1232003/diff/4002/20005
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/1232003/diff/4002/20005#newcode258
chrome/browser/extensions/extensions_service.cc:258:
installer->set_original_url(download_url);
Related to my comment about whether we need this method - this block of code
looks pretty similar to what's in ExtensionsService::UpdateExtension. At a
minimum I think we want to have a helper method that they both use.

http://codereview.chromium.org/1232003/diff/4002/20005#newcode870
chrome/browser/extensions/extensions_service.cc:870: }
I think this will end up leaving a junk directory in the profile dir with the
extension's contents - we should probably clean it up here.

http://codereview.chromium.org/1232003/diff/4002/20006
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/1232003/diff/4002/20006#newcode50
chrome/browser/extensions/extensions_service.h:50: bool install_silently)
For themes we don't care too much about versions. For extensions however, if a
new version requires "more permissions" (didn't use to use NPAPI, now does,
etc.) at update time we disable the extension and pop up an infobar telling the
user that the new version has been disabled because it needs more permission,
and give them the option of enabling or uninstalling. 

It seems we have to worry about the case where a user has version 1 installed on
computer A, sync pushes that fact over to computer B, and at the same time the
extension developer pushes out version 2 with increased permissions, and
computer B fetches version 2. We want to make sure they get the same
"permissions increase" experience, so I think you'll need to either support
syncing a version or make the code restricted to themes for now.

http://codereview.chromium.org/1232003/diff/4002/20006#newcode197
chrome/browser/extensions/extensions_service.h:197: // If an extension with the
given extension is already installed,
nit: ".. given extension .." should be ".. given id .."

http://codereview.chromium.org/1232003/diff/4002/20006#newcode202
chrome/browser/extensions/extensions_service.h:202: bool is_theme, bool
install_silently);
Why not just call this AddPendingExtension? It seems like an implementation
detail that it first checks if the extension id is already installed or pending,
which callers could check themselves before calling if they cared.

http://codereview.chromium.org/1232003/diff/4002/20006#newcode210
chrome/browser/extensions/extensions_service.h:210: const GURL& download_url);
I'm not sure why we need InstallPendingExtension - can't you just check the
pending_extension_map_ inside UpdateExtension? An extension being pending is
sort of "half-installed" already.

[akalin, 2010-03-25 03:34:36]

Please take another look.

http://codereview.chromium.org/1232003/diff/4002/20002
File chrome/browser/extensions/extension_updater.cc (right):

http://codereview.chromium.org/1232003/diff/4002/20002#newcode675
chrome/browser/extensions/extension_updater.cc:675: int no_url_count_,
google_url_count_, other_url_count_, theme_count_;
On 2010/03/25 00:23:02, Antony Sargent wrote:
> Would it read better if these were put into a struct? (Or maybe even a little
> class that had a reportStats() method that did the UMA_HISTOGRAM_COUNTS calls
> for you?). Your call.

Done.

http://codereview.chromium.org/1232003/diff/4002/20002#newcode727
chrome/browser/extensions/extension_updater.cc:727: fetches.begin(); it !=
fetches.end(); ++it) {
On 2010/03/25 00:23:02, Antony Sargent wrote:
> horizontal whitespace nit: "fetches" here should line up underneath the
> std::vector one line above.

Done.

http://codereview.chromium.org/1232003/diff/4002/20002#newcode732
chrome/browser/extensions/extension_updater.cc:732: }
On 2010/03/25 00:23:02, Antony Sargent wrote:
> Maybe put in a call to fetches.clear() here so that no one adds code below
this
> that tries to do something with the pointers in it (which are no longer
owned).

Done.

http://codereview.chromium.org/1232003/diff/4002/20002#newcode743
chrome/browser/extensions/extension_updater.cc:743:
service_->pending_extensions().end()) {
Turns out I still need the iterator here, but changed all the other calls.

On 2010/03/25 00:23:02, Antony Sargent wrote:
> You could use the ContainsKey helper from base/stl_util-inl.h here (and a few
> other places in the CL).
> 
> e.g. ContainsKey(service_->pending_extensions(), id)

http://codereview.chromium.org/1232003/diff/4002/20005
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/1232003/diff/4002/20005#newcode258
chrome/browser/extensions/extensions_service.cc:258:
installer->set_original_url(download_url);
Removed.

On 2010/03/25 00:23:02, Antony Sargent wrote:
> Related to my comment about whether we need this method - this block of code
> looks pretty similar to what's in ExtensionsService::UpdateExtension. At a
> minimum I think we want to have a helper method that they both use.

http://codereview.chromium.org/1232003/diff/4002/20005#newcode870
chrome/browser/extensions/extensions_service.cc:870: }
On 2010/03/25 00:23:02, Antony Sargent wrote:
> I think this will end up leaving a junk directory in the profile dir with the
> extension's contents - we should probably clean it up here.

Probably should delete extension too.  However, I'm not sure of the best way to
clean up the extension directory.  Can you point me to similar code, or outline
what I'd have to do?  Is it simply to post a recursive delete on
extension->path() on the IO thread?

http://codereview.chromium.org/1232003/diff/4002/20006
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/1232003/diff/4002/20006#newcode50
chrome/browser/extensions/extensions_service.h:50: bool install_silently)
I see what you mean.  Does it suffice to carry around the version info?  I did
that, but if there's more to be done, I'd prefer to just put a TODO somewhere.

On 2010/03/25 00:23:02, Antony Sargent wrote:
> For themes we don't care too much about versions. For extensions however, if a
> new version requires "more permissions" (didn't use to use NPAPI, now does,
> etc.) at update time we disable the extension and pop up an infobar telling
the
> user that the new version has been disabled because it needs more permission,
> and give them the option of enabling or uninstalling. 
> 
> It seems we have to worry about the case where a user has version 1 installed
on
> computer A, sync pushes that fact over to computer B, and at the same time the
> extension developer pushes out version 2 with increased permissions, and
> computer B fetches version 2. We want to make sure they get the same
> "permissions increase" experience, so I think you'll need to either support
> syncing a version or make the code restricted to themes for now.
>

http://codereview.chromium.org/1232003/diff/4002/20006#newcode197
chrome/browser/extensions/extensions_service.h:197: // If an extension with the
given extension is already installed,
On 2010/03/25 00:23:02, Antony Sargent wrote:
> nit: ".. given extension .." should be ".. given id .."

Done.

http://codereview.chromium.org/1232003/diff/4002/20006#newcode202
chrome/browser/extensions/extensions_service.h:202: bool is_theme, bool
install_silently);
On 2010/03/25 00:23:02, Antony Sargent wrote:
> Why not just call this AddPendingExtension? It seems like an implementation
> detail that it first checks if the extension id is already installed or
pending,
> which callers could check themselves before calling if they cared.

Done.

http://codereview.chromium.org/1232003/diff/4002/20006#newcode210
chrome/browser/extensions/extensions_service.h:210: const GURL& download_url);
You're right; I merged the two.

On 2010/03/25 00:23:02, Antony Sargent wrote:
> I'm not sure why we need InstallPendingExtension - can't you just check the
> pending_extension_map_ inside UpdateExtension? An extension being pending is
> sort of "half-installed" already.

[asargent_no_longer_on_chrome, 2010-03-25 17:06:41]

Looking good, just a couple minor issues.

http://codereview.chromium.org/1232003/diff/33001/17005
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/1232003/diff/33001/17005#newcode224
chrome/browser/extensions/extensions_service.cc:224: void DeleteFileHelper(const
FilePath& path, bool recursive) {
Add DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE)); to make sure this
doesn't accidentally get called on the UI thread.

http://codereview.chromium.org/1232003/diff/33001/17005#newcode886
chrome/browser/extensions/extensions_service.cc:886: // TODO(akalin): Clean up
extension directory.
Why not post a DeleteFileHelper task here? I believe the path is available at
extension->path() BTW.

http://codereview.chromium.org/1232003/diff/33001/17006
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/1232003/diff/33001/17006#newcode51
chrome/browser/extensions/extensions_service.h:51: const std::string& version,
It would be nice to make sure a PendingExtensionInfo can never have an invalid
version string - maybe you should have the constructor take a const Version& or
Version* instead of a std::string representing a version.

http://codereview.chromium.org/1232003/diff/33001/17006#newcode200
chrome/browser/extensions/extensions_service.h:200: const std::string& version,
bool is_theme, bool install_silently);
Same thing here - maybe take a const Version& or Version* instead of
std::string.

[akalin, 2010-03-25 21:44:49]

Please take another look.

http://codereview.chromium.org/1232003/diff/33001/17005
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/1232003/diff/33001/17005#newcode224
chrome/browser/extensions/extensions_service.cc:224: void DeleteFileHelper(const
FilePath& path, bool recursive) {
On 2010/03/25 17:06:41, Antony Sargent wrote:
> Add DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE)); to make sure this
> doesn't accidentally get called on the UI thread.

Done.

http://codereview.chromium.org/1232003/diff/33001/17005#newcode886
chrome/browser/extensions/extensions_service.cc:886: // TODO(akalin): Clean up
extension directory.
On 2010/03/25 17:06:41, Antony Sargent wrote:
> Why not post a DeleteFileHelper task here? I believe the path is available at
> extension->path() BTW. 

Done.

http://codereview.chromium.org/1232003/diff/33001/17006
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/1232003/diff/33001/17006#newcode51
chrome/browser/extensions/extensions_service.h:51: const std::string& version,
Done.  Depends on http://codereview.chromium.org/1364002 .

On 2010/03/25 17:06:41, Antony Sargent wrote:
> It would be nice to make sure a PendingExtensionInfo can never have an invalid
> version string - maybe you should have the constructor take a const Version&
or
> Version* instead of a std::string representing a version.

http://codereview.chromium.org/1232003/diff/33001/17006#newcode200
chrome/browser/extensions/extensions_service.h:200: const std::string& version,
bool is_theme, bool install_silently);
On 2010/03/25 17:06:41, Antony Sargent wrote:
> Same thing here - maybe take a const Version& or Version* instead of
> std::string.

Done.

[asargent_no_longer_on_chrome, 2010-03-25 21:54:35]

LGTM