Support PNG and quality control in chrome.tabs.captureVisibleTab().

Support PNG and quality control in chrome.tabs.captureVisibleTab().

BUG=21072
TEST=ExtensionAPIClientTest.CaptureVisibleTab

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

[rafaelw, 2010-04-07 21:01:51]

nice.

http://codereview.chromium.org/1527015/diff/2001/3002
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/1527015/diff/2001/3002#newcode812
chrome/browser/extensions/extension_tabs_module.cc:812: Value* arg0;
nit: how about something more descriptive, like window_value?

http://codereview.chromium.org/1527015/diff/2001/3002#newcode828
chrome/browser/extensions/extension_tabs_module.cc:828: Value* arg1;
ditto: options_value

http://codereview.chromium.org/1527015/diff/2001/3002#newcode832
chrome/browser/extensions/extension_tabs_module.cc:832: image_quality_ = 90;  //
Default quality setting for lossy formats is 90.
add a const somewhere for the default quality setting.

http://codereview.chromium.org/1527015/diff/2001/3002#newcode834
chrome/browser/extensions/extension_tabs_module.cc:834: if
(!arg1->IsType(Value::TYPE_NULL)) {
Value *argx;
if (!argx->IsType(Value::TYPE_NULL)) {
  ... do work for optional arguments
}

is going to become a common idiom.

How about adding a utility function like

bool HasOptionalArgument(ListValue* args, size_t index) {
  Value* value;
  return args->Get(index, &value) && !value->IsType(Value::TYPE_NULL)
}

so that this becomes

if (HasOptionalArgument(args, 1) {
  DictionaryValue* options;
  EXTENSION_FUNCTION_VALIDATE(args->GetDictionary(1, &options);
  ...
}

http://codereview.chromium.org/1527015/diff/2001/3002#newcode848
chrome/browser/extensions/extension_tabs_module.cc:848: NOTREACHED() <<
"Extension api validation should not allow format "
I feel like this is a schema validation error and should be treated like other
EXTENSION_FUNCTION_VALIDATE cases.

http://codereview.chromium.org/1527015/diff/2001/3003
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/1527015/diff/2001/3003#newcode158
chrome/browser/extensions/extension_tabs_module.h:158: ImageFormat
image_format_;
comment private members.

http://codereview.chromium.org/1527015/diff/2001/3004
File chrome/browser/extensions/extension_tabs_module_constants.cc (right):

http://codereview.chromium.org/1527015/diff/2001/3004#newcode18
chrome/browser/extensions/extension_tabs_module_constants.cc:18: const wchar_t
kIncognitoKey[] = L"incognito";
where is this being defined & used?

[Sam Kerner (Chrome), 2010-04-08 14:42:37]

Ready for another look.

http://codereview.chromium.org/1527015/diff/2001/3002
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/1527015/diff/2001/3002#newcode812
chrome/browser/extensions/extension_tabs_module.cc:812: Value* arg0;
On 2010/04/07 21:01:51, rafaelw wrote:
> nit: how about something more descriptive, like window_value?

Done.

http://codereview.chromium.org/1527015/diff/2001/3002#newcode828
chrome/browser/extensions/extension_tabs_module.cc:828: Value* arg1;
On 2010/04/07 21:01:51, rafaelw wrote:
> ditto: options_value

Done.

http://codereview.chromium.org/1527015/diff/2001/3002#newcode832
chrome/browser/extensions/extension_tabs_module.cc:832: image_quality_ = 90;  //
Default quality setting for lossy formats is 90.
On 2010/04/07 21:01:51, rafaelw wrote:
> add a const somewhere for the default quality setting.

Done.

http://codereview.chromium.org/1527015/diff/2001/3002#newcode834
chrome/browser/extensions/extension_tabs_module.cc:834: if
(!arg1->IsType(Value::TYPE_NULL)) {
On 2010/04/07 21:01:51, rafaelw wrote:
> Value *argx;
> if (!argx->IsType(Value::TYPE_NULL)) {
>   ... do work for optional arguments
> }
> 
> is going to become a common idiom.
> 
> How about adding a utility function like
> 
> bool HasOptionalArgument(ListValue* args, size_t index) {
>   Value* value;
>   return args->Get(index, &value) && !value->IsType(Value::TYPE_NULL)
> }
> 
> so that this becomes
> 
> if (HasOptionalArgument(args, 1) {
>   DictionaryValue* options;
>   EXTENSION_FUNCTION_VALIDATE(args->GetDictionary(1, &options);
>   ...
> }

I made the function a member of AsyncExtensionFunction, so that passing in the
list of args is not needed:

  if (HasOptionalArgument(1)) {
    DictionaryValue* options;
    EXTENSION_FUNCTION_VALIDATE(args->GetDictionary(1, &options));

I like that it is not possible to pass in the wrong args list, but it is less
clear that args is accessed.  Your call if args should be a parameter or not.

http://codereview.chromium.org/1527015/diff/2001/3002#newcode848
chrome/browser/extensions/extension_tabs_module.cc:848: NOTREACHED() <<
"Extension api validation should not allow format "
On 2010/04/07 21:01:51, rafaelw wrote:
> I feel like this is a schema validation error and should be treated like other
> EXTENSION_FUNCTION_VALIDATE cases.

Good point.  Done.

http://codereview.chromium.org/1527015/diff/2001/3003
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/1527015/diff/2001/3003#newcode158
chrome/browser/extensions/extension_tabs_module.h:158: ImageFormat
image_format_;
On 2010/04/07 21:01:51, rafaelw wrote:
> comment private members.

Done.

http://codereview.chromium.org/1527015/diff/2001/3004
File chrome/browser/extensions/extension_tabs_module_constants.cc (right):

http://codereview.chromium.org/1527015/diff/2001/3004#newcode18
chrome/browser/extensions/extension_tabs_module_constants.cc:18: const wchar_t
kIncognitoKey[] = L"incognito";
On 2010/04/07 21:01:51, rafaelw wrote:
> where is this being defined & used?

I moved this constant from line 35, to put it in alphabetical order.  It is used
in the implementation of chrome.tab.createWindow(), and it is a property name in
the Tab object used in chrome.tab APIs.

[rafaelw, 2010-04-08 17:47:24]

lgtm