[webkit-reviews] review requested: [Bug 25898] [Gtk] object:text-changed events should be emitted for entries and password text : [Attachment 61861] 3. Notify accessibility when something changes in a text 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 61861: 3. Notify accessibility when something changes in a text node
https://bugs.webkit.org/attachment.cgi?id=61861&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
Now attaching a new patch to replace this one as well...

(In reply to comment #24)
> (From update of attachment 61783 [details])
> Adding r=me. I think the changes you need to make are straightforward for
this one. I think calling these methods can be mostly reduce to two lines for
each instance
> if (AXObjectCache::)
>     document()->axCache()->nodeTextChange()

As I said in my previous comment, I'm not a committer so if you're ok with this
new patch I'd appreciate that, if possible, you set the commit-queue+ flag
alongside with the review+ (supposing it's correct!).

> WebCore/editing/AppendNodeCommand.cpp:60
>  +									0,
len);
> indentation

Fixed, alongside with the rest of the indentation issues.

> WebCore/editing/AppendNodeCommand.cpp:61
>  +	      }
> do you need the line breaking here in nodeTextChangeNotification?

I'm very used not to trespass the 80 line whenever possible so that's why I
broke it. Anyway, I was checking other files and it seems in WK code it's not
that strange to find long lines every now and then, so I've just changed this
to one line as well.

> WebCore/editing/AppendNodeCommand.cpp:78
>  +	      }
> I think the len > 0, node->nodeValue() != "\n" should go into the logic
inside AXObjectCache. Different platforms may want to process those, so i think
it should be left up to AX code to handle those cases.
> it will also make these code snippets much simpler

Absolutely. The length check should be in the platform specific code (and it
actually is already there!), and doing it outside it's just plain useless and
unneeded.

However, I need to leave the "\n" check in AppendNodeCommand since that's there
to avoid to notify a11y twice, as in that case it happens that the
InserNodeBeforeCommand will be always executed whenever a linebreak is done
(while AppendNodecommand is only run when making a linebreak at the end of the
text), so there's no need to do it also here (and it would even be a mistake
IMHO)

> WebCore/editing/DeleteFromTextNodeCommand.cpp:63
>  +  
> for one line if statements, no braces needed

Done.

> WebCore/editing/DeleteFromTextNodeCommand.cpp:82
>  +  }
> no braces needed

Done

> WebCore/editing/InsertIntoTextNodeCommand.cpp:57
>  +	  }
> no braces needed

Done

> WebCore/editing/InsertIntoTextNodeCommand.cpp:71
>  +  
> no braces needed

Done

> 
> WebCore/editing/InsertNodeBeforeCommand.cpp:62
>  +	      }
> same comments, let the AX code handle whether it should process this based on
length or not
> 
> WebCore/editing/InsertNodeBeforeCommand.cpp:78
>  +	      }
> ditto as above
> 

Done

> WebCore/editing/InsertNodeBeforeCommand.cpp:84
>  +  1.7.0.4
> extra line removed, should not be

As I said in the previous command, I don't understand this thing :-)