Introduce a stable set of errors for inline install

Introduce a stable set of errors for inline install

In addition to a developer-readable string for inline install failures, include
a stable error code. This isn't perfect, since a) the function prototype of
function(error, errorCode) isn't great, and b) we kind of lose track of what
went wrong after a certain point in the install process. Unfortunately, to fix
a) requires either a breaking change or a revamp to our whole error reporting,
and fixing b) requires stable error values at every point in our install
process (which we don't have, and won't anytime soon).

BUG=331887
TBR=benwells@chromium.org (app_list)
TBR=yoshiki@chromium.org (file_manager)

TBRs for trivial, no-functionality changes.

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

[not at google - send to devlin, 2014-07-10 20:54:28]

https://codereview.chromium.org/384823002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/webstore.json (right):

https://codereview.chromium.org/384823002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/webstore.json:16: {
Add a comment in here (we support comments in the JSON, obviously) explaining
what to keep in sync.

https://codereview.chromium.org/384823002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/webstore.json:19: "enum": [{"description": "An
uncommon, unrecognized, or unexpected error. In some cases, the readable error
string can provide more information.", "name": "otherError"}, {"description":
"The operation was aborted as the requestor is no longer alive.", "name":
"aborted"}, {"description": "The installation is not permitted.", "name":
"notPermitted"}, {"description": "Invalid Chrome Web Store item ID.", "name":
"invalidId"}, {"description": "Failed to retrieve extension metadata from the
Web Store.", "name": "webstoreRequestError"}, {"description": "The extension
metadata retrieved from the Web Store was invalid.", "name":
"invalidWebstoreResponse"}, {"description": "An error occurred while parsing the
extension manifest retrieved from the Web Store.", "name": "invalidManifest"},
{"description": "Failed to retrieve the extension's icon from the Web Store, or
the icon was invalid.", "name": "iconError"}, {"description": "The user canceled
the operation.", "name": "userCanceled"}, {"description": "The extension is
blacklisted.", "name": "blacklisted"}, {"description": "Unsatisfied
dependencies, such as shared modules.", "name": "missingDependencies"},
{"description": "Unsatisfied requirements, such as webgl.", "name":
"requirementViolations"}, {"description": "The extension is blocked by
management policies.", "name": "blockedByPolicy"}, {"description": "The launch
feature is not available.", "name": "launchFeatureDisabled"}, {"description":
"The launch feature is not supported for the extension type.", "name":
"launchUnsupportedExtensionType"}],
aand this needs a format (as discussed)

https://codereview.chromium.org/384823002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/webstore/webstore_api_constants.cc (right):

https://codereview.chromium.org/384823002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/webstore/webstore_api_constants.cc:13: //
chrome/common/extensions/webstore_install_result.h!
could you add a unit test for chrome/common/extensions/webstore_install_result.h
which sanity checks this? it can
- run from 0 to LAST and dereference this list. ASAN should catch whether it
hasn't been updated.
- assert that LAST is 15 (or whatever it is) with the failure comment being a
reminder to update this list and the one in the JSON file

https://codereview.chromium.org/384823002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/webstore/webstore_api_constants.cc:16:
"otherError",  // This corresponds to UNKNOWN_ERROR, but "other" sounds a lot
I actually prefer "unknown" :)

[Devlin, 2014-07-10 21:16:59]

https://codereview.chromium.org/384823002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/webstore/webstore_api_constants.cc (right):

https://codereview.chromium.org/384823002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/webstore/webstore_api_constants.cc:16:
"otherError",  // This corresponds to UNKNOWN_ERROR, but "other" sounds a lot
On 2014/07/10 20:54:27, kalman (OOO July 11-14) wrote:
> I actually prefer "unknown" :)

Typically, I do too.  But this is special, because we _do_ know the error - we
even send a string explaining it.  We just don't
a) keep track of it through the installation process or
b) have a mapping from our error messages to error codes.

So it seems silly to say
error: "Extensions cannot be installed for a supervised user"
errorCode: "unknownError"

At least, that's my impression.  If you still prefer unknownError, I can change
it :)

[not at google - send to devlin, 2014-07-10 21:18:38]

https://codereview.chromium.org/384823002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/webstore/webstore_api_constants.cc (right):

https://codereview.chromium.org/384823002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/webstore/webstore_api_constants.cc:16:
"otherError",  // This corresponds to UNKNOWN_ERROR, but "other" sounds a lot
On 2014/07/10 21:16:59, Devlin wrote:
> On 2014/07/10 20:54:27, kalman (OOO July 11-14) wrote:
> > I actually prefer "unknown" :)
> 
> Typically, I do too.  But this is special, because we _do_ know the error - we
> even send a string explaining it.  We just don't
> a) keep track of it through the installation process or
> b) have a mapping from our error messages to error codes.
> 
> So it seems silly to say
> error: "Extensions cannot be installed for a supervised user"
> errorCode: "unknownError"
> 
> At least, that's my impression.  If you still prefer unknownError, I can
change
> it :)

ah :) well can you rename the enum then?

[Devlin, 2014-07-10 22:11:43]

https://codereview.chromium.org/384823002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/webstore.json (right):

https://codereview.chromium.org/384823002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/webstore.json:16: {
On 2014/07/10 20:54:27, kalman (OOO July 11-14) wrote:
> Add a comment in here (we support comments in the JSON, obviously) explaining
> what to keep in sync.

Ah, forgot we allowed comments in our json.  Done.

https://codereview.chromium.org/384823002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/webstore.json:19: "enum": [{"description": "An
uncommon, unrecognized, or unexpected error. In some cases, the readable error
string can provide more information.", "name": "otherError"}, {"description":
"The operation was aborted as the requestor is no longer alive.", "name":
"aborted"}, {"description": "The installation is not permitted.", "name":
"notPermitted"}, {"description": "Invalid Chrome Web Store item ID.", "name":
"invalidId"}, {"description": "Failed to retrieve extension metadata from the
Web Store.", "name": "webstoreRequestError"}, {"description": "The extension
metadata retrieved from the Web Store was invalid.", "name":
"invalidWebstoreResponse"}, {"description": "An error occurred while parsing the
extension manifest retrieved from the Web Store.", "name": "invalidManifest"},
{"description": "Failed to retrieve the extension's icon from the Web Store, or
the icon was invalid.", "name": "iconError"}, {"description": "The user canceled
the operation.", "name": "userCanceled"}, {"description": "The extension is
blacklisted.", "name": "blacklisted"}, {"description": "Unsatisfied
dependencies, such as shared modules.", "name": "missingDependencies"},
{"description": "Unsatisfied requirements, such as webgl.", "name":
"requirementViolations"}, {"description": "The extension is blocked by
management policies.", "name": "blockedByPolicy"}, {"description": "The launch
feature is not available.", "name": "launchFeatureDisabled"}, {"description":
"The launch feature is not supported for the extension type.", "name":
"launchUnsupportedExtensionType"}],
On 2014/07/10 20:54:27, kalman (OOO July 11-14) wrote:
> aand this needs a format (as discussed)

Done.

https://codereview.chromium.org/384823002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/webstore/webstore_api_constants.cc (right):

https://codereview.chromium.org/384823002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/webstore/webstore_api_constants.cc:13: //
chrome/common/extensions/webstore_install_result.h!
On 2014/07/10 20:54:27, kalman (OOO July 11-14) wrote:
> could you add a unit test for
chrome/common/extensions/webstore_install_result.h
> which sanity checks this? it can
> - run from 0 to LAST and dereference this list. ASAN should catch whether it
> hasn't been updated.
> - assert that LAST is 15 (or whatever it is) with the failure comment being a
> reminder to update this list and the one in the JSON file

As discussed offline, a compile assert covers as much, needs less code, and has
the advantage of immediate feedback. :)

https://codereview.chromium.org/384823002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/webstore/webstore_api_constants.cc:16:
"otherError",  // This corresponds to UNKNOWN_ERROR, but "other" sounds a lot
On 2014/07/10 21:18:38, kalman (OOO July 11-14) wrote:
> On 2014/07/10 21:16:59, Devlin wrote:
> > On 2014/07/10 20:54:27, kalman (OOO July 11-14) wrote:
> > > I actually prefer "unknown" :)
> > 
> > Typically, I do too.  But this is special, because we _do_ know the error -
we
> > even send a string explaining it.  We just don't
> > a) keep track of it through the installation process or
> > b) have a mapping from our error messages to error codes.
> > 
> > So it seems silly to say
> > error: "Extensions cannot be installed for a supervised user"
> > errorCode: "unknownError"
> > 
> > At least, that's my impression.  If you still prefer unknownError, I can
> change
> > it :)
> 
> ah :) well can you rename the enum then?

Yep. :)

[not at google - send to devlin, 2014-07-10 22:39:43]

lgtm

[Devlin, 2014-07-10 22:48:51]

+palmer@ for c/c/e/chrome_extension_messages.h (added an enum param, checked for
bounds).

[palmer, 2014-07-10 22:53:21]

I'm sorry, I've got too much on my plate right now. Can you get one of the other
IPC OWNERS on it?

[Devlin, 2014-07-10 22:59:09]

s/palmer/nasko.

[nasko, 2014-07-18 13:32:14]

IPC LGTM

[Devlin, 2014-07-22 20:16:35]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-07-22 20:18:07]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/384823002/...

[commit-bot: I haz the power, 2014-07-22 23:57:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  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-07-23 00:00:39]

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

[commit-bot: I haz the power, 2014-07-23 00:00:40]

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

[Devlin, 2014-07-23 15:30:32]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-07-23 15:31:57]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/384823002/...

[yoshiki, 2014-07-23 16:36:12]

lgtm

[commit-bot: I haz the power, 2014-07-23 17:53:28]

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

[commit-bot: I haz the power, 2014-07-23 20:24:08]

Change committed as 285029

[tmdiep, 2014-07-25 00:45:04]

https://codereview.chromium.org/384823002/diff/140001/chrome/common/extension...
File chrome/common/extensions/api/webstore.json (right):

https://codereview.chromium.org/384823002/diff/140001/chrome/common/extension...
chrome/common/extensions/api/webstore.json:82: "name": "launchFeatureDisabled"
I saw this patch while merging.

These error codes are shared by the EphemeralAppLauncher and
WebstoreStandaloneInstaller, so there are a couple that will not occur for
inline installs:
- launchFeatureDisabled
- launchUnsupportedExtensionType
I think we should exclude these from this interface.

[Devlin, 2014-07-25 15:54:42]

On 2014/07/25 00:45:04, tmdiep wrote:
>
https://codereview.chromium.org/384823002/diff/140001/chrome/common/extension...
> File chrome/common/extensions/api/webstore.json (right):
> 
>
https://codereview.chromium.org/384823002/diff/140001/chrome/common/extension...
> chrome/common/extensions/api/webstore.json:82: "name": "launchFeatureDisabled"
> I saw this patch while merging.
> 
> These error codes are shared by the EphemeralAppLauncher and
> WebstoreStandaloneInstaller, so there are a couple that will not occur for
> inline installs:
> - launchFeatureDisabled
> - launchUnsupportedExtensionType
> I think we should exclude these from this interface.

I personally don't see the harm in leaving them in, especially if they do find
their way into inline install at some point.  I can't see any real harm that
comes from leaving them in (is there any I'm missing?)  And it seems a lot
easier to say "keep these lists in sync" rather than "keep these lists in sync,
except for any that currently can never occur for inline install."  But that's
just me - kalman@, any thoughts?

[not at google - send to devlin, 2014-07-25 16:00:58]

On 2014/07/25 15:54:42, Devlin wrote:
> On 2014/07/25 00:45:04, tmdiep wrote:
> >
>
https://codereview.chromium.org/384823002/diff/140001/chrome/common/extension...
> > File chrome/common/extensions/api/webstore.json (right):
> > 
> >
>
https://codereview.chromium.org/384823002/diff/140001/chrome/common/extension...
> > chrome/common/extensions/api/webstore.json:82: "name":
"launchFeatureDisabled"
> > I saw this patch while merging.
> > 
> > These error codes are shared by the EphemeralAppLauncher and
> > WebstoreStandaloneInstaller, so there are a couple that will not occur for
> > inline installs:
> > - launchFeatureDisabled
> > - launchUnsupportedExtensionType
> > I think we should exclude these from this interface.
> 
> I personally don't see the harm in leaving them in, especially if they do find
> their way into inline install at some point.  I can't see any real harm that
> comes from leaving them in (is there any I'm missing?)  And it seems a lot
> easier to say "keep these lists in sync" rather than "keep these lists in
sync,
> except for any that currently can never occur for inline install."  But that's
> just me - kalman@, any thoughts?

I can imagine a world in which inline install could be used to launch ephemeral
apps, and given it's simpler to leave them in I'm fine with that.

[tmdiep, 2014-07-27 23:33:22]

On 2014/07/25 16:00:58, kalman wrote:
> On 2014/07/25 15:54:42, Devlin wrote:
> > On 2014/07/25 00:45:04, tmdiep wrote:
> > >
> >
>
https://codereview.chromium.org/384823002/diff/140001/chrome/common/extension...
> > > File chrome/common/extensions/api/webstore.json (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/384823002/diff/140001/chrome/common/extension...
> > > chrome/common/extensions/api/webstore.json:82: "name":
> "launchFeatureDisabled"
> > > I saw this patch while merging.
> > > 
> > > These error codes are shared by the EphemeralAppLauncher and
> > > WebstoreStandaloneInstaller, so there are a couple that will not occur for
> > > inline installs:
> > > - launchFeatureDisabled
> > > - launchUnsupportedExtensionType
> > > I think we should exclude these from this interface.
> > 
> > I personally don't see the harm in leaving them in, especially if they do
find
> > their way into inline install at some point.  I can't see any real harm that
> > comes from leaving them in (is there any I'm missing?)  And it seems a lot
> > easier to say "keep these lists in sync" rather than "keep these lists in
> sync,
> > except for any that currently can never occur for inline install."  But
that's
> > just me - kalman@, any thoughts?
> 
> I can imagine a world in which inline install could be used to launch
ephemeral
> apps, and given it's simpler to leave them in I'm fine with that.

Ok, I'm fine with leaving them in. There is no real harm in doing so, only that
some developers may wonder what causes those error codes when they see it in the
API documentation.