200931 - [FIX]Implement batching of style sheet addition/removal

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

We want to be able to add/remove/enable/disable a whole bunch of stylesheets at
once while triggering only one style reresolve and hence one set of
reflow/repaint/frame construct operations.

When I do this, I need to fix the HTML sink's current brokenness where it will
flush the contexts on a sheet change and thus cause content notifications to be
lost (it should flush the contexts on beginning of a stylesheet
removal/addition/enabling batch).

Comment 1

[Boris Zbarsky [:bzbarsky]]

To clarify, currently the sink does not _flush_ the contexts; it just updates
them.  This means that frames get constructed, and the content is in the content
model, but some document observers do not get notified about it.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

The basic idea here is to enforce calling BeginUpdate/EndUpdate for all sheet
changes and rule changes, then to batch using those.  This fixes bug 193069 in
the  process.

Most of the patch is just updating the callers of BeginUpdate/EndUpdate.  The
substantive changes are in nsDocument.cpp and nsPresShell.cpp, as well as
nsIDocument.h and nsIDocumentObserver.h

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132185 [details] [diff] [review]
Proposed patch

What do you guys think?

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 132185 [details] [diff] [review]
Proposed patch

- In nsGenericHTMLElement::SetAttr():

   if (aNotify && mDocument) {
-    mDocument->BeginUpdate();
+    mDocument->BeginUpdate(UPDATE_CONTENT_MODEL);
     mDocument->AttributeWillChange(this, namespaceID, localName);
   }

   if (!mAttributes) {
     rv = NS_NewHTMLAttributes(&mAttributes);
     NS_ENSURE_SUCCESS(rv, rv);
   }
   rv = mAttributes->SetAttributeFor(aNodeInfo, aValue);
   NS_ENSURE_SUCCESS(rv, rv);

Early returns w/o calling EndUpdate(). There's probably more of these, do we
care? Should we write a little nsAutoUpdater helper class that could be used to
prevent this from happening?

sr=jst

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Should we write a little nsAutoUpdater

That's bug 131775 (blocked by this bug, because I hate conflicts with myself).

Comment 6

[Brendan Eich [:brendan]]

Hidden returns in NS_ENSURE_* considered harmful! ;-)

/be

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 132185 [details] [diff] [review]
Proposed patch

>Index: content/base/src/nsDocument.cpp
>===================================================================

> NS_IMETHODIMP
> nsDocument::UpdateStyleSheets(nsCOMArray<nsIStyleSheet>& aOldSheets,
>                               nsCOMArray<nsIStyleSheet>& aNewSheets)
> {
>+  // if an observer removes itself, we're ok (not if it removes
>+  // others though)
>+  PRInt32 obsIndx;
>+  for (PRInt32 obsIndx = mObservers.Count() - 1; obsIndx >= 0; --obsIndx) {

You declare obsIndx twice (you use it further down so keep the one outside the
for).

>@@ -2161,99 +2158,57 @@ nsDocument::AttributeChanged(nsIContent*
> 
> 
> NS_IMETHODIMP
> nsDocument::StyleRuleChanged(nsIStyleSheet* aStyleSheet,
>                              nsIStyleRule* aOldStyleRule,
>                              nsIStyleRule* aNewStyleRule)

>+  // Loop backwards to handle observers removing themselves
>+  for (i = mObservers.Count() - 1; i >= 0; --i) {
>     nsIDocumentObserver *observer =
>       NS_STATIC_CAST(nsIDocumentObserver *, mObservers.ElementAt(i));
> 

>+    observer->StyleRuleChanged(this, aStyleSheet, aOldStyleRule, aNewStyleRule);  }

Move the brace down.

>Index: content/html/document/src/nsHTMLContentSink.cpp
>===================================================================

>@@ -5341,121 +5341,49 @@ HTMLContentSink::UpdateAllContexts()
>   mCurrentContext->UpdateChildCounts();
> }
> 
> NS_IMPL_NSIDOCUMENTOBSERVER_LOAD_STUB(HTMLContentSink)
> NS_IMPL_NSIDOCUMENTOBSERVER_REFLOW_STUB(HTMLContentSink)
> NS_IMPL_NSIDOCUMENTOBSERVER_STATE_STUB(HTMLContentSink)
> NS_IMPL_NSIDOCUMENTOBSERVER_CONTENT(HTMLContentSink)

...

>+NS_IMPL_NSIDOCUMENTOBSERVER_STYLE_STUB(HTMLContentSink)

Maybe move this one next to the other _STUBs?
BTW, any reason why NS_IMPL_NSIDOCUMENTOBSERVER_CONTENT doesn't end in _STUB?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> You declare obsIndx twice

Fixed.

> Move the brace down.

Fixed.

> Maybe move this one next to the other _STUBs?

Fixed.

> BTW, any reason why NS_IMPL_NSIDOCUMENTOBSERVER_CONTENT doesn't end in _STUB?

It's all sicking's fault?  ;)

Patch checked in.  Slight regressions in Tp/Txul/Ts (if nothing else because the
sink now flushes out stuff on BeginUpdate more, which fixed bug 209217), but
those should hopefully get better when I fix bug 84582...