Summary: | Refactor automatically generated JS DOM bindings to replace operator new with static create methods | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Hahnenberg <mhahnenberg> | ||||||||||||
Component: | WebCore JavaScript | Assignee: | Mark Hahnenberg <mhahnenberg> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | oliver, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 64999 | ||||||||||||||
Attachments: |
|
Description
Mark Hahnenberg
2011-07-18 10:46:42 PDT
Created attachment 101375 [details]
Patch
Comment on attachment 101375 [details]
Patch
When changing bindings, you should run the command line tool named run-bindings-tests. It will show you the generated code so you can proofread it. Then you should use run-bindings-tests --reset-results to overwrite the results and include the changes to expected results in your patch.
Otherwise, looks great.
Created attachment 101379 [details]
Patch
Comment on attachment 101379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101379&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1492 > + if ($interfaceName eq "DOMWindow") { > + push(@implContent, "$className* ${className}::create(JSGlobalData& globalData, Structure* structure, PassRefPtr<$implType> impl, JSDOMWindowShell* windowShell)\n"); > + push(@implContent, "{\n"); > + push(@implContent, " return new (allocateCell<$className>(globalData.heap)) ${className}(globalData, structure, impl, windowShell);\n"); > + push(@implContent, "}\n\n"); > + } elsif ($dataNode->extendedAttributes->{"IsWorkerContext"}) { > + push(@implContent, "$className* ${className}::create(JSGlobalData& globalData, Structure* structure, PassRefPtr<$implType> impl)\n"); > + push(@implContent, "{\n"); > + push(@implContent, " return new (allocateCell<$className>(globalData.heap)) ${className}(globalData, structure, impl);\n"); > + push(@implContent, "}\n\n"); > + } else { > + push(@implContent, "$className* ${className}::create(Structure* structure, JSDOMGlobalObject* globalObject, PassRefPtr<$implType> impl)\n"); > + push(@implContent, "{\n"); > + push(@implContent, " return new (allocateCell<$className>(globalObject->globalData().heap)) ${className}(structure, globalObject, impl);\n"); > + push(@implContent, "}\n\n"); > + } I wonder if any of these should be inlined, and thus in the header? Comment on attachment 101379 [details] Patch Attachment 101379 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9155296 Created attachment 101408 [details]
Patch
Ignore the patch, I didn't fix the qt errors. Comment on attachment 101408 [details] Patch Attachment 101408 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9209029 Comment on attachment 101408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101408&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1520 > + #if ($interfaceName eq "DOMWindow") { > + # push(@implContent, "$className* ${className}::create(JSGlobalData& globalData, Structure* structure, PassRefPtr<$implType> impl, JSDOMWindowShell* windowShell)\n"); > + # push(@implContent, "{\n"); > + # push(@implContent, " return new (allocateCell<$className>(globalData.heap)) ${className}(globalData, structure, impl, windowShell);\n"); > + # push(@implContent, "}\n\n"); > + #} elsif ($dataNode->extendedAttributes->{"IsWorkerContext"}) { > + # push(@implContent, "$className* ${className}::create(JSGlobalData& globalData, Structure* structure, PassRefPtr<$implType> impl)\n"); > + # push(@implContent, "{\n"); > + # push(@implContent, " return new (allocateCell<$className>(globalData.heap)) ${className}(globalData, structure, impl);\n"); > + # push(@implContent, "}\n\n"); > + #} else { > + # push(@implContent, "$className* ${className}::create(Structure* structure, JSDOMGlobalObject* globalObject, PassRefPtr<$implType> impl)\n"); > + # push(@implContent, "{\n"); > + # push(@implContent, " return new (allocateCell<$className>(globalObject->globalData().heap)) ${className}(structure, globalObject, impl);\n"); > + # push(@implContent, "}\n\n"); > + #} This should be deleted, not just commented out. Comment on attachment 101408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101408&action=review Please fix the Qt and windows builds. > Source/JavaScriptCore/runtime/JSByteArray.h:82 > + static JSByteArray* create(ExecState* exec, Structure* structure, WTF::ByteArray* storage) The WTF:: should not be necessary here. > Source/WebCore/bindings/js/JSAudioConstructor.h:39 > class JSAudioConstructor : public DOMConstructorWithDocument { > - public: > + private: > JSAudioConstructor(JSC::ExecState*, JSC::Structure*, JSDOMGlobalObject*); In general, we like putting the private section below the public section. I would understand if you are trying to minimize the change to the functional aspect, but please clean this up afterwords. Created attachment 101665 [details]
Patch
Notes on the patch: I changed the perl script to generate the static create methods in the header to better facilitate inlining. I had some issues getting the patch to link on Windows due to what I *think* was the result of Visual Studio inlining the static function call in JSByteArray.h and not being able to find the resulting call to private constructor. After fiddling around with the JavaScriptCore symbol export file for a while, I moved the static create method body out of the header so as to prevent inlining, and the problem disappeared. Comment on attachment 101665 [details] Patch Rejecting attachment 101665 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2 Last 500 characters of output: 8554cd8b75b70c087929c437c4c58e84135981fe r91714 = dbcecc31ac7380ffb1cac39b7023aecc5877afbb Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Updating chromium port dependencies using gclient... ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/9244199 Created attachment 102013 [details]
Patch
I didn't make any changes to the patch except for in the WebCore ChangeLog. I couldn't reproduce the test failures, so I figured I'd let the bots do my work for me :-) Comment on attachment 102013 [details] Patch Clearing flags on attachment: 102013 Committed r91790: <http://trac.webkit.org/changeset/91790> All reviewed patches have been landed. Closing bug. |