Closed Bug 200931 Opened 17 years ago Closed 16 years ago

[FIX]Implement batching of style sheet addition/removal

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.6alpha

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

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).
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.
Priority: -- → P1
Target Milestone: --- → mozilla1.4alpha
Target Milestone: mozilla1.4alpha → mozilla1.5alpha
Blocks: 193069
Attached patch Proposed patchSplinter Review
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 on attachment 132185 [details] [diff] [review]
Proposed patch

What do you guys think?
Attachment #132185 - Flags: superreview?(jst)
Attachment #132185 - Flags: review?(peterv)
Summary: Implement batching of style sheet addition/removal → [FIX]Implement batching of style sheet addition/removal
Target Milestone: mozilla1.5alpha → mozilla1.6alpha
Blocks: 131775
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
Attachment #132185 - Flags: superreview?(jst) → superreview+
> Should we write a little nsAutoUpdater

That's bug 131775 (blocked by this bug, because I hate conflicts with myself).
Hidden returns in NS_ENSURE_* considered harmful! ;-)

/be
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?
Attachment #132185 - Flags: review?(peterv) → review+
> 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...
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.