webstore_private API implementations now uses JSON compiler generated code

webstore_private API implementations now uses JSON compiler generated code rather than manually (un)marshalling dict arguments.

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

[pals, 2013-08-12 10:30:38]

Please review.

[not at google - send to devlin, 2013-08-12 20:48:38]

https://codereview.chromium.org/22815002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/22815002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:207:
EXTENSION_FUNCTION_VALIDATE(params.get());
VALIDATE(params) is enough

https://codereview.chromium.org/22815002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:222:
ReadBundleInfo(InstallBundle::Params* params,
make this a const ref?

https://codereview.chromium.org/22815002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:270:
CHECK(params.get());
270 is redundant, EFV(params) will return if it fails.

https://codereview.chromium.org/22815002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:281: if
(params->details.icon_data.get() && params->details.icon_url.get()) {
I won't make this comment on every instance of this since it'll be too noisy;
but scoped_ptrs know how to be tested in boolean operations, so you just need

params->details.icon_data && params->details.icon_url

no .get()s.

https://codereview.chromium.org/22815002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:287:
if(params->details.icon_data.get())
space between if and (, ditto elsewhere.

https://codereview.chromium.org/22815002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:308:
enable_launcher_ = *params->details.enable_launcher;
all of this assigning into variables makes me thing we should just hold onto the
params as a member variable directly rather than expanding it out like this.

https://codereview.chromium.org/22815002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:375:
CHECK(false);
pull this switch into a separate function which returns the appropriate string.
It'll be more concise.

SetResultCode(code) {
  results_ = BIWM3::Results::Create(ResultCodeToString(code));
}

const char* ResultCodeToString(code) {
  switch (code) {
    case ERROR_NONE:
      return "";
  }
}

etc.

then, don't have a default case (so that compiler will error if an enum case is
missing) then NOTREACHED if it fails to return. And return "" I guess.

https://codereview.chromium.org/22815002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:679:
SendResponse(true);
Make these SyncExtensionFunctions then you won't need the SendResponse(true),
return true is enough.

You actually might not even need it either way, check the implementation of
AsyncExtensionFunction.

[pals, 2013-08-14 09:12:30]

Updated the patch. Please review.

https://codereview.chromium.org/22815002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/22815002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:207:
EXTENSION_FUNCTION_VALIDATE(params.get());
On 2013/08/12 20:48:38, kalman wrote:
> VALIDATE(params) is enough

Done.

https://codereview.chromium.org/22815002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:222:
ReadBundleInfo(InstallBundle::Params* params,
On 2013/08/12 20:48:38, kalman wrote:
> make this a const ref?

Done.

https://codereview.chromium.org/22815002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:270:
CHECK(params.get());
On 2013/08/12 20:48:38, kalman wrote:
> 270 is redundant, EFV(params) will return if it fails.

Done.

https://codereview.chromium.org/22815002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:281: if
(params->details.icon_data.get() && params->details.icon_url.get()) {
On 2013/08/12 20:48:38, kalman wrote:
> I won't make this comment on every instance of this since it'll be too noisy;
> but scoped_ptrs know how to be tested in boolean operations, so you just need
> 
> params->details.icon_data && params->details.icon_url
> 
> no .get()s.

Done.

https://codereview.chromium.org/22815002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:287:
if(params->details.icon_data.get())
On 2013/08/12 20:48:38, kalman wrote:
> space between if and (, ditto elsewhere.

Done.

https://codereview.chromium.org/22815002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:308:
enable_launcher_ = *params->details.enable_launcher;
On 2013/08/12 20:48:38, kalman wrote:
> all of this assigning into variables makes me thing we should just hold onto
the
> params as a member variable directly rather than expanding it out like this.

Done.

https://codereview.chromium.org/22815002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:375:
CHECK(false);
On 2013/08/12 20:48:38, kalman wrote:
> pull this switch into a separate function which returns the appropriate
string.
> It'll be more concise.
> 
> SetResultCode(code) {
>   results_ = BIWM3::Results::Create(ResultCodeToString(code));
> }
> 
> const char* ResultCodeToString(code) {
>   switch (code) {
>     case ERROR_NONE:
>       return "";
>   }
> }
> 
> etc.
> 
> then, don't have a default case (so that compiler will error if an enum case
is
> missing) then NOTREACHED if it fails to return. And return "" I guess.

Done.

https://codereview.chromium.org/22815002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:679:
SendResponse(true);
On 2013/08/12 20:48:38, kalman wrote:
> Make these SyncExtensionFunctions then you won't need the SendResponse(true),
> return true is enough.
> 
> You actually might not even need it either way, check the implementation of
> AsyncExtensionFunction.

Done.

[not at google - send to devlin, 2013-08-14 14:58:13]

lgtm

https://codereview.chromium.org/22815002/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/22815002/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:266:
params_.reset(BeginInstallWithManifest3::Params::Create(*args_).release());
better:

params_ = BeginInstallWithManifest3::Params::Create(*args_).Pass();

and you might not even need that Pass(), I can't remember.

https://codereview.chromium.org/22815002/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:307:
*params_->details.icon_data, icon_url, context_getter);
icon_data is optional and might be null here. I suggest assigning it to a local
variable around about 291 in a similar way to icon_url. Seems like it defaults
to null.

https://codereview.chromium.org/22815002/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:369:
*params_->details.localized_name,
this can also be null, needs to default to "".

[pals, 2013-08-16 08:17:24]

Please review one more time.

https://codereview.chromium.org/22815002/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/22815002/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:266:
params_.reset(BeginInstallWithManifest3::Params::Create(*args_).release());
On 2013/08/14 14:58:13, kalman wrote:
> better:
> 
> params_ = BeginInstallWithManifest3::Params::Create(*args_).Pass();
> 
> and you might not even need that Pass(), I can't remember.

Done.

https://codereview.chromium.org/22815002/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:307:
*params_->details.icon_data, icon_url, context_getter);
On 2013/08/14 14:58:13, kalman wrote:
> icon_data is optional and might be null here. I suggest assigning it to a
local
> variable around about 291 in a similar way to icon_url. Seems like it defaults
> to null.

Done. Defaulted to std::string().

https://codereview.chromium.org/22815002/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:369:
*params_->details.localized_name,
On 2013/08/14 14:58:13, kalman wrote:
> this can also be null, needs to default to "".

Done.

[commit-bot: I haz the power, 2013-08-16 08:17:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/22815002/12001

[commit-bot: I haz the power, 2013-08-16 13:58:59]

Change committed as 218012