[webkit-reviews] review denied: [Bug 20013] Windows AX huerusitics are poor : [Attachment 22444] This is the latest patch after I have done all the changes as per code review comments.

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Sankar Aditya Tanguturi
<sankaraditya at gmail.com>'s request for review:
Bug 20013: Windows AX huerusitics are poor
https://bugs.webkit.org/show_bug.cgi?id=20013

Attachment 22444: This is the latest patch after I have done all the changes as
per code review comments.
https://bugs.webkit.org/attachment.cgi?id=22444&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
In general this patch looks fine.  There are still a few style issues, and Jon
should look again for correctness.  I don't know if you have access to a Mac,
but I can run the layout tests on mine tomorrow if you can't.

Tab:
+	virtual bool isTextFormattingElement() const;

What does
+    const AccessibilityObject* accessibleParent() {  return m_accessibleParent
;}
+    void setAccessibleParent(const AccessibilityObject* );

accessibleParent mean? I assume you mean accessibilityParent?  "accessible"
reads "one that I can reach" instead of what I think you mean as "parent of
this dom/render object from the perspective of the accessibility interfaces"

Tab:
+		elementHashSet.add(pTag.toString());

No spaces before ;
+    virtual AccessibilityObject* firstAccessibilityChild() const ;
+    virtual AccessibilityObject* lastAccessibilityChild() const ;
+
+    virtual AccessibilityObject* previousAccessibilitySibling() const ;
+    virtual AccessibilityObject* nextAccessibilitySibling() const ;
+

Wrong (by webkit style) spacing:
+    const AccessibilityObject* accessibleParent() {  return m_accessibleParent
;}
+    void setAccessibleParent(const AccessibilityObject* );



This looks like it won't work dynamically.  m_indexInParent is never updated
when the parent's children are changed.  Maybe the point of this patch was only
to handle static content?

I could see this leading to a crash however, if m_indexInParent is set, and
then siblings are later removed, then next time nextAccessibilitySibling is
called on this object, m_indexInParent + 1 might be much larger than
siblingsVector.size(), and thus fail to trigger the:
+    if (m_indexInParent + 1 == siblingsVector.size())
+	 return 0;

causing a crash in 
+    return siblingsVector[m_indexInParent + 1].get();

Am I right?


This line:
return ariaText.isNull() || ariaText.isEmpty() ||
!ariaText.stripWhiteSpace().length()
Should just be:
return ariaText.isNull() || ariaText.impl()->containsOnlyWhiteSpace();

I'm surprised that containsOnlyWhiteSpace() is only available on StringImpl, it
really should be on String as well.  You could add:
bool containsOnlyWhiteSpace() { return isNull() || m_impl->
containsOnlyWhiteSpace(); } to PlatformString.h if you'd like and then just
call:
return text().isOnlyWhiteSpace();

Yeah, again, don't use "stripWhiteSpace()" as that's unecessary mallocs:
if (textUnderElement().stripWhiteSpace().length() != 0)  

Looks like you need an addition to String there as well.  Or you can manually
call the StringImpl method, but it would be better to just add
containsOnlyWhiteSpace() to String.

No need for the else after the return here:
+	     if (parentObjectUnignored()->ariaRoleAttribute() == MenuItemRole
+		 || parentObjectUnignored()->ariaRoleAttribute() ==
MenuButtonRole)
+		 return true;
+	     else
+		 return false;

This:
+	 if (children().size() == 0)
+	     return true;
+	 else
+	     return false;

should be:
return children().isEmpty();

Space between for and (, and no space between ) and ;
+    for(parentObj = m_object->parentObject() ; parentObj &&
parentObj->accessibilityIsIgnored(); parentObj = parentObj->parentObject()) { }



I wonder if this whole clause:
+    AccessibilityObject* parentObj = m_object->parentObject();
+    
+    for(parentObj = m_object->parentObject() ; parentObj &&
parentObj->accessibilityIsIgnored(); parentObj = parentObj->parentObject()) { }


shouldn't be turned into a function.  Like:

AccessibilityObject* parentObj = m_object->nonIgnoredParent();
if (parentObj)...

or similar.

Again, the change looks good (to me, not knowing much about AX), however you're
tripping up on WebKit style, and a few nits here and there.  This is kinda
expected... sadly it take a while to get used to hacking on WebKit.  But we're
here to help.  Happy to answer questions for you in #webkit.


Oh, and:
+2008-07-21  U-stanguturi\Sankar Tanguturi  <set EMAIL_ADDRESS environment
variable>

the prepare-changeLog script looks at your $USER and $EMAIL_ADDRESS environment
variables.  looks like yours aren't set to useful things.  You should just edit
the ChangeLogs manually to fix the name and email line.


Jon should look at this patch as well, but this will need one more round of
style-cleanup before we can land.