[Webkit-unassigned] [Bug 25898] [Gtk] object:text-changed events should be emitted for entries and password text

[bugzilla-daemon at webkit.org]

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


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #63314|review?                     |review-
               Flag|                            |




--- Comment #60 from Eric Seidel <eric at webkit.org>  2010-08-05 09:12:15 PST ---
(From update of attachment 63314)
WebCore/accessibility/AXObjectCache.cpp:463
 +      RefPtr<AccessibilityObject> object = getOrCreate(renderer);
getOrCreate needlessly returns a PassRefPtr, sadly.  Not your fault, but sad that it does here since it holds the reference itself.

WebCore/accessibility/AXObjectCache.h:132
 +      void nodeTextChangeNotification(RenderObject*, AXTextChange, unsigned offset, unsigned count);
Can this be a more specific type than RenderObject*?  We're trying to move to a more strongly typed RenderTree usage.  The AX code is currently the worse offender.

WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:62
 +      AccessibilityRenderObject* renderObject = static_cast<AccessibilityRenderObject*>(object);
Do we not have a toAccessibiltyRenderObject() function to do this cast more safely?

WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:63
 +      RenderObject* renderer = renderObject->renderer();
This is confusing to names the ax renderer "renderObject".  I would have called it axRenderObject or axRenderer or similar.

WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:66
 +      Node* node = renderer->node();
I'm surprised that AccessibilityRenderObject doesn't already have a node() helper method?

WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:68
 +      // Calculate global offset (in the whole text, not just in the current node)
Please put this in a helper function.

WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:80
 +      // Emit the right signal
Please put this in a helper function too.

WebCore/editing/AppendNodeCommand.cpp:58
 +              document()->axObjectCache()->nodeTextChangeNotification(m_node->renderer(), AXObjectCache::AXTextInserted, 0, len);
Can we pass a more specific type here?

WebCore/editing/AppendNodeCommand.cpp:54
 +      if (AXObjectCache::accessibilityEnabled()) {
It seems this block should be a helper function "sendAXTextChangedIngnoringLineBreaks"
Obviously you'd pass the type of notification.
If you wanted to share with the non-ignoring linebreaks case, you'd use an enum ShouldIgnoreLineBreaks { IgnoreLineBreaks, IncludingLineBreaks } and make "IncludingLineBreaks" the default value.

WebCore/editing/AppendNodeCommand.cpp:68
 +      if (AXObjectCache::accessibilityEnabled()) {
Copy/paste code is work of the devil. :p

WebCore/editing/DeleteSelectionCommand.cpp: 
 +          } else if (m_downstreamEnd.deprecatedEditingOffset() - startOffset > 0) {
Was this an ordered else-if for a reason?

WebCore/editing/DeleteSelectionCommand.cpp:459
 +              // just delete
This comment isn't helpful.

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