[Webkit-unassigned] [Bug 20013] Windows AX huerusitics are poor

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=20013





------- Comment #2 from eric at webkit.org  2008-07-22 10:04 PDT -------
(From update of attachment 22417)
This looks pretty good.  A few comments:

1.  We use 4 spaces, not tabs in webkit.

+       virtual AccessibilityObject* accFirstChild() const ;
+    virtual AccessibilityObject* accLastChild() const ;
+
+    virtual AccessibilityObject* previousAccessibilitySibling() const ;
+    virtual AccessibilityObject* nextAccessibilitySibling() const ;

These aren't consistent.

Probably accFirstChild should change to:

firstAccessibilityChild() instead.

There seem to be some strange uses of "const" in this class... 

+WTF::HashSet<String> createTextFormattingElementNamesHash() 

No need to use WTF:: there, I believe using namespace WTF should already have
been done at some point in a header for you.

Probably better to have createTextFormattingElementNamesHash return a
HashSet<String>*, since HashSet does a full deep copy in its copy constructor,
so you'd end up with a lot of copying going on during return.

+    HashSet<String> elementHashSet ;

extra space.

Extra space:
+    return elementHashSet;
+
+}

It seems m_accessibilityID is misnamed (not your fault).  It should probably be
"m_indexInParent".

No need for an "else" after return in:
+        return
textFormattingElementNames.contains(m_renderer->element()->nodeName().lower());
+    else
+        return false;

The lower() here is not necessary:
+        return
textFormattingElementNames.contains(m_renderer->element()->nodeName().lower());


if you use a case-insensitive hash:

HashSet<String, CaseFoldingHash>

It also appears that the more common paradigm is not to use a HashSet* as I
just said above, but rather to just init the hash in the function.  See:
http://trac.webkit.org/browser/trunk/WebCore/xml/XMLHttpRequest.cpp#L90

+bool AccessibilityRenderObject::isLinkOrListItemElement() const
should probably use a Hash in the same pattern as shown in the link above. 
Darin Adler once figured out what the break-even point was... I think in terms
of code complexity, anything past about 4 items looks cleaner in a Hash.

Again, extra space, and no need for "else" after return:
+                 m_renderer->element()->hasTagName(dtTag))
+        return true;
+    else
+        return false;
+
+}

Extra spaces at end of line:
     if (!m_renderer)
-        return String();
+        return String();    

We have an append() function which takes a Vector and a String, so this:
+            String elementTitle = childrenVector[i]->title();
+           
elementText.append(elementTitle.characters(),elementTitle.length());

should just be:
append(elementText, elementTitle);

Again, no else needed after return:
+            if (!alt.isNull())
+                return alt.string();
+            else
+                return String();
+        }

No extra space needed before ;
+        return ariaText.isNull() || ariaText.isEmpty() ||
!ariaText.stripWhiteSpace().length() ;

I believe the WebKit style guides say the outer if here should have { }:
+    if (isLinkOrListItemElement())         
+        if (children().size() == 0)
+            return true;
+        else
+            return false;
+

No spaces needed before ):
+    if (m_renderer->element() && m_renderer->element()->hasTagName(htmlTag) )
+        return true;
+
+    if (m_renderer->element() && m_renderer->element()->hasTagName(bodyTag) )
+        return true;

Add  { } remove use of else after return:
+    if (m_renderer->isBlockFlow() )  
+      if (m_renderer->isAnonymousBlock())   
+          return true;
+      else  
+          return false;

+    if (node && (node->hasTagName(tdTag) ||
+                 node->hasTagName(thTag)
+                                 ))                                 
+        return CellRole;

)) should be on the same line as the rest of it.  It can all be on one line I
bet.  We don't have a strong 80-column limit in WebKit, although most folks try
to wrap their text to make it easy to read.

Space after if before (, but not before ):
+    if(node && node->hasTagName(liTag) )
+        return ListItemRole;
+
+    if(node && node->hasTagName(hrTag) )
+        return SeparatorRole;

Besides the strange formatting, this looks fine to me.  Jon Honeycutt should
really review it for correctness.  I'll ask him to do so.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.