Summary: | AX: Implement isolated tree support for math objects | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||||
Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aboxhall, andresg_22, apinheiro, commit-queue, darin, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, webkit-bug-importer, zalan | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
chris fleizach
2019-11-10 00:09:12 PST
Created attachment 383238 [details]
patch
Comment on attachment 383238 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=383238&action=review > Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:-37 > -AXIsolatedObject::AXIsolatedObject(const AXCoreObject& object) Removing the const here because some of the math objects access children() which is non-const Created attachment 383239 [details]
patch
setProperty(AXPropertyName::HelpText, object.helpTextAttributeValue().isolatedCopy()); + AG: don't think the code style rules state this, but I would prefer (kindly request) blank lines to have zero spaces. Otherwise VO reads "space, space, ..." very annoying. There is an option in Xcode to automatically get rid off trailing spaces, and that does the right thing. - static Ref<AXIsolatedObject> create(const AXCoreObject&); + static Ref<AXIsolatedObject> create(AXCoreObject&); AG: Fine for now. But we should have the create function and the constructor to take an AccessibilityObject, constructor marked as explicit. And a separate copy constructor that takes an AXIsolatedObject. + AXIsolatedObject(AXCoreObject&); + void initializeAttributeData(AXCoreObject&); AG: As in create, we should have an explicit constructor that takes an AccessibilityObject, and a separate copy constructor. initializeAttributeData should take an AccessibilityObject. (In reply to chris fleizach from comment #5) > setProperty(AXPropertyName::HelpText, > object.helpTextAttributeValue().isolatedCopy()); > + > > AG: don't think the code style rules state this, but I would prefer (kindly > request) blank lines to have zero spaces. Otherwise VO reads "space, space, > ..." > very annoying. There is an option in Xcode to automatically get rid off > trailing spaces, and that does the right thing. > > > - static Ref<AXIsolatedObject> create(const AXCoreObject&); > + static Ref<AXIsolatedObject> create(AXCoreObject&); > > AG: Fine for now. But we should have the create function and the constructor > to > take an AccessibilityObject, constructor marked as explicit. And a separate > copy constructor that takes an AXIsolatedObject. > > > + AXIsolatedObject(AXCoreObject&); > + void initializeAttributeData(AXCoreObject&); > > AG: As in create, we should have an explicit constructor that takes an > AccessibilityObject, and a separate copy constructor. initializeAttributeData > should take an AccessibilityObject. Talked with Andres and we discussed at this time we have to deal with the fact that children() is non-const so we need to handle this here because we're calling children() now for the math things Created attachment 383513 [details]
patch
Comment on attachment 383513 [details] patch Clearing flags on attachment: 383513 Committed r252457: <https://trac.webkit.org/changeset/252457> All reviewed patches have been landed. Closing bug. Comment on attachment 383513 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=383513&action=review > Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:201 > + AXID nodeID = WTF::switchOn(value, > + [&] (Optional<AXID> typedValue) { > + if (!typedValue) > + return InvalidAXID; > + return typedValue.value(); > + }, > + [] (auto&) { return InvalidAXID; } > + ); I don’t understand the reason behind the [&] capture; that mistake seems to be repeated over and over again in the other similar functions. Also maybe we could write this in a way that's a little more readable using valueOr(InvalidAXID). I don’t understand why we have both InvalidAXID and Optional<AXID>; two ways of doing the same thing. We should choose one idiom or the other rather than mixing the two. This here is extra code to translate between them. Indentation mistake here. This return statement should be indented. (In reply to Darin Adler from comment #10) > Comment on attachment 383513 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383513&action=review > > > Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:201 > > + AXID nodeID = WTF::switchOn(value, > > + [&] (Optional<AXID> typedValue) { > > + if (!typedValue) > > + return InvalidAXID; > > + return typedValue.value(); > > + }, > > + [] (auto&) { return InvalidAXID; } > > + ); > > I don’t understand the reason behind the [&] capture; that mistake seems to > be repeated over and over again in the other similar functions. > > Also maybe we could write this in a way that's a little more readable using > valueOr(InvalidAXID). > > I don’t understand why we have both InvalidAXID and Optional<AXID>; two ways > of doing the same thing. We should choose one idiom or the other rather than > mixing the two. This here is extra code to translate between them. > > Indentation mistake here. This return statement should be indented. Thanks for your feedback. Will look into make those changes > Thanks for your feedback. Will look into make those changes https://bugs.webkit.org/show_bug.cgi?id=204233 |