[webkit-reviews] review denied: [Bug 25898] [Gtk] object:text-changed events should be emitted for entries and password text : [Attachment 63314] Single patch for fixing this bug

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Mario Sanchez Prada
<msanchez at igalia.com>'s request for review:
Bug 25898: [Gtk] object:text-changed events should be emitted for entries and
password text
https://bugs.webkit.org/show_bug.cgi?id=25898

Attachment 63314: Single patch for fixing this bug
https://bugs.webkit.org/attachment.cgi?id=63314&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.