Closed Bug 864000 Opened 7 years ago Closed 7 years ago

document mPositioningDirty from nsSVGTextFrame2 better

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: jwatt, Assigned: heycam)

Details

Attachments

(1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
There doesn't seem to be any reason for mPositioningDirty to exist on nsSVGTextFrame2. If that's true, we should remove it. The only place that it's used is in an assertion, and that assertion only existed in the old nsSVGTextFrame class in order to ensure that we were properly keeping mPositioningDirty in sync with the dirty bits (or at least that we weren't setting it to true if the dirty bits weren't also set).
Attachment #739951 - Flags: review?(cam)
Attachment #739951 - Flags: feedback?(longsonr)
mPositioningDirty stops us infinite looping when we have text in a pattern, mask or clipPath. 

On top of that why would we want to keep recalculating the positions when we already know them?
Not in nsSVGTextFrame2. Look at the patch. As I said in comment 0, the only place we read from it is in an assert.
The patch Robert just posted in bug 839958 would look at mPositioningDirty at the top of UpdateGlyphPositioning to see if we can exit early.  I am surprised that wasn't there before, to be honest -- we call UpdateGlyphPositioning() at the top of many DOM facing methods.

But I guess you are be asserting Jonathan that the check on the frame dirty bits should serve the same purpose as mPositioningDirty?
(In reply to Cameron McCormack (:heycam) from comment #3)
> But I guess you are be asserting Jonathan that the check on the frame dirty
> bits should serve the same purpose as mPositioningDirty?

Preferably. If not, I'd like us to document why, because the purpose and semantics of mPositioningDirty vs the dirty bits really needs to be made clear.
So there are cases where we can avoid doing some work by keeping a separate flag like this.  For example, say we modify a text positioning attribute.  We want to run DoGlyphPositioning() again, but we don't want to reflow the anonymous block frame, since that's not going to change anything.
We currently have:

  /**
   * Whether something has changed to invalidate the values in mPositions.
   */

Can we try and improve on that to draw attention to the different semantics of the dirty bits and this member? Maybe something along the lines of:

  /**
   * The NS_FRAME_IS_DIRTY and NS_FRAME_HAS_DIRTY_CHILDREN bits indicate
   * that our anonymous block child needs to be reflowed, and that mPositions
   * will likely need to be updated as a consequence. These are set, for
   * example, when the font-family changes. Sometimes we only need to
   * update mPositions though. For example if the x/y attributes change.
   * mPositioningDirty is used to indicate this latter "things are dirty" case
   * to allow us to avoid reflowing the anonymous block when it is not necessary.
   */
  bool mPositioningDirty;

Or something...
Yes, I think that suggested comment is better.  r=me if you want to land a comment change to that effect.
mPositioningDirty could/should become an SVG frame state bit though.
Comment on attachment 739951 [details] [diff] [review]
patch

Cancelling review/feedback, since we're not doing this.
Attachment #739951 - Attachment is obsolete: true
Attachment #739951 - Flags: review?(cam)
Attachment #739951 - Flags: feedback?(longsonr)
(In reply to Robert Longson from comment #8)
> mPositioningDirty could/should become an SVG frame state bit though.

I filed bug 865901 for this.
Summary: Remove mPositioningDirty from nsSVGTextFrame2 → document mPositioningDirty from nsSVGTextFrame2 better
I'm comment 6 as a rubber stamp for that comment change.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba72022a3db0
Assignee: nobody → cam
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/ba72022a3db0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.