[webkit-reviews] review granted: [Bug 217093] Fix for multiple layout tests in accessibility isolated tree mode. : [Attachment 410018] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Sep 29 13:52:04 PDT 2020


Darin Adler <darin at apple.com> has granted Andres Gonzalez
<andresg_22 at apple.com>'s request for review:
Bug 217093: Fix for multiple layout tests in accessibility isolated tree mode.
https://bugs.webkit.org/show_bug.cgi?id=217093

Attachment 410018: Patch

https://bugs.webkit.org/attachment.cgi?id=410018&action=review




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 410018
  --> https://bugs.webkit.org/attachment.cgi?id=410018
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410018&action=review

> Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp:200
> -    setProperty(AXPropertyName::ComputedRoleString,
object.computedRoleString());
> +    setProperty(AXPropertyName::ComputedRoleString,
object.computedRoleString().isolatedCopy());

We keep getting this wrong. What can we do to make this easier to get right?

>>>> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:212
>>>> +	      nodeChange.m_wrapper = axObject.wrapper();
>>> 
>>> I feel like accessing the internal m_wrapper variable directly is a bit of
bad form. can we add a setter method to set this value?
>> 
>> Actually I was going the opposite direction based on a comment by Darin
Adler in another review that we don't use m_ prefix for structure member
variables. I don't think it is common either to have setters/getters for
structure variables which are public. Unless you feel strongly about this, I
think we don't need to make this structure any less transparent, after all it
is just an object and its wrapper, it could even be a pair.
> 
> ok yea, we should just rename m_wrapper to wrapper

In fact, this brief discussion/misunderstanding demonstrates the value of
leaving "m_" out of the names of public data members.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:345
> +	   NodeChange(AXIsolatedObject&);
>	   NodeChange(AXIsolatedObject&, AccessibilityObjectWrapper*);
>	   NodeChange(const NodeChange&);

Do we really need these? I’d rather we use aggregate style initialization
rather than constructors, and let the copy constructor get generated for us
(and a move constructor too). We should give that a try.


More information about the webkit-reviews mailing list