Summary: | Avoid confusion between AccessibilityObject::isTable() / isAccessibilityTable() | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||
Component: | Accessibility | Assignee: | Chris Dumez <cdumez> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, cfleizach, commit-queue, darin, dmazzoni, jcraig, jdiggs, mario, samuel_white, webkit-bug-importer | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2014-10-20 17:06:36 PDT
Created attachment 240165 [details]
Patch
Attachment 240165 [details] did not pass style-queue:
ERROR: Source/WebCore/accessibility/AccessibilityTable.cpp:585: Multi line control clauses should use braces. [whitespace/braces] [4]
Total errors found: 1 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 240168 [details]
Patch
Attachment 240168 [details] did not pass style-queue:
ERROR: Source/WebCore/accessibility/AccessibilityTable.cpp:585: Multi line control clauses should use braces. [whitespace/braces] [4]
Total errors found: 1 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 240173 [details]
Patch
Attachment 240173 [details] did not pass style-queue:
ERROR: Source/WebCore/accessibility/AccessibilityTable.cpp:585: Multi line control clauses should use braces. [whitespace/braces] [4]
Total errors found: 1 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 240175 [details]
Patch
Attachment 240175 [details] did not pass style-queue:
ERROR: Source/WebCore/accessibility/AccessibilityTable.cpp:585: Multi line control clauses should use braces. [whitespace/braces] [4]
Total errors found: 1 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 240177 [details]
Patch
Attachment 240177 [details] did not pass style-queue:
ERROR: Source/WebCore/accessibility/AccessibilityTable.cpp:585: Multi line control clauses should use braces. [whitespace/braces] [4]
Total errors found: 1 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 240177 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240177&action=review While I do think we are getting more clarity here, the really verbose repeated is && downcast dance is irritating. I wish there was a cleaner way to write all of those. > Source/WebCore/accessibility/AccessibilityARIAGridCell.cpp:66 > + if (!is<AccessibilityTable>(parent) || !downcast<AccessibilityTable>(*parent).isExposableThroughAccessibility()) > return nullptr; Maybe this would read better as !(a && b) rather than !a || !b? > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1631 > + if (is<AccessibilityList>(*obj) > + || (is<AccessibilityTable>(*obj) && downcast<AccessibilityTable>(*obj).isExposableThroughAccessibility()) > + || obj->isTree() > + || obj->isCanvas()) > return false; How about 3 or 4 separate if statements? I don’t see why we need to use one with || here. > Source/WebCore/accessibility/AccessibilityTable.cpp:54 > + , m_isExposableThroughAccessibility(true) Seems a little strange to initialize this both here and in the init function. It seems like we’d have a problem if we looked at this before calling init. > Source/WebCore/accessibility/AccessibilityTable.h:63 > + virtual int tableLevel() const override final; Glad you marked this final. I’d like to see more of these public virtual override functions made private, protected, or final. > Source/WebCore/accessibility/AccessibilityTable.h:96 > + virtual bool computeIsTableExposableThroughAccessibility() const; Not sure why this is protected instead of private. Do derived classes call through to the base class? > Source/WebCore/accessibility/AccessibilityTableRow.cpp:79 > - if (!table || !table->isAccessibilityTable()) > + if (!is<AccessibilityTable>(table) || !downcast<AccessibilityTable>(*table).isExposableThroughAccessibility()) > return false; > > return true; This should be: return is<AccessibilityTable>(table) && downcast<AccessibilityTable>(*table).isExposableThroughAccessibility(); > Source/WebCore/accessibility/AccessibilityTableRow.cpp:110 > if (is<AccessibilityTable>(*parent)) > - return downcast<AccessibilityTable>(*parent).isAccessibilityTable() ? downcast<AccessibilityTable>(parent) : nullptr; > + return downcast<AccessibilityTable>(*parent).isExposableThroughAccessibility() ? downcast<AccessibilityTable>(parent) : nullptr; Seems like this goes out of its way to use ? : when it could just fall through to return nullptr. Created attachment 240293 [details]
Patch
Attachment 240293 [details] did not pass style-queue:
ERROR: Source/WebCore/accessibility/AccessibilityTable.cpp:585: Multi line control clauses should use braces. [whitespace/braces] [4]
Total errors found: 1 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 240293 [details] Patch Clearing flags on attachment: 240293 Committed r175068: <http://trac.webkit.org/changeset/175068> All reviewed patches have been landed. Closing bug. |