280363 - SVG code does long-lived update batches

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 280214 comment 6.  Basically, Begin/EndUpdateViewBatch is meant for
synchronous update batches during which we do not come back to the event loop. 
Using this api blocks all painting for the entire viewmanager tree. 
Furthermore, while in the middle of an update batch we do not apply invalidates
to the rendering tree.  So if a widget move operation happens while in an update
batch (eg scrolling) or someone tries to call Composite() or otherwise paint,
the result will be somewhat unpredictable (see bug 280214 again; invalidates
were basically dropped on the floor there).

So we have the following questions:

1)  Should calling disableRefresh on an SVG element really disable all painting
    for the viewmanager tree?
2)  Why do SVG frames disable all painting between Init() and Reflow()?  What if
    the frame never gets reflown?  Say it's destroyed after Init() but before
    Reflow()?  (I suspect the right answer is, "this code should be removed").
3)  Depending on the answer to #1, does SVG need a new nsIViewManager api to
    handle disableRefresh?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

This testcase shows SVG permanently disabling painting for the document. 
Placed in a frame/iframe, this permanently (until the toplevel page is changed)
disables all painting for the toplevel page and all its frame descendants.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So, as far as the methods on SVGElement go, please see
<http://lists.w3.org/Archives/Public/www-svg/2005Feb/0033.html>.  In brief, the
behavior is so poorly defined for suspend/unsuspendRedraw, that I feel that they
should be no-ops in Mozilla.  roc says that's what they are in ASV as well, so....

Comment 3

[Alex Fritze]

Until we switch over to a queue-based notification system, these methods are
needed for performance reasons. Our notification batching code depends on them.
So e.g. the JS code 
circle.cx.baseVal.value = 10;
circle.cy.baseVal.value = 20;

is slower than

svg.suspendRedraw();
circle.cx.baseVal.value = 10;
circle.cy.baseVal.value = 20;
svg.unsuspendRedraw();

Comment 4

[Boris Zbarsky [:bzbarsky]]

I can leave that part in, and just take out the messing with the viewmanager...

Comment 5

[Alex Fritze]

(In reply to comment #4)
> I can leave that part in, and just take out the messing with the viewmanager...

Sounds good.

Comment 6

[Jonathan Watt [:jwatt]]

Boris, the patch for bug 340451 got rid of the Begin/EndUpdateViewBatch calls and replaced them with a call to UpdateView.

Comment 7

[Boris Zbarsky [:bzbarsky]]

By "the egin/EndUpdateViewBatch calls" you mean the ones that fall under item 2 in comment 0?

If so, this bug is fixed.

Comment 8

[Jonathan Watt [:jwatt]]

You fixed item 2 yourself in bug 305246 (which you set as blocking this bug), so I'm assuming you had other things in mind.

At any rate, there are now no longer any instances of the string "UpdateViewBatch" in any SVG files: http://mxr.mozilla.org/seamonkey/search?string=UpdateViewBatch&find=SVG&&tree=seamonkey

Comment 9

[Boris Zbarsky [:bzbarsky]]

No, bug 305246 fixed item 1.

Sounds like this is fixed.  Most excellent.