Bug 168179 – Remove the remaining functions out of JSDOMBinding

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Sam Weinig]

Remove more functions out of JSDOMBinding

[Sam Weinig]

Created attachment 301276 [details]
Patch

[Sam Weinig]

Created attachment 301284 [details]
Patch

[Sam Weinig]

Created attachment 301290 [details]
Patch

[Darin Adler]

Comment on attachment 301290 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301290&action=review

> Source/JavaScriptCore/runtime/IteratorOperations.cpp:170
> +    JSObject* object = asObject(value);

I would have used a reference here.

> Source/JavaScriptCore/runtime/IteratorOperations.h:47
> +JS_EXPORT_PRIVATE bool hasIteratorMethod(ExecState&, JSValue);
> +
>  JS_EXPORT_PRIVATE JSValue iteratorForIterable(ExecState*, JSValue iterable);

I think these should be paragraphed together because they seem pretty closely related.

> Source/WebCore/bindings/js/JSDOMBinding.h:26
> +// FIXME: Remove this header.

What is this blocked on?

[Darin Adler]

Comment on attachment 301290 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301290&action=review

>> Source/JavaScriptCore/runtime/IteratorOperations.cpp:170
>> +    JSObject* object = asObject(value);
> 
> I would have used a reference here.

Oops, I see this is in JavaScriptCore, not WebCore, and I guess the local custom here its that we want to use pointers any time it’s a heap cell like this is.

[Sam Weinig]

(In reply to comment #4)
> Comment on attachment 301290 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301290&action=review
> 
> > Source/JavaScriptCore/runtime/IteratorOperations.cpp:170
> > +    JSObject* object = asObject(value);
> 
> I would have used a reference here.
> 
> > Source/JavaScriptCore/runtime/IteratorOperations.h:47
> > +JS_EXPORT_PRIVATE bool hasIteratorMethod(ExecState&, JSValue);
> > +
> >  JS_EXPORT_PRIVATE JSValue iteratorForIterable(ExecState*, JSValue iterable);
> 
> I think these should be paragraphed together because they seem pretty
> closely related.

Ok.

> 
> > Source/WebCore/bindings/js/JSDOMBinding.h:26
> > +// FIXME: Remove this header.
> 
> What is this blocked on?

I need to remove it and get everything building by adding the headers to the appropriate places. I want to do that in a separate pass.

Thanks for the review.

[Sam Weinig]

Committed r212207: <http://trac.webkit.org/changeset/212207>

[Michael Catanzaro]

Carlos, it broke our bindings generation tests, can you investigate please?

[Michael Catanzaro]

It broke the Mac bindings tests as well!

[Michael Catanzaro]

And it broke the cloop bot:

/Volumes/Data/slave/sierra-cloop-debug/build/Source/JavaScriptCore/runtime/Lookup.h:392:28: error: incomplete type 'JSC::JSFunction' named in nested name specifier
    return JSValue::encode(JSFunction::create(state->vm(), state->lexicalGlobalObject(), length, propertyName.publicName(), nativeFunction));
                           ^~~~~~~~~~~~
In file included from /Volumes/Data/slave/sierra-cloop-debug/build/Source/JavaScriptCore/runtime/ArrayPrototype.cpp:25:
In file included from /Volumes/Data/slave/sierra-cloop-debug/build/Source/JavaScriptCore/runtime/ArrayPrototype.h:23:
In file included from /Volumes/Data/slave/sierra-cloop-debug/build/Source/JavaScriptCore/runtime/JSArray.h:24:
In file included from /Volumes/Data/slave/sierra-cloop-debug/build/Source/JavaScriptCore/runtime/ButterflyInlines.h:28:
In file included from /Volumes/Data/slave/sierra-cloop-debug/build/Source/JavaScriptCore/runtime/ArrayStorage.h:33:
In file included from /Volumes/Data/slave/sierra-cloop-debug/build/Source/JavaScriptCore/runtime/Structure.h:28:
In file included from /Volumes/Data/slave/sierra-cloop-debug/build/Source/JavaScriptCore/runtime/ClassInfo.h:25:
In file included from /Volumes/Data/slave/sierra-cloop-debug/build/Source/JavaScriptCore/interpreter/CallFrame.h:28:
/Volumes/Data/slave/sierra-cloop-debug/build/Source/JavaScriptCore/interpreter/StackVisitor.h:41:7: note: forward declaration of 'JSC::JSFunction'
class JSFunction;
      ^

[Darin Adler]

To fix the bindings tests someone just has to regenerate the results I think. To fix the CLoop build we just need to add an include.

[Ryosuke Niwa]

Cloop build fix attempted in https://trac.webkit.org/changeset/212212.

[Ryosuke Niwa]

Rebaselined the bindings tests in https://trac.webkit.org/changeset/212213.