Bug 42054 – Implement LayoutTestController::markerTextForListItem() for Mac and Windows DRT

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

[Daniel Bates]

We should implement LayoutTestController::markerTextForListItem() for the Mac and Windows ports so that we can convert the list layout tests from render tree/pixel tests to text-based tests.

[Daniel Bates]

Created attachment 61191 [details]
Patch

I was not sure where best to put these functions in the WebKit portion of the Mac and Windows ports.

I am open to suggestions on this patch, including its approach as well as where best to place the code.

[Jakub Wieczorek]

There are already two bugs filed for Mac and Win (I should have mentioned them in the Skipped list). They should be closed once this is implemented.
https://bugs.webkit.org/show_bug.cgi?id=37929
https://bugs.webkit.org/show_bug.cgi?id=37930

[Adam Roben (:aroben)]

Comment on attachment 61191 [details]
Patch

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

[Darin Adler]

Comment on attachment 61191 [details]
Patch

It's very strange to have this doing all the JavaScript conversion. I'm not sure why _computedStyleIncludingVisitedInfo:forElement: works that way.

A logical place to put the marker text function in the Mac port would be a private function on the DOMElement class. A function to map a JSContextRef and JSValueRef to an Objective-C DOMElement* might also be useful. But I don't think that really belongs on WebView. Maybe that should be a private class method of DOMElement.

[Daniel Bates]

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

Yes.

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

Will change.

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

Will add this check.

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

Will move IDL definition to the end of the file.

[Daniel Bates]

Created attachment 61314 [details]
Patch

Updated patch based on Adam Roben's comments.

[Daniel Bates]

(In reply to comment #4)
> (From update of attachment 61191 [details])
> It's very strange to have this doing all the JavaScript conversion. I'm not sure why _computedStyleIncludingVisitedInfo:forElement: works that way.
> 
> A logical place to put the marker text function in the Mac port would be a private function on the DOMElement class. A function to map a JSContextRef and JSValueRef to an Objective-C DOMElement* might also be useful. But I don't think that really belongs on WebView. Maybe that should be a private class method of DOMElement.

I will look into this.

[Daniel Bates]

I split up the Mac and Windows changes into separate patches and used the existing bugs Bug #37929 and Bug #37930, respectively. I thought this would make it easier to review/discuss the changes and better track the landing of these patches.

[Daniel Bates]

Resolving this bug as fixed since we landed the Mac implementation in changeset 63445 <http://trac.webkit.org/changeset/63445> (Bug # 37929) and the Windows implementation in changeset 63710 <http://trac.webkit.org/changeset/63710> (Bug #37930).