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

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  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 63712: Single patch for fixing this bug
https://bugs.webkit.org/attachment.cgi?id=63712&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
(In reply to comment #60)
> (From update of attachment 63314 [details])
> 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.

Actually, getOrCreate() is handling and returning a RefPtr, not a PassRefPtr,
so I guess the only problem in the code is actually my fault and a matter of
changing the following line in AXObjectCache.cpp:

  RefPtr<AccessibilityObject> object = getOrCreate(renderer);

...into something like this:

  AccessibilityObject* object = getOrCreate(renderer);

...which is what it's done all over the place and seems to me like the right
way to go (done in current new attachment)
 

> 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.

As far as I can see, it can't, because using an specific subclass like
RenderText would be not correct, as other subclasses of RenderObject (such as
RenderTextControl, which is NOT a subclass of RenderText) would also be valid
for this method. So, I guess there's no best option here than accepting a
RenderObject and later on checking whether is actually the right kind of object
depending on the context (for the gtk port the rule of thumb would be that the
associated platform-dependant a11y object (the wrapper) should implement the
AtkText interface.

Also, looking for something else in AXObjectCache.h that could help me with
this I think I found a similar situation with the
AXObjectCache::contentChanged() method:

    // Called by a node when text or a text equivalent (e.g. alt) attribute is
changed.
    void contentChanged(RenderObject*);

Looks like it could use a RenderObject because of a similar reason, not 100%
sure though.

Anyway, feel free to point out that I'm completely wrong if you think so. I'll
appreciate that.

 
> 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?

Added to AccessibilityRenderObject.h, following the lead of other similar ones,
such as toRenderText()
 

> 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.

Taken into account (although no longer a problem) in the new patch.


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

Fixed.

 
> 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.

Done (both).


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

I think we can't in this case. See rationale above.


> 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.

Done that way

> 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.

I thought about that, but in this case I think it's more than enough (and even
cleaner) to implement it the other way.


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

Agreed, copy/paste along with "premature optimizations" are the "root of all
evil".
Fixed.


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

As explained in comments #1 and #62 (and maybe somewhere else as well), the
idea is to make sure the deleteFromTextNode() function gets executed even when
removing all the text of a node, so that "else if" must become a normal "if"
and moved above the "if (removing-all-text) { removeNode() }" branch, to make
sure it gets executed before removing the node.


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

I missed this one, thanks for the catch.


As you can see I'm now attaching a new patch addressing all the issues
commented above but the thing about being more specific with the RenderObject
parameter, as explained above.

Hope this new patch gets closer to the good solution, and thanks a lot for your
thoroughful review, Eric.