WebstorePrivate extension API cleanup, part 1

WebstorePrivate extension API cleanup, part 1:

Convert webstorePrivate functions from ChromeAsyncExtensionFunction (which is deprecated) to UIThreadExtensionFunction.

Committed: https://crrev.com/325d8a111604bc221bc318b67bf9c703da8ddb81
Cr-Commit-Position: refs/heads/master@{#315288}

[Marc Treib, 2015-02-05 15:22:27]

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

[Marc Treib, 2015-02-05 15:22:27]

Hi Benjamin,
this is the first part of the cleanup you suggested in crrev.com/850283003:
Switching from ChromeAsyncExtensionFunction::RunAsync to ExtensionFunction::Run.
So far, I've only done beginInstallWithManifest3 - please verify that this is
what you had in mind, then I'll also do the same thing for the other functions.
After that, I'll start on the result code unification (in another CL).
Thanks!

[not at google - send to devlin, 2015-02-05 17:16:18]

Thanks for this cleanup, it's great. I've left a comments, fixing or discussing
them will make future cleanup easier.

https://codereview.chromium.org/885443010/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/885443010/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:289:
default:
It's better to leave out the default case so that leaving out any unclaimed
cases is a compile error. You could initialize |code| to ERROR_NONE then
[D]CHECK_NE(ERROR_NONE) before calling RespondWithError.

https://codereview.chromium.org/885443010/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:397:
WebstorePrivateBeginInstallWithManifest3Function::RespondNowWithError(
IMO - suggestion - these 3 functions below don't save a whole lot.

RespondNowWithError(code, message)
vs
RespondNow(BuildResponseForError(code, message))

isn't that different, particularly if you call BuildResponseForError less wordy,
like "WebstoreError" (or similar). Same deal with BuildResponseForSuccess. The
advantage is fewer function calls and better composition with
Respond/RespondNow.

I make this comment because it was a deliberate decision to not supply helper
methods on ExtensionFunction like RespondError(err) and force callers to specify
Respond(Error(err)) to minimise API surface.

https://codereview.chromium.org/885443010/diff/20001/extensions/browser/exten...
File extensions/browser/extension_function.h (right):

https://codereview.chromium.org/885443010/diff/20001/extensions/browser/exten...
extensions/browser/extension_function.h:314: //
alarms::Get::Results::Create(alarm).
It's funny actually - this ResponseAction/ResponseValue was a big refactor I did
a few months ago to avoid this exact scenario - APIs specifying both an error
and a result. It's supposed to be illegal, but some APIs (notably
webstorePrivate) still do it.

One big motivating factor is that if we convert to Promise APIs we won't have a
chance to pass errors and values in, since they're 2 separate callbacks. In that
situation we'd extend Error() to also take a Value instead of these format
strings.

However, given webstorePrivate does actually rely on this behavior, I suppose we
need to account for it.

I'd write it a little differently, in the spirit of better composition with
NoArguments/OneArgument/etc family -

// Using this ResponseValue indicates something is wrong with the API.
// It shouldn't be possible to have both an error *and* some arguments.
// Some legacy APIs do rely on it though, like webstorePrivate.
ResponseValue ErrorWithArguments(ResponseValue error, ResponseValue args)

[Marc Treib, 2015-02-06 10:26:15]

Thanks for the comments, all addressed!
I'll do the same for the other webstorePrivate functions now.

https://codereview.chromium.org/885443010/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/885443010/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:289:
default:
On 2015/02/05 17:16:17, kalman wrote:
> It's better to leave out the default case so that leaving out any unclaimed
> cases is a compile error. You could initialize |code| to ERROR_NONE then
> [D]CHECK_NE(ERROR_NONE) before calling RespondWithError.

Done.

https://codereview.chromium.org/885443010/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:397:
WebstorePrivateBeginInstallWithManifest3Function::RespondNowWithError(
On 2015/02/05 17:16:17, kalman wrote:
> IMO - suggestion - these 3 functions below don't save a whole lot.
> 
> RespondNowWithError(code, message)
> vs
> RespondNow(BuildResponseForError(code, message))
> 
> isn't that different, particularly if you call BuildResponseForError less
wordy,
> like "WebstoreError" (or similar). Same deal with BuildResponseForSuccess. The
> advantage is fewer function calls and better composition with
> Respond/RespondNow.
> 
> I make this comment because it was a deliberate decision to not supply helper
> methods on ExtensionFunction like RespondError(err) and force callers to
specify
> Respond(Error(err)) to minimise API surface.

I've removed the convenience functions. You're right, it's probably
cleaner/easier to read without them.
BuildResponseForError isn't just used for Webstore errors though, so I've left
the name as it is. I can live with the wordiness.

https://codereview.chromium.org/885443010/diff/20001/extensions/browser/exten...
File extensions/browser/extension_function.h (right):

https://codereview.chromium.org/885443010/diff/20001/extensions/browser/exten...
extensions/browser/extension_function.h:314: //
alarms::Get::Results::Create(alarm).
On 2015/02/05 17:16:17, kalman wrote:
> It's funny actually - this ResponseAction/ResponseValue was a big refactor I
did
> a few months ago to avoid this exact scenario - APIs specifying both an error
> and a result. It's supposed to be illegal, but some APIs (notably
> webstorePrivate) still do it.
> 
> One big motivating factor is that if we convert to Promise APIs we won't have
a
> chance to pass errors and values in, since they're 2 separate callbacks. In
that
> situation we'd extend Error() to also take a Value instead of these format
> strings.
> 
> However, given webstorePrivate does actually rely on this behavior, I suppose
we
> need to account for it.
> 
> I'd write it a little differently, in the spirit of better composition with
> NoArguments/OneArgument/etc family -
> 
> // Using this ResponseValue indicates something is wrong with the API.
> // It shouldn't be possible to have both an error *and* some arguments.
> // Some legacy APIs do rely on it though, like webstorePrivate.
> ResponseValue ErrorWithArguments(ResponseValue error, ResponseValue args)

Done.

[Marc Treib, 2015-02-06 15:13:45]

New patchset is up which does the same thing for all the webstorePrivate
functions. PTAL!

https://codereview.chromium.org/885443010/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/885443010/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:453:
dispatcher()->delegate()->GetAssociatedWebContents(),
Could this just be chrome_details_.GetAssociatedWebContents() instead?

[not at google - send to devlin, 2015-02-06 18:14:52]

lgtm, awesome.

https://codereview.chromium.org/885443010/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/885443010/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:453:
dispatcher()->delegate()->GetAssociatedWebContents(),
On 2015/02/06 15:13:45, Marc Treib wrote:
> Could this just be chrome_details_.GetAssociatedWebContents() instead?

I expect so. You can get rid of the Dispatcher dependency then :)

(Trivia: I actually really don't like the whole GetAssociatedWebContents thing,
and plan to get rid of it someday. It doesn't quite do what you expect)

[Marc Treib, 2015-02-09 09:21:21]

New patchsets have been uploaded after l-g-t-m from kalman@chromium.org

[Marc Treib, 2015-02-09 09:21:48]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-02-09 09:22:34]

https://codereview.chromium.org/885443010/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/885443010/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:453:
dispatcher()->delegate()->GetAssociatedWebContents(),
On 2015/02/06 18:14:52, kalman wrote:
> On 2015/02/06 15:13:45, Marc Treib wrote:
> > Could this just be chrome_details_.GetAssociatedWebContents() instead?
> 
> I expect so. You can get rid of the Dispatcher dependency then :)
> 
> (Trivia: I actually really don't like the whole GetAssociatedWebContents
thing,
> and plan to get rid of it someday. It doesn't quite do what you expect)

Done.

[commit-bot: I haz the power, 2015-02-09 09:22:38]

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

[commit-bot: I haz the power, 2015-02-09 11:11:57]

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

[commit-bot: I haz the power, 2015-02-09 11:11:58]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Marc Treib, 2015-02-09 11:34:37]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2015-02-09 11:35:50]

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

[commit-bot: I haz the power, 2015-02-09 13:46:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-09 13:46:41]

Patchset 5 (id:??) landed as
https://crrev.com/325d8a111604bc221bc318b67bf9c703da8ddb81
Cr-Commit-Position: refs/heads/master@{#315288}