[webkit-reviews] review denied: [Bug 37930] [Win] Implement LayoutTestController::markerTextForListItem() : [Attachment 61605] 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 37930: [Win] Implement LayoutTestController::markerTextForListItem()
https://bugs.webkit.org/show_bug.cgi?id=37930

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +HRESULT WebFrame::elementFromJS(JSContextRef context, JSValueRef nodeObject,
IDOMElement **element)
> +{
> +    if (!element)
> +	   return E_POINTER;
> +
> +    *element = 0;
> +
> +    if (!context)
> +	   return E_FAIL;
> +
> +    JSLock lock(SilenceAssertionsOnly);
> +    ExecState* execState = toJS(context);
> +    if (!nodeObject)
> +	   return E_FAIL;

I'd null-check nodeObject at the same time as context.

> +    JSValue jsValue = toJS(execState, nodeObject);

I don't think the execState or jsValue local variables are useful.

> @@ -114,4 +114,6 @@ interface IWebFramePrivate : IUnknown
>      HRESULT paintScrollViewRectToContextAtPoint([in] RECT rect, [in] POINT
pt, [in] OLE_HANDLE deviceContext);
>  
>      HRESULT renderTreeAsExternalRepresentation([in] BOOL forPrinting, [out,
retval] BSTR* result);
> +
> +    [local] HRESULT elementFromJS([in] JSContextRef context, [in] JSValueRef
nodeObject, [out, retval] IDOMElement** element);

Putting this on IWebViewPrivate would be a little more conventional, I think.

> +    if (!framePrivate)
> +	   return JSRetainPtr<JSStringRef>();

You can say "return 0" instead of "return JSRetainPtr<JSStringRef>()".

> +    wstring text(textBSTR, SysStringLen(textBSTR));
> +    SysFreeString(textBSTR);
> +    JSRetainPtr<JSStringRef> markerText(Adopt,
JSStringCreateWithCharacters(text.data(), text.length()));
> +    return markerText;
>  }

Again, you can use JSStringCreateWithBSTR to make this easier and cleaner.