Closed Bug 313351 Opened 16 years ago Closed 11 years ago

make nsIDocument::ContentStatesChanged take only one content node


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






(Reporter: dbaron, Assigned: bzbarsky)




(1 file)

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.)
Once we do this we should consider moving the notification from nsIDocumentObserver to nsIMutationObserver.
QA Contact: ian → events
Blocks: 598833
Attached patch Just Do ITSplinter Review
Attachment #511072 - Flags: review?(dbaron)
Assignee: dbaron → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
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]) {
  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
Attachment #511072 - Flags: review?(dbaron) → review+
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.)
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".
Blocks: 633271
Whiteboard: [need review] → [need landing]
Flags: in-testsuite-
Whiteboard: [need landing] → fixed-in-cedar
Target Milestone: --- → mozilla2.2
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.