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

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).

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.

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

Robert, why did you obsolete this patch, are you planning a new one? This should land for 1.9 since it fixes a regression.

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

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.

(In reply to comment #6) I can't reproduce this fault now.

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

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 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.

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

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.

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

(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.

(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?

(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 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.

It also comes with reftests for risk mitigation.

Comment on attachment 313932 [details] [diff] [review] for check-in a1.9=beltzner

This seems like it should be a blocker.

checked in.

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