[webkit-reviews] review requested: [Bug 25898] [Gtk] object:text-changed events should be emitted for entries and password text : [Attachment 61859] 2. New functions in AXObjectCache to call when text changes in a node

[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 61859: 2. New functions in AXObjectCache to call when text changes
in a node
https://bugs.webkit.org/attachment.cgi?id=61859&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
First of all, thanks for reviewing and sorry for marking the patch you set as
review+ as obsolete, but I had to do it since I couldn't commit the changes by
myself anyway (I'm not committer). So I'm now just replacing the old version of
the patch with a new one so you could review it and, if it's correct, both mark
it as review+ and commit-queue+ :-)

(In reply to comment #23)
> (From update of attachment 61782 [details])
> This patch is just about there. I'm putting review- just because i'd like to
see it one more time before it goes through. thanks!
> 
> WebCore/accessibility/AXObjectCache.cpp:473
>  +  }
> it looks like you don't really need this method, since its only used by the
RenderObject version.

Sure, you're right. I did it that way following the lead of the
AXObject::postNotification() method, thinking that perhaps that was the way to
go because of some yet-unknown reason to me... but I normally think needed
methods should get in, and if you think the same that's a good reason for me to
remove it.

So... done.

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:68
>  +  
> This looks like a roundabout way to get object->parentObject()->wrapper(). 

True. I'm still getting used to this forest of trees in WK and sometimes I get
a bit lost... as in this case :-). Thanks for pointing it out, I've tried your
suggestion and works also as a charm (but cleaner).

Done.

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:73
>  +  
> identation is wrong
> 
> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:88
>  +  
> identation is wrong

Fixed all indentation issues in all files.

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:94
>  +  }
> looks like you should use a switch statement here which sets a char* or
String to "text-change*". then you'd only have to call g_signal* one time.

Done (I've used a GOwnPtr to avoid using g_free(), hope that's the right thing
to do)

> WebCore/accessibility/win/AXObjectCacheWin.cpp:113
>  +  1.7.0.4
> extra line removal not needed

Not sure I understand this one... Isn't it just the end of the patch file?

> WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:65
>  +  
> identation

Fixed.

Thanks for the review. As I said at the beginning here you have attached a new
patch addressing these issues