[webkit-reviews] review denied: [Bug 42054] Implement LayoutTestController::markerTextForListItem() for Mac and Windows DRT : [Attachment 61191] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 42054: Implement LayoutTestController::markerTextForListItem() for Mac and
Windows DRT
https://bugs.webkit.org/show_bug.cgi?id=42054

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +- (JSValueRef)_markerTextForListItem:(JSContextRef)context
forElement:(JSValueRef)value
> +{
> +    JSLock lock(SilenceAssertionsOnly);
> +    ExecState* execState = toJS(context);
> +    if (!value)
> +	   return JSValueMakeUndefined(context);
> +    JSValue jsValue = toJS(execState, value);
> +    Element* element = toElement(jsValue);

I assume toElement checks that jsValue really represents an Element?

> +    if (!element)
> +	   return JSValueMakeUndefined(context);
> +    String markerText = markerTextForListItem(element);
> +    JSRetainPtr<JSStringRef> markerTextJS(Adopt,
JSStringCreateWithUTF8CString(markerText.utf8().data()));
> +    return JSValueMakeString(context, markerTextJS.get());
> +}

It seems strange to convert a string to a JSValue, only to convert it back to a
string. It's also weird that a method that claims to return "text" returns a
JSValueRef. Why not return an NSString on Mac and a BSTR on Windows?

> +HRESULT STDMETHODCALLTYPE WebView::markerTextForListItem(
> +    /* [in] */ JSContextRef context,
> +    /* [in] */ JSValueRef nodeObject,
> +    /* [out, retval] */ JSValueRef* markerText)
> +{
> +    if (!context)
> +	   return E_FAIL;

It's conventional to first check that markerText is non-null. If it is null,
you should bail and return E_POINTER.

> +++ WebKit/win/Interfaces/IWebViewPrivate.idl (working copy)
> @@ -236,5 +236,7 @@ interface IWebViewPrivate : IUnknown
>  
>      HRESULT paintScrollViewRectToContextAtPoint([in] RECT rect, [in] POINT
pt, [in] OLE_HANDLE dc);
>  
> +    [local] HRESULT markerTextForListItem([in] JSContextRef context, [in]
JSValueRef nodeObject, [out, retval] JSValueRef* markerText);
> +
>      [local] HRESULT reportException([in] JSContextRef context, [in]
JSValueRef exception);
>  }

Your new function must be added to the end of the interface to avoid breaking
nightly builds.