Bug 217093 – Fix for multiple layout tests in accessibility isolated tree mode.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andres Gonzalez]

Fix for multiple layout tests in accessibility isolated tree mode.

[Andres Gonzalez]

Created attachment 410018 [details]
Patch

[chris fleizach]

Comment on attachment 410018 [details]
Patch

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

> 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?

[Andres Gonzalez]

(In reply to chris fleizach from comment #2)
> Comment on attachment 410018 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410018&action=review
> 
> > 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.

[chris fleizach]

Comment on attachment 410018 [details]
Patch

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

>>> 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

[Darin Adler]

Comment on attachment 410018 [details]
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.

[Andres Gonzalez]

Created attachment 410074 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #4)
> Comment on attachment 410018 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410018&action=review
> 
> >>> 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

Done.

[Andres Gonzalez]

(In reply to Darin Adler from comment #5)
> Comment on attachment 410018 [details]
> 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?

I will try the suggestion you gave me some time ago about using a template class to do this instead of having to isolatedCopy each individual string. I just put it in the back burner for more pressing issues, but it's time I try that to avoid these errors.

> 
> >>>> 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.

Done, removed the m_ prefix.
> 
> > 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.

Done.

[EWS]

Committed r267794: <https://trac.webkit.org/changeset/267794>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410074 [details].

[Radar WebKit Bug Importer]

<rdar://problem/69795437>