Update the EphemeralAppLauncher for use by the webstorePrivate API

Update the EphemeralAppLauncher for use by the webstorePrivate API

- The WebstoreStandaloneInstaller now supports error codes to allow
translation to API error codes.
- The EphemeralAppLauncher now also returns error codes and performs
more checks before installing an ephemeral app or enabling an
installed app.
- Introduces the ExtensionInstallChecker, which checks blacklist
state, requirement errors and management policies for a given
extension.

BUG=380980
TEST=browser_tests (EphemeralAppLauncherTest.*)
unit_tests (ExtensionInstallCheckerTest.*)

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

[tmdiep, 2014-06-17 06:53:25]

This patch is the first installment for the webstorePrivate API changes.

The size of this patch is just due to tests. The main files to review are:
webstore_standalone_installer
ephemeral_app_launcher
extension_install_checker

Thanks!

[asargent_no_longer_on_chrome, 2014-06-18 20:25:10]

[Sorry for review latency]

lgtm with some suggestions

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/apps/ephe...
File chrome/browser/apps/ephemeral_app_launcher.h (right):

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/apps/ephe...
chrome/browser/apps/ephemeral_app_launcher.h:61: LAUNCH_BLOCKED_BY_POLICY
Hmm, this enum seems to have a lot of overlap with the InstallResult enum in
WebstoreStandaloneInstaller. Would it make sense to re-use the InstallResult
ones and only have the ones unique to launching here? Or maybe have a common
place where all the common ones are listed with a name that is more generic?
e.g.

namespace extensions {
namespace webstore_install {

enum InstallOrLaunchResult {
  SUCCESS,
  UNKNOWN_ERROR,
  INVALID_ID,
  INVALID_MANIFEST,
  etc...
};


}
}

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_install_checker.h (right):

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_install_checker.h:27: // TODO(tmdiep): This
is an upgrade of ExtensionInstaller. CrxInstaller and
can you add a crbug entry and link to it here in the comment, so either you (or
someone else someday) remembers to do this? =)

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/webstore_installer_test.cc (right):

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_installer_test.cc:138: // TODO(tmdiep):
Refactor and remove the use of the command line flag.
You can link to crbug.com/357774 here

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/webstore_standalone_installer.cc (right):

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_standalone_installer.cc:302:
!approval->is_ephemeral) {
Something to think about for a future refactoring and not necessarily this CL-
can we make it so that WebstoreStandaloneInstaller doesn't have to know about
the specific concept of ephemeral apps, and instead has some generic hooks that
a subclass would implement (or a Delegate object that gets passed in)?

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/webstore_standalone_installer.h (right):

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_standalone_installer.h:76:
INSTALL_MISSING_DEPENDENCIES
optional nit: I would personally find the comments here easier to read with a
line of whitespace after each constant, e.g.

// Whatever
FOO,

// Something else
BAR


instead of

// Whatever
FOO,
// Something else
BAR


(not sure if our new 'git cl format' overlord will permit this or not though)

[tmdiep, 2014-06-18 22:28:34]

Just replied to some comments. I'll address other review comments when I get in
the office.

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/apps/ephe...
File chrome/browser/apps/ephemeral_app_launcher.h (right):

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/apps/ephe...
chrome/browser/apps/ephemeral_app_launcher.h:61: LAUNCH_BLOCKED_BY_POLICY
On 2014/06/18 20:25:10, Antony Sargent wrote:
> Hmm, this enum seems to have a lot of overlap with the InstallResult enum in
> WebstoreStandaloneInstaller. Would it make sense to re-use the InstallResult
> ones and only have the ones unique to launching here? Or maybe have a common
> place where all the common ones are listed with a name that is more generic?
> e.g.
> 
> namespace extensions {
> namespace webstore_install {
> 
> enum InstallOrLaunchResult {
>   SUCCESS,
>   UNKNOWN_ERROR,
>   INVALID_ID,
>   INVALID_MANIFEST,
>   etc...
> };
> 
> 
> }
> }

I went back and forth with this issue a fair bit. The problem is that launch
result is a superset of install result, but unfortunately you can't inherit
enums. What do you think about returning an int error code instead of enum from
all the installers. Then subclasses can add extra codes.

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/webstore_standalone_installer.cc (right):

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_standalone_installer.cc:302:
!approval->is_ephemeral) {
On 2014/06/18 20:25:10, Antony Sargent wrote:
> Something to think about for a future refactoring and not necessarily this CL-
> can we make it so that WebstoreStandaloneInstaller doesn't have to know about
> the specific concept of ephemeral apps, and instead has some generic hooks
that
> a subclass would implement (or a Delegate object that gets passed in)?
> 
> 

This is actually not specific to the EphemeralAppLauncher. If the user wants to
install an app and we already have it installed ephemerally, it just gets
promoted to a full install.

[asargent_no_longer_on_chrome, 2014-06-19 00:16:44]

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/apps/ephe...
File chrome/browser/apps/ephemeral_app_launcher.h (right):

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/apps/ephe...
chrome/browser/apps/ephemeral_app_launcher.h:61: LAUNCH_BLOCKED_BY_POLICY
On 2014/06/18 22:28:34, tmdiep wrote:
> On 2014/06/18 20:25:10, Antony Sargent wrote:
> > Hmm, this enum seems to have a lot of overlap with the InstallResult enum in
> > WebstoreStandaloneInstaller. Would it make sense to re-use the InstallResult
> > ones and only have the ones unique to launching here? Or maybe have a common
> > place where all the common ones are listed with a name that is more generic?
> > e.g.
> > 
> > namespace extensions {
> > namespace webstore_install {
> > 
> > enum InstallOrLaunchResult {
> >   SUCCESS,
> >   UNKNOWN_ERROR,
> >   INVALID_ID,
> >   INVALID_MANIFEST,
> >   etc...
> > };
> > 
> > 
> > }
> > }
> 
> I went back and forth with this issue a fair bit. The problem is that launch
> result is a superset of install result, but unfortunately you can't inherit
> enums. What do you think about returning an int error code instead of enum
from
> all the installers. Then subclasses can add extra codes.

I think it might be ok to have a single enum somewhere in a shared header file
with all the various codes, and the common ones don't have any prefix (eg
SUCCESS, UNKNOWN_ERROR) and the specific ones might (eg
LAUNCH_FEATURE_DISABLED).

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/webstore_standalone_installer.cc (right):

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_standalone_installer.cc:302:
!approval->is_ephemeral) {
On 2014/06/18 22:28:34, tmdiep wrote:
> On 2014/06/18 20:25:10, Antony Sargent wrote:
> > Something to think about for a future refactoring and not necessarily this
CL-
> > can we make it so that WebstoreStandaloneInstaller doesn't have to know
about
> > the specific concept of ephemeral apps, and instead has some generic hooks
> that
> > a subclass would implement (or a Delegate object that gets passed in)?
> > 
> > 
> 
> This is actually not specific to the EphemeralAppLauncher. If the user wants
to
> install an app and we already have it installed ephemerally, it just gets
> promoted to a full install.

Ah, ok, that makes sense.

[tmdiep, 2014-06-19 01:48:37]

Thanks for the review! All review comments addressed. I moved the result codes
to webstore_install_result.h - which combines all the install and launch result
codes.

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_install_checker.h (right):

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_install_checker.h:27: // TODO(tmdiep): This
is an upgrade of ExtensionInstaller. CrxInstaller and
On 2014/06/18 20:25:10, Antony Sargent wrote:
> can you add a crbug entry and link to it here in the comment, so either you
(or
> someone else someday) remembers to do this? =)

Done.

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/webstore_installer_test.cc (right):

https://codereview.chromium.org/339103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/webstore_installer_test.cc:138: // TODO(tmdiep):
Refactor and remove the use of the command line flag.
On 2014/06/18 20:25:10, Antony Sargent wrote:
> You can link to crbug.com/357774 here

Done.

[tmdiep, 2014-06-19 01:49:18]

+tapted for stamp on app_list/.../webstore_result

[tapted, 2014-06-19 03:01:58]

app_list/.../webstore_result* lgtm

[tmdiep, 2014-06-19 03:04:21]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-06-19 03:05:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmdiep@chromium.org/339103002/60001

[commit-bot: I haz the power, 2014-06-19 08:31:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-19 08:31:45]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[tmdiep, 2014-06-19 10:21:37]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-06-19 10:23:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmdiep@chromium.org/339103002/80001

[tmdiep, 2014-06-19 12:14:16]

The CQ bit was unchecked by tmdiep@chromium.org

[tmdiep, 2014-06-20 01:44:15]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-06-20 01:46:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmdiep@chromium.org/339103002/100001

[commit-bot: I haz the power, 2014-06-20 07:39:29]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-20 08:11:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-20 08:11:12]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[tmdiep, 2014-06-20 08:26:11]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-06-20 08:27:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmdiep@chromium.org/339103002/120001

[commit-bot: I haz the power, 2014-06-20 13:41:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-20 14:34:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-20 14:34:46]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[tmdiep, 2014-06-20 22:15:52]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-06-20 22:19:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tmdiep@chromium.org/339103002/120001

[commit-bot: I haz the power, 2014-06-21 01:19:10]

Change committed as 278890