Bug 87899

Created attachment 152429 [details] Patch

Comment on attachment 152429 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152429&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:196 > + if (node->renderer()) we should add a comment that Nodes with renderers should be made into AccessibilityRenderObjects because we prefer using the render tree as the data model > Source/WebCore/accessibility/AXObjectCache.cpp:444 > + if (node->renderer()) { is it possible a node got a renderer attached after we had made the object, meaning that we'd still want to try to clean up the m_nodeObjectMapping? > Source/WebCore/accessibility/AXObjectCache.h:188 > + HashMap<Node*, AXID> m_nodeObjectMapping; does it make sense to have three of these hash maps? or should it be a more generic type? > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:142 > + m_node = renderer->node(); should we call setNode() here so that AccessibilityRenderObject doesn't need to know about m_node > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3469 > + AccessibilityNodeObject::addChildren(); can we add these lines in a new method like "addCanvasChildren" and put it below with the other special cases?

Created attachment 153367 [details] Patch

(In reply to comment #2) > (From update of attachment 152429 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152429&action=review > > > Source/WebCore/accessibility/AXObjectCache.cpp:196 > > + if (node->renderer()) > > we should add a comment that Nodes with renderers should be made into AccessibilityRenderObjects because we prefer using the render tree as the data model Good idea, done. > > Source/WebCore/accessibility/AXObjectCache.cpp:444 > > + if (node->renderer()) { > > is it possible a node got a renderer attached after we had made the object, meaning that we'd still want to try to clean up the m_nodeObjectMapping? True, I think this could happen if an element is first inside of a canvas and then later reparented outside the canvas. In that case the node -> AccessibilityObject would leak. In the next change I'm going to replace all of the calls to AXObjectCache from everywhere else in WebCore so that they act on a Node instead of a Renderer. That will fix notifications and I think I can fix this corner case then. I specifically mentioned this test in https://bugs.webkit.org/show_bug.cgi?id=91794 and I'll write a test to confirm it's fixed then. > > Source/WebCore/accessibility/AXObjectCache.h:188 > > + HashMap<Node*, AXID> m_nodeObjectMapping; > > does it make sense to have three of these hash maps? or should it be a more generic type? Unfortunately I can't think of an easy way to use a more generic type without doing more unsafe type conversions. I think this is fine since every AccessibilityObject only appears in one hash map. > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:142 > > + m_node = renderer->node(); > > should we call setNode() here so that AccessibilityRenderObject doesn't need to know about m_node Good idea, made m_node private too. > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3469 > > + AccessibilityNodeObject::addChildren(); > > can we add these lines in a new method like "addCanvasChildren" and put it below with the other special cases? Sure, done. I had to modify canHaveChildren also.

Comment on attachment 153367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153367&action=review looks good. let's go for it > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:294 > + unsigned length = children.size(); i believe you want size_t for size() calls

Created attachment 153413 [details] Patch for landing

Comment on attachment 153413 [details] Patch for landing Clearing flags on attachment: 153413 Committed r123182: <http://trac.webkit.org/changeset/123182>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 91863

This patch was rolled out in <http://trac.webkit.org/changeset/123219>. The cause for the rollout were ASSERT crashes in debug builds. See: http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r123210%20(1119)/results.html http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r123210%20(1119)/accessibility/canvas-accessibilitynodeobject-crash-log.txt http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r123210%20(1119)/accessibility/canvas-crash-log.txt

Created attachment 153582 [details] Patch

This latest patch fixes the assertion failure. Sorry, I didn't check thoroughly enough after making the final changes. Now I remember why I originally had AccessibilityRenderObject::addChildren call AccessibilityNodeObject::addChildren at the top - because both methods assume m_haveChildren is false initially. I fixed it by having addCanvasChildren assert there are no children and then reset m_haveChildren to false before calling AccessibilityNodeObject::addChildren. Everything else is the same.

Comment on attachment 153582 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153582&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3460 > + if (node() && node()->hasTagName(canvasTag)) { we should re-write this so it uses early return > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3465 > + return; no need for return statement here

Created attachment 153732 [details] Patch

Comment on attachment 153732 [details] Patch Clearing flags on attachment: 153732 Committed r123428: <http://trac.webkit.org/changeset/123428>

All reviewed patches have been landed. Closing bug.