731858 - TextOverflow creates 2 display items for a single frame

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: Add index numbers to nsDisplayTextOverflow

TextOverflow::CreateMarkers can create 2 nsDisplayTextOverflow items for a single frame. The pair of frame pointer and GetPerFrameKey() is meant to be unique for a display item.

I'm surprised that this isn't caught by FrameLayerBuilder currently, but it is with my DLBI code.

Patch adds a unique index to nsDisplayTextOverflow (same as was done for nsDisplayTransform) so that GetPerFrameKey() returns a unique value.

Comment 1

[Mats Palmgren (inactive)]

Comment on attachment 601817 [details] [diff] [review]
Add index numbers to nsDisplayTextOverflow

Thanks, looks good, but could you write it like this instead:

  virtual PRUint32 GetPerFrameKey() {
    return (mIndex << TYPE_BITS) | nsDisplayItem::GetPerFrameKey();
  }

so that it fits in < 80 columns.

FYI, the function ClipMarker() potentially also creates a nsDisplayClip per
nsDisplayTextOverflowMarker item (wrapping it), I assume that's OK since
nsDisplayClip::GetPerFrameKey() returns zero.

Comment 2

[Matt Woodrow (:mattwoodrow)]

(In reply to Mats Palmgren [:mats] from comment #1)
> Comment on attachment 601817 [details] [diff] [review]
> Add index numbers to nsDisplayTextOverflow
> 
> Thanks, looks good, but could you write it like this instead:
> 
>   virtual PRUint32 GetPerFrameKey() {
>     return (mIndex << TYPE_BITS) | nsDisplayItem::GetPerFrameKey();
>   }
> 
> so that it fits in < 80 columns.

Sure, done.

> 
> FYI, the function ClipMarker() potentially also creates a nsDisplayClip per
> nsDisplayTextOverflowMarker item (wrapping it), I assume that's OK since
> nsDisplayClip::GetPerFrameKey() returns zero.

Yeah, this is fine, nsDisplayClip isn't stored in FrameLayerBuilder.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4392f9d56d2e

Comment 3

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/4392f9d56d2e