Bug 149554 – Add support for WebIDL JSBuiltin attributes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

JSBuiltin is currently only supported for methods.

[youenn fablet]

Created attachment 261922 [details]
Patch

[WebKit Commit Bot]

Attachment 261922 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/Lookup.cpp:39:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/Lookup.cpp:40:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/Lookup.cpp:42:  Wrong number of spaces before statement. (expected: 16)  [whitespace/indent] [4]
Total errors found: 3 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xabier Rodríguez Calvar]

LGTM

[Darin Adler]

Comment on attachment 261922 [details]
Patch

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

> Source/JavaScriptCore/runtime/Lookup.cpp:40
> +        accessor->setGetter(vm, globalObject, value.attributes() & Builtin ?
> +            JSFunction::createBuiltinFunction(vm, value.builtinAccessorGetterGenerator()(vm), globalObject, *getterName) :
> +            JSFunction::create(vm, globalObject, 0, *getterName, value.accessorGetter()));

WebKit coding style formatting for this puts the ? and the : at the beginning of the second and third lines, not the end of the first and second.

> Source/JavaScriptCore/runtime/Lookup.h:82
> +    BuiltinGenerator builtinAccessorGetterGenerator() const { ASSERT(m_attributes & Accessor && m_attributes & Builtin); return reinterpret_cast<BuiltinGenerator>(m_values.value1); }
> +    BuiltinGenerator builtinAccessorSetterGenerator() const { ASSERT(m_attributes & Accessor && m_attributes & Builtin); return reinterpret_cast<BuiltinGenerator>(m_values.value2); }

Normally we’d write these as two separate assertions rather than using &&.

[youenn fablet]

Created attachment 262046 [details]
Patch for landing

[youenn fablet]

Thanks for the review!

(In reply to comment #4)
> Comment on attachment 261922 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261922&action=review
> 
> > Source/JavaScriptCore/runtime/Lookup.cpp:40
> > +        accessor->setGetter(vm, globalObject, value.attributes() & Builtin ?
> > +            JSFunction::createBuiltinFunction(vm, value.builtinAccessorGetterGenerator()(vm), globalObject, *getterName) :
> > +            JSFunction::create(vm, globalObject, 0, *getterName, value.accessorGetter()));
> 
> WebKit coding style formatting for this puts the ? and the : at the
> beginning of the second and third lines, not the end of the first and second.

Fixed.

> 
> > Source/JavaScriptCore/runtime/Lookup.h:82
> > +    BuiltinGenerator builtinAccessorGetterGenerator() const { ASSERT(m_attributes & Accessor && m_attributes & Builtin); return reinterpret_cast<BuiltinGenerator>(m_values.value1); }
> > +    BuiltinGenerator builtinAccessorSetterGenerator() const { ASSERT(m_attributes & Accessor && m_attributes & Builtin); return reinterpret_cast<BuiltinGenerator>(m_values.value2); }
> 
> Normally we’d write these as two separate assertions rather than using &&.

Fixed by moving the implementation as inline.

[WebKit Commit Bot]

Comment on attachment 262046 [details]
Patch for landing

Clearing flags on attachment: 262046

Committed r190305: <http://trac.webkit.org/changeset/190305>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.