Boris Zbarsky [:bzbarsky] Reporter
	Description • 13 years ago

It's slow, we don't need it.  See bug 653887.

But actually, I think this would regress bug 423269 because we'd get Begin/EndUpdate happening under a scriptblocker with no outer updates....  Jonas, is that correct?  Do we need to move more things that listen for EndUpdate to post scriptrunners in EndUpdate, perhaps?

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 1 • 13 years ago

Gah.  There are also frame AttributeChanged implementations that mutate the frame tree (!).

I'll need to audit those....

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 2 • 13 years ago

Specifically, nsTableCellFrame::AttributeChanged calling nsTableFrame::AttributeChangedFor which changes the cellmap and then starts messing with colframes.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 3 • 13 years ago

And of course XBL does the whole add/remove nodes thing under nsXBLPrototypeBinding::AttributeChanged...

On the other hand, the dep on bug 653126 is the other way: if we stop calling BeginUpdate right up front in SetAttrAndNotify (and specifically if we don't call it anywhere before the SetAndTakeAttr call), then the problem of accidentally flushing the sink when our style data is not in a good state is gone.  So fixing this bug would automagically fix bug 653126.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 4 • 13 years ago

I further believe that the add/remove I mention in comment 3 is fine, except insofar as it triggers outermost EndUpdate while still under a scriptblocker.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 5 • 7 years ago

Not sure whether this has been coming up noticeably in quantum flow profiles...

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 7 years ago

Not too much, perhaps because RestyleManager's AttributeWillChange/AttributeChanged are so slow that
one may not even end up looking at Begin/EndUpdate so closely.
Or in innerHTML cases SetAttr happens before element is in tree so everything is reasonable fast.

I did file bug 1350885 since EndUpdate shows up a bit when inserting new elements to tree, so I guess same applies to SetAttr too.
(EndUpdate was showing up more before bug 1352389)
Hmm, but is this bug about nsIDocumentObserver's method, or the Begin/EndUpdate on nsDocument?
If the former, then this has shown up even less. I guess this is about that.


In a synthetic testcase nsDocument::EndUpdate does seem to take 3% of attribute setting.
(RestyleManager's AttributeWillChange/AttributeChanged together >22%)

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 7 • 7 years ago

> Hmm, but is this bug about nsIDocumentObserver's method, or the Begin/EndUpdate on nsDocument?

Both.  The latter calls the former, of course.  All the complicated logic that makes it hard to say whether it's ok to skip this is in the former.