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.
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.
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
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.
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.
(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.
Created attachment 61314 [details] Patch Updated patch based on Adam Roben's comments.
(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.
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.
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).