423998 - Fix repainting regression(s) and multiple invalidation bugs

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Use new InvalidateCoveredRegion in nsSVGGlyphFrame::UpdateGraphic
Make nsSVGGeometryFrame::UpdateGraphic return void
Reword suppressInvalidation logic
a) check whether its necessary in UpdateGraphic or caller should deal
b) don't use default arguments

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Highlights:

First the easy bits.

Rename UpdateGraphic in text/glyphs to NotifyGlyphMetricsChange (that's what this function eventually calls).
This allows us to rename nsSVGGlyphFrame::UpdateGeometry to nsSVGGlyphFrame::UpdateGraphic to make the naming consistent between nsSVGPathGeometryFrame and nsSVGForeignObjectFrame

Remove unused arguments and simplify suppressInvalidation processing.

Remove unused code from nsSVGUtils.cpp

Now the more complicated bits...

Ensure nsSVGGlyphFrame::UpdateGraphic invalidates the right areas if filters are present. I missed this out of the previous patch as the function had a different name (hence the renaming above).

Ensure  nsSVGTextFrame::UpdateGlyphPositioning sets mPositioningDirty early as otherwise an infinite loop will occur with filters. This function calls SetGlyphPosition which calls UpdateGraphic, the invalidation that occurs requires the bounding box to be calculated which calls UpdateGlyphPositioning to ensure that the bounding box is correct.

Make nsSVGTextFrame::NotifyGlyphMetricsChange call UpdateGlyphPositioning. Roc removed it in bug 392233 but needs to be there. Lets say you have some text and you append or delete it thus getting into nsSVGGlyphFrame::CharacterDataChanged. This calls NotifyGlyphMetricsChange which just sets a flag now. Unless you manually refresh the screen there is no update. You just can't get quality reviewers these days (and I don't mean vlad).

Comment 2

[Robert Longson [:longsonr]]

Note that without this patch http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-types-basicDOM-01-b.html is missing the text in the bottom left corner as that is dynamically added. See last paragraph of comment 1 for why.

Comment 3

[Damon Sicore (:damons)]

Why should this block?  -'ing for now.  Need to know the impact here.  

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Robert, why did you obsolete this patch, are you planning a new one?

This should land for 1.9 since it fixes a regression.

Comment 5

[Robert Longson [:longsonr]]

Attachment: updated patch

This moves us to a single non-virtual UpdateGraphic function.

Comment 6

[Robert Longson [:longsonr]]

Comment on attachment 311122 [details] [diff] [review]
updated patch

Oops, unfortunately while the text in the testcase now displays it is incorrect. GetScreenCTM is displaying the wrong answer.

Comment 7

[Robert Longson [:longsonr]]

Attachment: WIP reftests

Comment 8

[Robert Longson [:longsonr]]

(In reply to comment #6)

I can't reproduce this fault now.

Comment 9

[Robert Longson [:longsonr]]

Attachment: retests

Comment 10

[Robert Longson [:longsonr]]

This patch fixes regressions in dynamic text display and also fixes text to work properly in the face of dynamic filter changes.

Comment 11

[Robert Longson [:longsonr]]

Attachment: updated patch

Previous patch had problems with text disappearing in some cases. One line changed here moves mPositioningDirty = PR_FALSE in UpdateGlyphPositioning slightly later to be closer to previous logic which fixes things.

Comment 12

[Robert Longson [:longsonr]]

Comment on attachment 311199 [details] [diff] [review]
updated patch

There is still something wrong. http://www.codedread.com/svgtest.svg does not display all the time after a shift refresh. Moving the mouse on the firefox display causes the missing text to display.

Apologies for all the review requests.

Comment 13

[Robert Longson [:longsonr]]

Attachment: updated patch

Added a NotifyGlyphMetricsChange to nsSVGGlyphFrame::InitialUpdate so that http://www.codedread.com/svgtest.svg displays. That was another regression from bug 392233.

Comment 14

[Robert Longson [:longsonr]]

Attachment: remove stuff not part of patch

Comment 17

[Jonathan Watt [:jwatt]]

Comment on attachment 311262 [details] [diff] [review]
remove stuff not part of patch

r=jwatt

This patch was a bit of a PITA to review, not because there's anything wrong with the patch, but because of the confused state of our code. I'm quite glad to see these changes. While I wouldn't dare claim we'll now be able to say our notification and invalidation scheme is sane, it's a nice step forward.

I've tried really hard to find something wrong with the patch, but (despite some false alarms) it looks good to me. Please address the following though:

nsSVGUtils.cpp: The DoUpdate changes are not necessary. Please revert them for now. In UpdateGraphic the NS_STATE_SVG_NONDISPLAY_CHILD check is not valid for all nsIFrame implementations. That bit is used for different things on non-SVG frames. You should be able to just change the argument to be of type nsISVGChildFrame* and kill the QI.

My only other comment would be that the UpdateGlyphPositioning call in nsSVGTextFrame::NotifyGlyphMetricsChange seems quite undesirable. If that's the way it was before then you're right, probably safest to just put it back for now.

Comment 18

[Jonathan Watt [:jwatt]]

We should certainly block on this. These repainting bugs are serious and at least one of them is a regression.

Comment 19

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to comment #17)

> nsSVGUtils.cpp: The DoUpdate changes are not necessary. Please revert them for
> now. 

Done.

> In UpdateGraphic the NS_STATE_SVG_NONDISPLAY_CHILD check is not valid for
> all nsIFrame implementations. That bit is used for different things on non-SVG
> frames. You should be able to just change the argument to be of type
> nsISVGChildFrame* and kill the QI.

Done, however GetStateBits is a method of nsIFrame so I still need a QI. Doing it your way I need it from nsISVGChildFrame to nsIFrame rather than vice versa.

We could put guards into various functions using IsFrameOfType in some other followup bug I guess.

> 
> My only other comment would be that the UpdateGlyphPositioning call in
> nsSVGTextFrame::NotifyGlyphMetricsChange seems quite undesirable. If that's the
> way it was before then you're right, probably safest to just put it back for
> now.
> 

It's necessary to prevent some of the redraw regressions.

Comment 20

[Robert Longson [:longsonr]]

Attachment: oops redid the wrong bit

Comment 21

[Jonathan Watt [:jwatt]]

(In reply to comment #19)
> Done, however GetStateBits is a method of nsIFrame so I still need a QI. Doing
> it your way I need it from nsISVGChildFrame to nsIFrame rather than vice versa.

Oh, right. Still, best to enforce that we have an SVG frame at compile time, rather than run time.

> We could put guards into various functions using IsFrameOfType in some other
> followup bug I guess.

Again, I'd prefer to see us enforce types at compile time, perhaps using a blank common-to-all SVG base class. Anyway, that's for another bug.

> > My only other comment would be that the UpdateGlyphPositioning call in
> > nsSVGTextFrame::NotifyGlyphMetricsChange seems quite undesirable. If that's the
> > way it was before then you're right, probably safest to just put it back for
> > now.
> > 
> 
> It's necessary to prevent some of the redraw regressions.

Sorry, when I said undesirable, I meant from a performance perspective. Updating each time we set the dirty flag seems to defeat the purpose of having a dirty flag in the first place, so it would be nice to figure out what's wrong there (but safer to leave that until after 1.9 at this stage).

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 313860 [details] [diff] [review]
oops redid the wrong bit

+    nsISVGChildFrame* svgChildFrame = nsnull;
+    CallQueryInterface(this, &svgChildFrame);

I'd rather call the variable "svgFrame" since it refers to *this* frame, not a child.

Otherwise looks good. But how do we ensure that when we change content that's be used as a mask or pattern, the users of the mask or pattern get updated?

Comment 23

[Robert Longson [:longsonr]]

Attachment: for check-in

(In reply to comment #22)
> (From update of attachment 313860 [details] [diff] [review])
> +    nsISVGChildFrame* svgChildFrame = nsnull;
> +    CallQueryInterface(this, &svgChildFrame);
> 
> I'd rather call the variable "svgFrame" since it refers to *this* frame, not a
> child.

Done.

> 
> Otherwise looks good. But how do we ensure that when we change content that's
> be used as a mask or pattern, the users of the mask or pattern get updated?
> 

For mask, filter and clip we have mutation observers on the content and we call nsSVGMaskProperty::DoUpdate() et al when the content changes.

For gradient and pattern we use the old nsISVGValue mechanism to observe the content frames and the content frames tell us when things have changed. We should try to replace this post 1.9.

Comment 24

[Robert Longson [:longsonr]]

Comment on attachment 313932 [details] [diff] [review]
for check-in

Although this patch has a fair number of lines in it quite a lot of that is converting the existing 3 implementations of UpdateGraphic (with their own individual bugs) into one implementation. Other changes are simply reinstatements of previously existing function calls to ensure we always redraw text changed.

Comment 25

[Robert Longson [:longsonr]]

It also comes with reftests for risk mitigation.

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 313932 [details] [diff] [review]
for check-in

a1.9=beltzner

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This seems like it should be a blocker.

Comment 28

[Robert Longson [:longsonr]]

checked in.

Comment 29

[Robert Longson [:longsonr]]

reftests checked in:

http://lxr.mozilla.org/seamonkey/source/layout/reftests/svg/dynamic-rect-01.svg
http://lxr.mozilla.org/seamonkey/source/layout/reftests/svg/dynamic-rect-02.svg
http://lxr.mozilla.org/seamonkey/source/layout/reftests/svg/dynamic-rect-03.svg
http://lxr.mozilla.org/seamonkey/source/layout/reftests/svg/dynamic-text-01.svg
http://lxr.mozilla.org/seamonkey/source/layout/reftests/svg/dynamic-text-02.svg
http://lxr.mozilla.org/seamonkey/source/layout/reftests/svg/dynamic-text-03.svg