Closed Bug 423998 Opened 14 years ago Closed 14 years ago

Fix repainting regression(s) and multiple invalidation bugs

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: longsonr, Assigned: longsonr)

References

()

Details

(Keywords: regression, testcase)

Attachments

(2 files, 8 obsolete files)

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
Attached patch patch (obsolete) — Splinter Review
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).
Attachment #310776 - Flags: review?(jwatt)
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.
Attachment #310776 - Flags: review?(jwatt)
Attachment #310776 - Attachment is obsolete: true
Why should this block?  -'ing for now.  Need to know the impact here.  
Flags: blocking1.9? → blocking1.9-
Robert, why did you obsolete this patch, are you planning a new one?

This should land for 1.9 since it fixes a regression.
Attached patch updated patch (obsolete) — Splinter Review
This moves us to a single non-virtual UpdateGraphic function.
Attachment #311122 - Flags: review?(jwatt)
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.
Attachment #311122 - Flags: review?(jwatt)
Attached patch WIP reftests (obsolete) — Splinter Review
(In reply to comment #6)

I can't reproduce this fault now.
Attached patch retestsSplinter Review
Attachment #311125 - Attachment is obsolete: true
Attachment #311168 - Flags: review?(jwatt)
Attachment #311122 - Flags: review?(jwatt)
This patch fixes regressions in dynamic text display and also fixes text to work properly in the face of dynamic filter changes.
Attached patch updated patch (obsolete) — Splinter Review
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.
Attachment #311122 - Attachment is obsolete: true
Attachment #311199 - Flags: review?(jwatt)
Attachment #311122 - Flags: review?(jwatt)
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.
Attachment #311199 - Flags: review?(jwatt)
Attached patch updated patch (obsolete) — Splinter Review
Added a NotifyGlyphMetricsChange to nsSVGGlyphFrame::InitialUpdate so that http://www.codedread.com/svgtest.svg displays. That was another regression from bug 392233.
Attachment #311199 - Attachment is obsolete: true
Attached patch remove stuff not part of patch (obsolete) — Splinter Review
Attachment #311261 - Attachment is obsolete: true
Attachment #311262 - Flags: review?(jwatt)
Duplicate of this bug: 426503
Duplicate of this bug: 427064
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.
Attachment #311262 - Flags: review?(jwatt) → review+
We should certainly block on this. These repainting bugs are serious and at least one of them is a regression.
Blocks: 426503, 427064
Flags: blocking1.9- → blocking1.9?
Summary: Improve invalidation logic → Fix repainting regression(s) and multiple invalidation bugs
Attached patch address review comments (obsolete) — Splinter Review
(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.
Attachment #311262 - Attachment is obsolete: true
Attachment #313859 - Flags: superreview?(roc)
Attached patch oops redid the wrong bit (obsolete) — Splinter Review
Attachment #313859 - Attachment is obsolete: true
Attachment #313860 - Flags: superreview?(roc)
Attachment #313859 - Flags: superreview?(roc)
(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 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?
Attachment #313860 - Flags: superreview?(roc) → superreview+
Attached patch for check-inSplinter Review
(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.
Attachment #313860 - Attachment is obsolete: true
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.
Attachment #313932 - Flags: approval1.9?
It also comes with reftests for risk mitigation.
Comment on attachment 313932 [details] [diff] [review]
for check-in

a1.9=beltzner
Attachment #313932 - Flags: approval1.9? → approval1.9+
Flags: blocking1.9?
This seems like it should be a blocker.
Flags: blocking1.9+
Whiteboard: [reviewed patch in hand]
Blocks: 392233
Attachment #311168 - Flags: review?(jwatt)
checked in.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [reviewed patch in hand]
Flags: in-testsuite?
Depends on: 428228
You need to log in before you can comment on or make changes to this bug.