Bug 64732 – Refactor automatically generated JS DOM bindings to replace operator new with static create methods

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Mark Hahnenberg]

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

[Mark Hahnenberg]

Created attachment 101375 [details]
Patch

[Darin Adler]

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.

[Mark Hahnenberg]

Created attachment 101379 [details]
Patch

[Darin Adler]

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?

[Early Warning System Bot]

Comment on attachment 101379 [details]
Patch

Attachment 101379 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9155296

[Mark Hahnenberg]

Created attachment 101408 [details]
Patch

[Mark Hahnenberg]

Ignore the patch, I didn't fix the qt errors.

[Early Warning System Bot]

Comment on attachment 101408 [details]
Patch

Attachment 101408 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9209029

[Darin Adler]

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.

[Sam Weinig]

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.

[Mark Hahnenberg]

Created attachment 101665 [details]
Patch

[Mark Hahnenberg]

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.

[WebKit Review Bot]

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

[Mark Hahnenberg]

Created attachment 102013 [details]
Patch

[Mark Hahnenberg]

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 :-)

[WebKit Review Bot]

Comment on attachment 102013 [details]
Patch

Clearing flags on attachment: 102013

Committed r91790: <http://trac.webkit.org/changeset/91790>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.