Bug 40854 – JSC bindings for Image Resizer API

• Component: Images
• Status: RESOLVED INVALID

[Sterling Swigart]

This patch exposes the classes and their callbacks to JSC through Image.webkitGetImage. The API is functionally incomplete, so it only will ever call the error callback or throw exceptions.

[Sterling Swigart]

Created attachment 59151 [details]
Patch 2: JSC bindings

[Sterling Swigart]

Created attachment 59155 [details]
Patch 2: JSC bindings

[Sterling Swigart]

Created attachment 59163 [details]
Patch 2: JSC bindings

[Sterling Swigart]

Created attachment 59165 [details]
Patch 2: JSC bindings

[Adam Barth]

Comment on attachment 59165 [details]
Patch 2: JSC bindings

I'm pretty sure you can create these bindings without any extra code.  If you have trouble, you should ask dumi.

WebCore/ChangeLog:59
 +  2010-06-18  Sterling Swigart  <[e-mail]>
You have two ChangeLogs in this patch.

WebCore/bindings/js/JSCustomImageResizerErrorCallback.cpp:47
 +  JSCustomImageResizerErrorCallback::JSCustomImageResizerErrorCallback(JSObject* callback, JSDOMGlobalObject* globalObject)
We should be able to auto-generate these callbacks now.  There are some examples in the database bindings.

WebCore/html/HTMLImageElement.idl:56
 +          [Custom] void webkitGetImage(in DOMString mimeType, in ImageResizerSuccessCallback successCallback, [Optional] in ImageResizerErrorCallback errorCallback, [Optional] in ResizeOptions options)
This shouldn't be custom.

[Sterling Swigart]

Created attachment 59321 [details]
Patch 2: JSC bindings

[Dumitru Daniliuc]

Comment on attachment 59321 [details]
Patch 2: JSC bindings

some drive-by comments:

WebCore/bindings/scripts/CodeGeneratorJS.pm:2182
 +                  push(@implContent, "    args.append(");
minor: it might be nicer to keep the code shorter by moving this line and the one after the if-block, inside the if-else block:
if (...)
  push(@implContent, "    args.append(jsString(exec, ${paramName}));\n");
else
  ...

or maybe even add a separate function for this? getToJSString()? i'll let adam decide what's best. :)

WebCore/bindings/scripts/CodeGeneratorJS.pm:2185
 +                   } else {
indentation is off by 1 space.

do we need to change the V8 code generator too? or toV8() works correctly on Strings?

[Adam Barth]

I'm concerned that the spec for this API hasn't received enough review.  For example, it's unclear to to implement resizeOptions in other languages, such as Objective-C.  If we want other browsers to adopt this API, we should make sure it follows the regular conventions of other DOM APIs, such as being language neutral.

[Sterling Swigart]

(In reply to comment #5)
> (From update of attachment 59165 [details])
> I'm pretty sure you can create these bindings without any extra code.  If you have trouble, you should ask dumi.
> 

Done.

> WebCore/ChangeLog:59
>  +  2010-06-18  Sterling Swigart  <[e-mail]>
> You have two ChangeLogs in this patch.
> 

Fixed.

> WebCore/bindings/js/JSCustomImageResizerErrorCallback.cpp:47
>  +  JSCustomImageResizerErrorCallback::JSCustomImageResizerErrorCallback(JSObject* callback, JSDOMGlobalObject* globalObject)
> We should be able to auto-generate these callbacks now.  There are some examples in the database bindings.
> 

Done.

> WebCore/html/HTMLImageElement.idl:56
>  +          [Custom] void webkitGetImage(in DOMString mimeType, in ImageResizerSuccessCallback successCallback, [Optional] in ImageResizerErrorCallback errorCallback, [Optional] in ResizeOptions options)
> This shouldn't be custom.

I asked WPL about this, but I am still looking for further feedback on the current state of this patch.


@Dumitru:

- I consolidated the new CodeGeneratorJS.pm code.
- This patch is only for JSC bindings--when the V8 bindings are added, CodeGeneratorV8.pm will be changed to match.

[Sterling Swigart]

Created attachment 59383 [details]
Patch 2: JSC bindings

[Sterling Swigart]

Created attachment 59387 [details]
Patch 2: JSC bindings

[Sterling Swigart]

Created attachment 59415 [details]
Patch 2: JSC bindings

[Sterling Swigart]

Created attachment 59451 [details]
Patch 2: JSC bindings

[Sterling Swigart]

Created attachment 59463 [details]
Patch 2: JSC bindings

[Sterling Swigart]

Created attachment 59465 [details]
Patch 2: JSC bindings

[WebKit Review Bot]

Attachment 59465 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/js/JSHTMLImageElementCustom.cpp:92:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/js/JSHTMLImageElementCustom.cpp:93:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/js/JSHTMLImageElementCustom.cpp:94:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/js/JSHTMLImageElementCustom.cpp:101:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/js/JSHTMLImageElementCustom.cpp:102:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/bindings/js/JSHTMLImageElementCustom.cpp:103:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 6 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sterling Swigart]

Created attachment 59467 [details]
Patch 2: JSC bindings

[WebKit Review Bot]

Attachment 59467 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3281627

[Sterling Swigart]

Created attachment 59474 [details]
Patch 2: JSC bindings

[Sterling Swigart]

Created attachment 59476 [details]
Patch 2: JSC bindings

[Sterling Swigart]

Created attachment 59478 [details]
Patch 2: JSC bindings

[WebKit Review Bot]

Attachment 59478 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3337572

[Sterling Swigart]

Created attachment 59527 [details]
Patch 2: JSC bindings

[Early Warning System Bot]

Attachment 59527 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3309603

[Sterling Swigart]

Created attachment 59530 [details]
Patch 2: JSC bindings

[WebKit Review Bot]

Attachment 59527 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3291659

[WebKit Review Bot]

Attachment 59530 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3273615

[Dumitru Daniliuc]

Just wanted to point out that the cr-linux bot (incorrectly) skips at least one step when building WebKit, so it goes red for any patch with IDL changes. If you're concerned about that bot, I'd recommend just patching the changes into a clean Chromium client and building it locally.

[Adam Barth]

(In reply to comment #28)
> Just wanted to point out that the cr-linux bot (incorrectly) skips at least one step when building WebKit, so it goes red for any patch with IDL changes. If you're concerned about that bot, I'd recommend just patching the changes into a clean Chromium client and building it locally.

What step is the bot supposed to run?  It just calls "build-webkit --chromium"

[Sterling Swigart]

(In reply to comment #29)
> (In reply to comment #28)
> > Just wanted to point out that the cr-linux bot (incorrectly) skips at least one step when building WebKit, so it goes red for any patch with IDL changes. If you're concerned about that bot, I'd recommend just patching the changes into a clean Chromium client and building it locally.
> 
> What step is the bot supposed to run?  It just calls "build-webkit --chromium"

In my case, I was developing this API under the ENABLE_IMAGE_RESIZER flag, but someone flipped it from 0 to 1 without me knowing. It shouldn't be building the stuff in the IDLs at all--which is being fixed with 41116.

[Dumitru Daniliuc]

> What step is the bot supposed to run?  It just calls "build-webkit --chromium"

It looks like it misses the "gclient sync" step (WebKitTools/Scripts/update-webkit-chromium). So whenever new files are added, the Makefile is not regenerated, which leads to build errors.