Bug 37930

Summary: [Win] Implement LayoutTestController::markerTextForListItem()
Product: WebKit Reporter: Jakub Wieczorek <jwieczorek>
Component: Tools / TestsAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, bweinstein, darin, dbates, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on:    
Bug Blocks: 42054    
Attachments:
Description Flags
Patch
aroben: review-
Patch aroben: review+

Description Jakub Wieczorek 2010-04-21 09:17:55 PDT
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
Comment 1 Daniel Bates 2010-07-14 21:25:19 PDT
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.
Comment 2 WebKit Review Bot 2010-07-14 22:54:50 PDT
Attachment 61605 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3576007
Comment 3 Brian Weinstein 2010-07-14 23:18:32 PDT
Make sure you touch WebKit.idl as part of your patch so the EWS bot rebuilds the idl files you need.
Comment 4 Adam Roben (:aroben) 2010-07-15 12:13:13 PDT
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.
Comment 5 Daniel Bates 2010-07-15 13:15:55 PDT
(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.
Comment 6 Daniel Bates 2010-07-15 13:18:56 PDT
(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));"
Comment 7 Daniel Bates 2010-07-15 16:01:00 PDT
Created attachment 61739 [details]
Patch

Updated patch based on Adam Roben's comments.
Comment 8 WebKit Review Bot 2010-07-15 23:37:21 PDT
Attachment 61739 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3352713
Comment 9 Adam Roben (:aroben) 2010-07-19 07:05:49 PDT
Comment on attachment 61739 [details]
Patch

r=me
Comment 10 Daniel Bates 2010-07-19 20:01:35 PDT
Committed r63710: <http://trac.webkit.org/changeset/63710>