Bug 42054

Summary: Implement LayoutTestController::markerTextForListItem() for Mac and Windows DRT
Product: WebKit Reporter: Daniel Bates <dbates>
Component: Tools / TestsAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, bweinstein, darin, jwieczorek
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 37929, 37930    
Bug Blocks:    
Attachments:
Description Flags
Patch
aroben: review-
Patch none

Description Daniel Bates 2010-07-11 23:41:38 PDT
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.
Comment 1 Daniel Bates 2010-07-11 23:49:49 PDT
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.
Comment 2 Jakub Wieczorek 2010-07-12 01:37:23 PDT
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 3 Adam Roben (:aroben) 2010-07-12 06:27:03 PDT
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 4 Darin Adler 2010-07-12 14:08:38 PDT
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.
Comment 5 Daniel Bates 2010-07-12 19:38:01 PDT
(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.
Comment 6 Daniel Bates 2010-07-12 19:40:15 PDT
Created attachment 61314 [details]
Patch

Updated patch based on Adam Roben's comments.
Comment 7 Daniel Bates 2010-07-12 19:45:37 PDT
(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.
Comment 8 Daniel Bates 2010-07-14 21:32:23 PDT
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.
Comment 9 Daniel Bates 2010-07-23 21:59:27 PDT
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).