[webkit-reviews] review granted: [Bug 37929] [Mac] Implement LayoutTestController::markerTextForListItem() : [Attachment 61603] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 37929: [Mac] Implement LayoutTestController::markerTextForListItem()
https://bugs.webkit.org/show_bug.cgi?id=37929

Attachment 61603: Patch
https://bugs.webkit.org/attachment.cgi?id=61603&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> + at 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.

> + at 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?