313351 - make nsIDocument::ContentStatesChanged take only one content node

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsIDocument::ContentStatesChanged takes two content nodes as an argument.  The
original rationale for this was probably primitive coalescing (at most
2-at-a-time), on the assumption (now false) that most event state changes
involved one node going into a state and one going out.  We have real style
change coalescing now, and we should just remove this extra optimization to
simplify the code.  (And remove the extra "s" from the function name.)

(I thought I'd filed a bug on this a while ago, but I can't find it.)

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Once we do this we should consider moving the notification from nsIDocumentObserver to nsIMutationObserver.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Just Do IT

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 511072 [details] [diff] [review]
Just Do IT

> PRBool
> nsEventStateManager::SetContentState(nsIContent *aContent, nsEventStates aState)
...
>       if (notifyContent[0]) {
>-        doc1->ContentStatesChanged(notifyContent[0], notifyContent[1],
>-                                   simpleStates);
>+        doc1->ContentStateChanged(notifyContent[0], simpleStates);
>+        if (notifyContent[1]) {
>+          doc1->ContentStateChanged(notifyContent[1], simpleStates);
>+        }
>         if (notifyContent[2]) {
>           // more that two notifications are needed (should be rare)
>-          // XXX a further optimization here would be to group the
>-          // notification pairs together by parent/child, only needed if
>-          // more than two content changed (ie: if [0] and [2] are
>-          // parent/child, then notify (0,2) (1,3))
>-          doc1->ContentStatesChanged(notifyContent[2], notifyContent[3],
>-                                     simpleStates);
>+          doc1->ContentStateChanged(notifyContent[2], simpleStates);
>+          if (notifyContent[3]) {
>+            doc1->ContentStateChanged(notifyContent[3], simpleStates);
>+          }
>           if (notifyContent[4]) {
>             // more that four notifications are needed (should be rare)
>-            doc1->ContentStatesChanged(notifyContent[4], nsnull,
>-                                       simpleStates);
>+            doc1->ContentStateChanged(notifyContent[4], simpleStates);
>           }
>         }
>       }

Given that the notifyContent array is compressed (see the code just above), could you at least make this:

for (PRInt32 i = 0; i < maxNotify; ++i) {
  if (!notifyContent[i]) {
    break;
  }
  doc1->ContentStatesChanged(notifyContent[i], simpleStates);
}

I think the doc2 case is broken enough that you should replace it with an assertion that notifyContent[i]->GetOwnerDoc() == mPresContext->GetDocument() (or |doc|, see next sentence) inside the above loop, and you should rename doc1 to doc and initialize it using mPresContext->GetDocument().  (Send that to try before landing, though.)

That said, I'd appreciate another patch on top of this one to remove *much* more code from this function.

The rest of the patch looks great.  r=dbaron

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Though, actually, maybe the doc2 case actually works right now because we only hit it in cases where a single bit is being passed to SetContentState, and therefore we have at most two nodes in the array.  I don't see any callers that pass more than one bit.

So I actually take back the bit about replacing the doc2 case.  Could you just make the doc2 case a loop too, and make it loop from 1 to maxContent (rather than 0 as in the doc1 case)?  (Though really we should stop notifying the wrong document.)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, wait, there's a patch in my review queue on bug 633271 to do this -- so basically ignore all the comments here except the "r=dbaron".

Comment 6

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/projects/cedar/rev/a9f63be5e953

Comment 7

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/a9f63be5e953