|Summary:||Add stub for CanvasProxy|
|Product:||WebKit||Reporter:||Gregg Tavares <gman>|
|Component:||Canvas||Assignee:||Gregg Tavares <gman>|
|Severity:||Normal||CC:||abarth, abecsi, dbates, dino, gyuyoung.kim, kbr, ojan.autocc, rakuco, syoichi, vestbo, webkit.review.bot, zan|
|Version:||528+ (Nightly build)|
Description Gregg Tavares 2013-01-07 16:33:47 PST
Add stub for CanvasProxy
Comment 2 Gregg Tavares 2013-01-07 16:36:33 PST
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.
Comment 3 Kenneth Russell 2013-01-07 16:50:02 PST
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?
Comment 4 Adam Barth 2013-01-07 16:52:37 PST
This feature should be announced on webkit-dev as described by http://www.webkit.org/coding/adding-features.html
Comment 5 Adam Barth 2013-01-07 16:53:39 PST
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.
Comment 6 Adam Barth 2013-01-07 16:55:10 PST
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?
Comment 7 Gregg Tavares 2013-01-07 16:57:09 PST
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?
Comment 8 Adam Barth 2013-01-07 17:00:12 PST
> 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
Comment 9 Gregg Tavares 2013-01-07 17:31:06 PST
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?
Comment 10 Adam Barth 2013-01-07 20:37:14 PST
You're right about JSCanvasProxy. You marked some functions [Custom] and I didn't see the implementation.
Comment 12 Gregg Tavares 2013-01-08 11:15:27 PST
Here's a simplified patch? It just adds the files and the ENABLE_CANVASPROXY flags
Comment 13 Zan Dobersek 2013-01-08 11:34:33 PST
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
Comment 15 Gregg Tavares 2013-01-08 12:23:55 PST
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.
Comment 16 Adam Barth 2013-01-08 12:51:48 PST
> 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.
Comment 17 Adam Barth 2013-01-08 12:53:07 PST
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.
Comment 18 Adam Barth 2013-01-08 12:54:29 PST
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.
Comment 20 Gregg Tavares 2013-01-08 13:07:04 PST
Sent. I was just waiting for the patch to be less embarrassing ;-) http://lists.webkit.org/pipermail/webkit-dev/2013-January/023251.html
Comment 21 Dean Jackson 2013-01-08 13:23:54 PST
Comment 22 Dean Jackson 2013-01-08 13:25:48 PST
(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.
Comment 24 Gregg Tavares 2013-01-08 14:07:52 PST
Fixed the copyright stuff. changed to ENABLE_CANVAS_PROXY
Comment 25 WebKit Review Bot 2013-01-09 10:42:32 PST
Comment on attachment 181755 [details] Patch Rejecting attachment 181755 [details] from commit-queue. firstname.lastname@example.org 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.
Comment 26 WebKit Review Bot 2013-01-09 12:21:10 PST
Comment on attachment 181755 [details] Patch Clearing flags on attachment: 181755 Committed r139220: <http://trac.webkit.org/changeset/139220>
Comment 27 WebKit Review Bot 2013-01-09 12:21:17 PST
All reviewed patches have been landed. Closing bug.