[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