[Webkit-unassigned] [Bug 37929] [Mac] Implement LayoutTestController::markerTextForListItem()

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=37929


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #61603|review?                     |review+
               Flag|                            |




--- Comment #3 from Darin Adler <darin at apple.com>  2010-07-15 06:27:19 PST ---
(From update of attachment 61603)
> + 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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.