864000 - document mPositioningDirty from nsSVGTextFrame2 better

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Attachment: patch

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

Comment 1

[Robert Longson [: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?

Comment 2

[Jonathan Watt [:jwatt]]

Not in nsSVGTextFrame2. Look at the patch. As I said in comment 0, the only place we read from it is in an assert.

Comment 3

[Cameron McCormack (:heycam)]

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?

Comment 4

[Jonathan Watt [:jwatt]]

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

Comment 5

[Cameron McCormack (:heycam)]

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.

Comment 6

[Jonathan Watt [:jwatt]]

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

Comment 7

[Cameron McCormack (:heycam)]

Yes, I think that suggested comment is better.  r=me if you want to land a comment change to that effect.

Comment 8

[Robert Longson [:longsonr]]

mPositioningDirty could/should become an SVG frame state bit though.

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 739951 [details] [diff] [review]
patch

Cancelling review/feedback, since we're not doing this.

Comment 10

[Cameron McCormack (:heycam)]

(In reply to Robert Longson from comment #8)
> mPositioningDirty could/should become an SVG frame state bit though.

I filed bug 865901 for this.

Comment 11

[Cameron McCormack (:heycam)]

I'm comment 6 as a rubber stamp for that comment change.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba72022a3db0

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ba72022a3db0