Bug 37929 – [Mac] Implement LayoutTestController::markerTextForListItem()

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jakub Wieczorek]

Added in http://trac.webkit.org/changeset/57986 and used in the following tests:

fast/lists/ol-nested-items-dynamic-insert.html
fast/lists/ol-nested-items-dynamic-remove.html
fast/lists/ol-nested-items.html
fast/lists/ol-nested-list-dynamic-insert.html
fast/lists/ol-nested-list-dynamic-remove.html
fast/lists/ol-nested-list.html

[Daniel Bates]

Created attachment 61603 [details]
Patch

I thought to split up the Mac and Windows patches and use Bug #42054 as a meta bug.

With insight from Adam Roben, I updated the patch based on Darin Adler's comment in Bug #42054 <https://bugs.webkit.org/show_bug.cgi?id=42054#c4>.

Also, added class function +[DOMElement _DOMElementFromJSContext] to convert a JSContextRef and JSValueRef into a DOMElement.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/mac/DOM/WebDOMOperationsPrivate.h:32:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 8 files


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

[Darin Adler]

Comment on attachment 61603 [details]
Patch

> +@implementation DOMElement (WebDOMElementOperationsPrivate)

I suggest a blank line after "@implementation" here.

> +    if (!context)
> +        return 0;
> +
> +    JSLock lock(SilenceAssertionsOnly);
> +    ExecState* execState = toJS(context);
> +    if (!value)
> +        return 0;

No reason to wait until after taking JSLock to null-check value.

> +
> +    JSValue jsValue = toJS(execState, value);
> +    Element* element = toElement(jsValue);
> +    return element ? (DOMElement *)kit(element) : 0;
> +}

The kit function can handle null. You should be able to write it like this:

    return kit(toElement(toJS(toJS(context), value)));

Without all the local variables and the type casts.

> +
> +- (NSString *)_markerTextForListItem
> +{
> +    return WebCore::markerTextForListItem(static_cast<WebCore::Element*>(core(self)));

Do you need the type cast? I thought core and kit had type-specific versions.

> +@end

I suggest a line before "@end".

> +    JSRetainPtr<JSStringRef> markerText(Adopt, JSStringCreateWithCFString((CFStringRef)[element _markerTextForListItem]));
> +    return markerText;

Will this work for elements that return nil for the string?

[Daniel Bates]

Thank you for the review.

(In reply to comment #3)
> (From update of attachment 61603 [details])
> > +@implementation DOMElement (WebDOMElementOperationsPrivate)
> 
> I suggest a blank line after "@implementation" here.

Will add blank line before I land.

> 
> > +    if (!context)
> > +        return 0;
> > +
> > +    JSLock lock(SilenceAssertionsOnly);
> > +    ExecState* execState = toJS(context);
> > +    if (!value)
> > +        return 0;
> 
> No reason to wait until after taking JSLock to null-check value.

Will change before I land.

> 
> > +
> > +    JSValue jsValue = toJS(execState, value);
> > +    Element* element = toElement(jsValue);
> > +    return element ? (DOMElement *)kit(element) : 0;
> > +}
> 
> The kit function can handle null. You should be able to write it like this:
> 
>     return kit(toElement(toJS(toJS(context), value)));
> 
> Without all the local variables and the type casts.

Will change before I land.

> 
> > +
> > +- (NSString *)_markerTextForListItem
> > +{
> > +    return WebCore::markerTextForListItem(static_cast<WebCore::Element*>(core(self)));
> 
> Do you need the type cast? I thought core and kit had type-specific versions.

The cast is not necessary, will import DOMElementInternal.h to use the type-specific kit() (some how missed this file when scanning through the source).

> 
> > +@end
> 
> I suggest a line before "@end".

Will add blank line.

> 
> > +    JSRetainPtr<JSStringRef> markerText(Adopt, JSStringCreateWithCFString((CFStringRef)[element _markerTextForListItem]));
> > +    return markerText;
> 
> Will this work for elements that return nil for the string?

-[DOMElement _markerTextForListItem] never returns the nil object. Moreover, it does work correctly should WebCore::markerTextForListItem() return a null string (i.e. WebCore::String()).

[Daniel Bates]

Committed r63445: <http://trac.webkit.org/changeset/63445>