574003 - coalesce text events on nodes removal

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

This should make fast an example https://bugzilla.mozilla.org/attachment.cgi?id=450065 from bug 570500

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Marco Zehe (:MarcoZ)]

Comment on attachment 453391 [details] [diff] [review]
patch

>+        if (aLastToFirst) {
>+          while (this.DOMNode.firstChild)
>+            this.DOMNode.removeChild(this.DOMNode.lastChild);

Is this intentional that you check for firstChild but remove lastChild? Because below you continue:

>+        } else {
>+          while (this.DOMNode.firstChild)
>+            this.DOMNode.removeChild(this.DOMNode.firstChild);

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> (From update of attachment 453391 [details] [diff] [review])
> >+        if (aLastToFirst) {
> >+          while (this.DOMNode.firstChild)
> >+            this.DOMNode.removeChild(this.DOMNode.lastChild);
> 
> Is this intentional that you check for firstChild but remove lastChild? 

I check if there are children.

Comment 4

[Marco Zehe (:MarcoZ)]

(In reply to comment #3)
> I check if there are children.

OK, so this is not a copy/paste error but intentional coding. Thanks!

Comment 5

[Marco Zehe (:MarcoZ)]

Tests are OK, waiting for try-server build to give it a test run before I r+.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> (In reply to comment #3)
> > I check if there are children.
> 
> OK, so this is not a copy/paste error but intentional coding. Thanks!

I think yes. Actually it doesn't matter if you check firstChild or lastChild but performance perhaps.

(In reply to comment #5)
> Tests are OK, waiting for try-server build to give it a test run before I r+.

I started it but it's built upon bug 573955 which try server has failures on linux. I need to investigate this first of all. Hopefully tomorrow.

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 454099 [details] [diff] [review]
patch2

>+AccHideEvent::
>+  AccHideEvent(nsAccessible* aTarget, nsINode* aTargetNode,
>+               PRBool aIsAsynch, EIsFromUserInput aIsFromUserInput) :
>+  nsAccEvent(nsIAccessibleEvent::EVENT_HIDE, aTarget, aIsAsynch,
>+             aIsFromUserInput, eCoalesceFromSameSubtree)
>+{
>+  mNode = aTargetNode;

Please initialize via ': mNode(aTargetNode)' syntax to avoid coverity (?) warnings -- couldn't easily find old bug due to history overwrite.

>+  PRUint32 GetLength() const { return mModifiedText.Length(); }

Nice. Might as compute it lazily right :)

>+  friend class nsAccEventQueue;

Why?

Comment 9

[alexander :surkov (:asurkov)]

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-59c3f1989ee1/

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #8)

> >+  friend class nsAccEventQueue;
> 
> Why?

to get an access to private members.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> >+  nsAccEvent(nsIAccessibleEvent::EVENT_HIDE, aTarget, aIsAsynch,
> >+             aIsFromUserInput, eCoalesceFromSameSubtree)
> >+{
> >+  mNode = aTargetNode;
> 
> Please initialize via ': mNode(aTargetNode)' syntax to avoid coverity (?)
> warnings -- couldn't easily find old bug due to history overwrite.

mNode belongs to nsAccEvent, I can't initialize it here.

Comment 12

[Marco Zehe (:MarcoZ)]

Comment on attachment 454099 [details] [diff] [review]
patch2

r=me with:

>+     * Remove children from text container from first to last child or visa
>+     * versa.

Nit: "vice".

>+      // Remove all childewn.

Nit: "children".

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #10)
> (In reply to comment #8)
> 
> > >+  friend class nsAccEventQueue;
> > 
> > Why?
> 
> to get an access to private members.

Right - but I didn't see where. OK.

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 454099 [details] [diff] [review]
patch2

>@@ -209,18 +214,25 @@ nsAccEventQueue::WillRefresh(mozilla::Ti

>+    if (accEvent->mEventRule != nsAccEvent::eDoNotEmit) {
>       mDocument->ProcessPendingEvent(accEvent);
>+
>+      AccHideEvent* hideEvent = downcast_accEvent(accEvent);
>+      if (hideEvent) {
>+        if (hideEvent->mTextChangeEvent)
>+          mDocument->ProcessPendingEvent(hideEvent->mTextChangeEvent);

Why you process the text change event if the rule for the owner is eDoNotEmit?

Comment 15

[alexander :surkov (:asurkov)]

I guess you meant when it's _not_ eDoNotEmit. Because if hide was coalesced then no other event doesn't make sense from this subtree, since it means its parent was hidden.

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #15)
> I guess you meant when it's _not_ eDoNotEmit.

It was late and I missed the "!" -- sorry for noise. I'm glad it was just tiredness on my part.

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 454099 [details] [diff] [review]
patch2

> nsAccEventQueue::Push(nsAccEvent *aEvent)

>+  // Associate text change with hide event if it wasn't during coalescence.

I think you need to say if it wasn't _what_? Removed or associated?

>+  AccHideEvent* hideEvent = downcast_accEvent(aEvent);
>+  if (hideEvent && !hideEvent->mTextChangeEvent)
>+    CreateTextChangeEventFor(hideEvent);

I'm curious how often mTextChangeEvent is false here. Perhaps add a comment?

>+        // XXX: deal with show events separately because they can't be
>+        // coalesced by accessible tree as hide events since target accessibles

In this context 'the same as' or 'like' would be clearer I think.


>+  } else if (aThisEvent->mPrevSibling == aTailEvent->mAccessible) {
>+    PRUint32 oldLen = textEvent->GetLength();
>+    aTailEvent->mAccessible->AppendTextTo(textEvent->mModifiedText,
>+                                          0, PR_UINT32_MAX);
>+    textEvent->mStart -= textEvent->GetLength() - oldLen;
>+  }

I'm not sure what this is for. Isn't textEvent->GetLength() - oldLen always 0?

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 454099 [details] [diff] [review]
patch2

>+  /**
>+   * Create text change event caused by hide event. When a node is hidden or
>+   * removed, the text in an ancestor hyper text will lose characters. Create
>+   * text change event unless the node being removed or frame being destroyed.

Nit: 'node is being' 'frame is being'

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #17)
> (From update of attachment 454099 [details] [diff] [review])

> >+  } else if (aThisEvent->mPrevSibling == aTailEvent->mAccessible) {
> >+    PRUint32 oldLen = textEvent->GetLength();
> >+    aTailEvent->mAccessible->AppendTextTo(textEvent->mModifiedText,
> >+                                          0, PR_UINT32_MAX);
> >+    textEvent->mStart -= textEvent->GetLength() - oldLen;
> >+  }
> 
> I'm not sure what this is for. Isn't textEvent->GetLength() - oldLen always 0?

Oh I see it now. OK.

Comment 20

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 454099 [details] [diff] [review]
patch2

r=me

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> (From update of attachment 454099 [details] [diff] [review])
> > nsAccEventQueue::Push(nsAccEvent *aEvent)
> 
> >+  // Associate text change with hide event if it wasn't during coalescence.
> 
> I think you need to say if it wasn't _what_? Removed or associated?
> 
> >+  AccHideEvent* hideEvent = downcast_accEvent(aEvent);
> >+  if (hideEvent && !hideEvent->mTextChangeEvent)
> >+    CreateTextChangeEventFor(hideEvent);
> 
> I'm curious how often mTextChangeEvent is false here. Perhaps add a comment?

For these two I've added comment "// Associate text change with hide event if it wasn't stolen from hiding siblings during coalescence." implicitly describing when it's null.

Comment 22

[alexander :surkov (:asurkov)]

landed on 1.9.3 (2.0) - http://hg.mozilla.org/mozilla-central/rev/d20dcbee6197

Comment 23

[Ginn Chen]

Attachment: fix bustage with sun studio 12

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 455650 [details] [diff] [review]
fix bustage with sun studio 12

thanks, Ginn.

Comment 25

[Ginn Chen]

http://hg.mozilla.org/mozilla-central/rev/6c1fcdfdc2c8

bustage fix committed.