Bug 37930 – [Win] Implement LayoutTestController::markerTextForListItem()

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jakub Wieczorek]

Added in http://trac.webkit.org/changeset/57986 and used in the following tests:

fast/lists/ol-nested-items-dynamic-insert.html
fast/lists/ol-nested-items-dynamic-remove.html
fast/lists/ol-nested-items.html
fast/lists/ol-nested-list-dynamic-insert.html
fast/lists/ol-nested-list-dynamic-remove.html
fast/lists/ol-nested-list.html

[Daniel Bates]

Created attachment 61605 [details]
Patch

I thought to split up the Mac and Windows patches and use Bug #42054 as a meta bug.

With insight from Adam Roben, I updated the patch based on Darin Adler's comment in Bug #42054 <https://bugs.webkit.org/show_bug.cgi?id=42054#c4>. I thought to keep the implementation of both ports similar for consistency. So, I added a similar DOMElement conversion function to the Windows port, WebFrame::elementFromJS (I am open to naming suggestions).

I was not sure where to place WebFrame::elementFromJS. My original thought was to add a class method to DOMElement (similar to the class method added to the WebDOMElementOperationsPrivate category of DOMElement in the Mac patch - see bug #37929), but I felt that keeping the function in some kind of private interface better demarcated that this function is not for public usage.

I am open to suggestions on this patch.

[WebKit Review Bot]

Attachment 61605 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3576007

[Brian Weinstein]

Make sure you touch WebKit.idl as part of your patch so the EWS bot rebuilds the idl files you need.

[Adam Roben (:aroben)]

Comment on attachment 61605 [details]
Patch

> +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.

[Daniel Bates]

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

Will change.

> 
> > +    JSValue jsValue = toJS(execState, nodeObject);
> 
> I don't think the execState or jsValue local variables are useful.

Will remove these local variables and write "Element* elt = toElement(jsValue);" as "Element *elt = toElement(toJS(toJS(context), value));".

> 
> > @@ -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.

Will move to IWebViewPrivate.

> 
> > +    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.

Will change.

[Daniel Bates]

(In reply to comment #5)
> Will remove these local variables and write "Element* elt = toElement(jsValue);" as "Element *elt = toElement(toJS(toJS(context), value));".
> 

I meant write as "Element* elt = toElement(toJS(toJS(context), nodeObject));"

[Daniel Bates]

Created attachment 61739 [details]
Patch

Updated patch based on Adam Roben's comments.

[WebKit Review Bot]

Attachment 61739 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3352713

[Adam Roben (:aroben)]

Comment on attachment 61739 [details]
Patch

r=me

[Daniel Bates]

Committed r63710: <http://trac.webkit.org/changeset/63710>