[Webkit-unassigned] [Bug 197095] Accessibility text search and selection API enhancements.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=197095

--- Comment #15 from Andres Gonzalez <andresg_22 at apple.com> ---
(In reply to chris fleizach from comment #14)
> Comment on attachment 367978 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367978&action=review
> 
> > Tools/DumpRenderTree/AccessibilityUIElement.cpp:291
> > +        startFrom = JSValueToStringCopy(context, arguments[1], exception);
> 
> we should be able to make these RefPtr<> so we don't have to release
> ourselves

JSStringRef is an opaque object that cannot be wrapped in a smart pointer like RefPtr since RefPtr doesn't have access to its constructor/destructor.

> 
> > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:188
> > +        JSObjectSetPropertyAtIndex(context, arrayObj, i, JSObjectMake(context, elements[i].getJSClass(), new T(elements[i])), 0);
> 
> are we going to leak new T() object here? I think so

I believe not because JSObjectMake is creating the object as follows:

    JSCallbackObject<JSDestructibleObject>* object = JSCallbackObject<JSDestructibleObject>::create(exec, exec->lexicalGlobalObject(), exec->lexicalGlobalObject()->callbackObjectStructure(), jsClass, data);

which should release the inner object (data) of class jsClass on deletion. The implementation in WTR is better since it passes in a RefPtr to the inner class instead of a raw pointer to the inner class. But it is probably beyond the scope of this change to convert all DRT to using RefPtr<AccessibilityXXXX>.

> 
> > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:215
> > +    return v;
> 
> can we WTFMove this back so that we don't have copy the array on return?

The compiler is doing that for us thanks to return value optimization (RVO) or copy elision.
> 
> > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:202
> >          JSObjectSetPropertyAtIndex(context, arrayObj, i, JSObjectMake(context, elements[i]->wrapperClass(), elements[i].get()), 0);
> 
> does JSObjectMake return an object that needs to be released?

I believe it should be released when the container arrayObj is released.

> 
> > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:229
> > +    return v;
> 
> this will copy the whole array again on return right? 
> can we WTFMove this back instead?

RVO takes care of it.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190423/348c495b/attachment.html>