Bug 64732 - Refactor automatically generated JS DOM bindings to replace operator new with static create methods
Summary: Refactor automatically generated JS DOM bindings to replace operator new with...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on:
Blocks: 64999
  Show dependency treegraph
 
Reported: 2011-07-18 10:46 PDT by Mark Hahnenberg
Modified: 2011-07-26 14:56 PDT (History)
2 users (show)

See Also:


Attachments
Patch (31.21 KB, patch)
2011-07-19 13:41 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (48.80 KB, patch)
2011-07-19 13:51 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (55.63 KB, patch)
2011-07-19 16:37 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (61.89 KB, patch)
2011-07-21 16:43 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (61.83 KB, patch)
2011-07-26 08:42 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2011-07-18 10:46:42 PDT
This is the second of a series of bugs to refactor JSC and its related DOM bindings to use static create methods when creating heap-allocated JS objects.  The goal of this bug is to replace all instances of JSCell::operator new within the generated JS DOM bindings in WebCore with the new static create methods.

See also: https://bugs.webkit.org/show_bug.cgi?id=64466
Comment 1 Mark Hahnenberg 2011-07-19 13:41:49 PDT
Created attachment 101375 [details]
Patch
Comment 2 Darin Adler 2011-07-19 13:48:37 PDT
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.
Comment 3 Mark Hahnenberg 2011-07-19 13:51:55 PDT
Created attachment 101379 [details]
Patch
Comment 4 Darin Adler 2011-07-19 13:55:31 PDT
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 5 Early Warning System Bot 2011-07-19 14:10:15 PDT
Comment on attachment 101379 [details]
Patch

Attachment 101379 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9155296
Comment 6 Mark Hahnenberg 2011-07-19 16:37:43 PDT
Created attachment 101408 [details]
Patch
Comment 7 Mark Hahnenberg 2011-07-19 16:38:04 PDT
Ignore the patch, I didn't fix the qt errors.
Comment 8 Early Warning System Bot 2011-07-19 16:50:19 PDT
Comment on attachment 101408 [details]
Patch

Attachment 101408 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9209029
Comment 9 Darin Adler 2011-07-19 17:30:05 PDT
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 10 Sam Weinig 2011-07-19 19:22:18 PDT
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.
Comment 11 Mark Hahnenberg 2011-07-21 16:43:34 PDT
Created attachment 101665 [details]
Patch
Comment 12 Mark Hahnenberg 2011-07-21 16:48:54 PDT
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 13 WebKit Review Bot 2011-07-25 15:56:55 PDT
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
Comment 14 Mark Hahnenberg 2011-07-26 08:42:35 PDT
Created attachment 102013 [details]
Patch
Comment 15 Mark Hahnenberg 2011-07-26 08:44:20 PDT
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 16 WebKit Review Bot 2011-07-26 14:56:27 PDT
Comment on attachment 102013 [details]
Patch

Clearing flags on attachment: 102013

Committed r91790: <http://trac.webkit.org/changeset/91790>
Comment 17 WebKit Review Bot 2011-07-26 14:56:33 PDT
All reviewed patches have been landed.  Closing bug.