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

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Daniel Bates <dbates at webkit.org>  2010-07-15 12:13:58 PST ---
Thank you for the review.

(In reply to comment #3)
> (From update of attachment 61603 [details])
> > + at 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).

> 
> > + at 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()).

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