Add stub for CanvasProxy
Created attachment 181595 [details] Patch
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 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?
This feature should be announced on webkit-dev as described by http://www.webkit.org/coding/adding-features.html
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 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?
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?
> 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
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?
You're right about JSCanvasProxy. You marked some functions [Custom] and I didn't see the implementation.
Created attachment 181712 [details] Patch
Here's a simplified patch? It just adds the files and the ENABLE_CANVASPROXY flags
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
Created attachment 181727 [details] Patch
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.
> 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 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.
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.
Created attachment 181740 [details] Patch
Sent. I was just waiting for the patch to be less embarrassing ;-) http://lists.webkit.org/pipermail/webkit-dev/2013-January/023251.html
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.
(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.
Created attachment 181755 [details] Patch
Fixed the copyright stuff. changed to ENABLE_CANVAS_PROXY
Comment on attachment 181755 [details] Patch Rejecting attachment 181755 [details] from commit-queue. gman@google.com 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 on attachment 181755 [details] Patch Clearing flags on attachment: 181755 Committed r139220: <http://trac.webkit.org/changeset/139220>
All reviewed patches have been landed. Closing bug.