Bug 106275 – Add stub for CanvasProxy

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Gregg Tavares]

Add stub for CanvasProxy

[Gregg Tavares]

Created attachment 181595 [details]
Patch

[Gregg Tavares]

Here's a first attempt at a stub for CanvasProxy which is part of the proposed solution for rendering to a canvas from a web worker.

[Kenneth Russell]

Comment on attachment 181595 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181595&action=review

> Source/WebCore/html/canvas/CanvasProxy.idl:38
> +    [Custom] DOMObject getContext(in [Optional=DefaultIsUndefined] DOMString contextId);

I know this is still being discussed on the public mailing lists, but should this follow the current spec and be setContext instead? Or use a "webkit" prefix?

[Adam Barth]

This feature should be announced on webkit-dev as described by http://www.webkit.org/coding/adding-features.html

[Adam Barth]

Comment on attachment 181595 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181595&action=review

> Source/WebCore/html/canvas/CanvasProxy.idl:27
> +    Conditional=WEBGL

Is WEBGL the right conditional to use for this feature?  I would have expected us to create a new conditional since this feature is likely to mature at a different rate than WebGL.

[Adam Barth]

Comment on attachment 181595 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181595&action=review

> Source/WebCore/GNUmakefile.list.am:83
> +	DerivedSources/WebCore/JSCanvasProxy.cpp \
> +	DerivedSources/WebCore/JSCanvasProxy.h \

It looks like you might have forgotten to "svn add" these files.  I'm surprised that you need custom bindings though.  Can the code generator not handle these cases?

[Gregg Tavares]

Since you can't create a CanvasProxy directly I was thinking that canvas.transferControlToProxy would be canvas.webkitTransferControlToProxy

What do you think?


As for the condition=WEBGL I'll fix that. Any idea where do these flags go for both idl files and .cpp files?

[Adam Barth]

> As for the condition=WEBGL I'll fix that. Any idea where do these flags go for both idl files and .cpp files?

I recommend grepping around and looking at examples.  A good example to look at might be SCRIPTED_SPEECH

[Gregg Tavares]

I'm confused

I thought DerivedSources/WebCore/JSCanvasProxy.cpp are generated from the idls. That's why they're in "DerivedSources". I've always had to add them to the various build files but never actually create them or check them in.

Am I confused?

[Adam Barth]

You're right about JSCanvasProxy.  You marked some functions [Custom] and I didn't see the implementation.

[Gregg Tavares]

Created attachment 181712 [details]
Patch

[Gregg Tavares]

Here's a simplified patch? It just adds the files and the ENABLE_CANVASPROXY flags

[Zan Dobersek]

Comment on attachment 181712 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181712&action=review

> Source/WebCore/GNUmakefile.list.am:6398
> +if ENABLE_CANVASPROXY

This part of feature addition is not required anymore on the Autotools build system. Please remove it. See below for more info.

> Tools/qmake/mkspecs/features/features.pri:21
> +    ENABLE_CANVAS_PROXY=0 \

This should probably be ENABLE_CANVASPROXY as in the rest of the patch, though ENABLE_CANVAS_PROXY fits the current feature define naming style better.

> configure.ac:621
> +AC_MSG_CHECKING([whether to enable CanvasProxy support])

No need for a new configuration flag and the subsequent checking on the Automake variable in (in this case) GNUmakefile.list.am.
The Autotools build system that the GTK port uses has recently taken a new approach on the feature addition, simplifying the work required.
More on this:
http://lists.webkit.org/pipermail/webkit-dev/2013-January/023248.html
http://trac.webkit.org/wiki/AddingFeatures#ActivatingafeatureforAutotoolsbasedports

[Gregg Tavares]

Created attachment 181727 [details]
Patch

[Gregg Tavares]

Thank you for those links. Uploaded new patch

As for ENABLE_CANVASPROXY vs ENABLE_CANVAS_PROXY given the class I'm enabling is CanvasProxy I thought one word was more appropriate.

I see ENABLE_FULLSCREEN not ENABLE_FULL_SCREEN and ENABLE_GAMEPAD not ENABLE_GAME_PAD as well as ENABLE_DATALIST_ELEMENT instead of ENABLE_DATA_LIST_ELEMENT if though that element is DataList

But I can change it if I needs to be changed.

[Adam Barth]

> As for ENABLE_CANVASPROXY vs ENABLE_CANVAS_PROXY given the class I'm enabling is CanvasProxy I thought one word was more appropriate.

I don't think we have that consistent a style for naming ENABLE macros.  I probably would have used ENABLE_CANVAS_PROXY, but I don't think it's a big deal one way or the other.  Thanks for adding the macro.

[Adam Barth]

Comment on attachment 181727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181727&action=review

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:5793
> +

This change looks spurious.

[Adam Barth]

The patch looks good, but would you be willing to email webkit-dev as mentioned in Comment #4 ?  I'm sorry if this seems like extra process, but it's how we check in with the community and avoid hard feelings later on.

[Gregg Tavares]

Created attachment 181740 [details]
Patch

[Gregg Tavares]

Sent. I was just waiting for the patch to be less embarrassing ;-)

http://lists.webkit.org/pipermail/webkit-dev/2013-January/023251.html

[Dean Jackson]

Comment on attachment 181740 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181740&action=review

> Source/WebCore/ChangeLog:18
> +        (WebCore):

I think the convention is to remove these lines now. I wish prepare-ChangeLog didn't add them.

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:38
> +ENABLE_CANVASPROXY = ;

Any reason you chose CANVASPROXY over CANVAS_PROXY which seems to be the style of all other flags?

> Source/WebCore/html/canvas/CanvasProxy.cpp:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

You probably want Google on this :)

> Source/WebCore/html/canvas/CanvasProxy.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY

And here.

> Source/WebCore/html/canvas/CanvasProxy.h:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

Ditto.

> Source/WebCore/html/canvas/CanvasProxy.h:35
> +class CanvasProxy;
> +

Don't need this.

> Source/WebCore/html/canvas/CanvasProxy.idl:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

Ditto.

[Dean Jackson]

(In reply to comment #21)

> Any reason you chose CANVASPROXY over CANVAS_PROXY which seems to be the style of all other flags?

Sorry. I missed the discussion above.

[Gregg Tavares]

Created attachment 181755 [details]
Patch

[Gregg Tavares]

Fixed the copyright stuff.
changed to ENABLE_CANVAS_PROXY

[WebKit Review Bot]

Comment on attachment 181755 [details]
Patch

Rejecting attachment 181755 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 181755 [details]
Patch

Clearing flags on attachment: 181755

Committed r139220: <http://trac.webkit.org/changeset/139220>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.