Closed Bug 1397059 Opened 7 years ago Closed 7 years ago

Make sure the frame+displayitemkey for nsDisplayTextOverflowMarker is unique

Categories

(Core :: Web Painting, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch overflow-marker-frame (obsolete) — Splinter Review
FrameLayerBuilder requires that the Frame()/GetPerFrameKey() is unique for each display item, but we haven't had great assertions enforcing this in the past. We use this for retaining information about layerization/invalidation, and with retained-dl, merging. Retained-dl has added new assertions on this, so they show up more often. nsDisplayTextOverflow can be created multiple times (twice per line), but we only differentiate between the before/after versions, all lines use the same frame. I couldn't figure out a great way to differentiate these items per-line, so I switched to instead using the first frame of the line as the frame. What do you think Mats?
Attachment #8904768 - Flags: review?(mats)
Assignee: nobody → matt.woodrow
Comment on attachment 8904768 [details] [diff] [review] overflow-marker-frame > FrameLayerBuilder requires that the Frame()/GetPerFrameKey() is unique for each display item OK, is this the only issue we're trying to fix here? If so, it seems like a rather big change to address that issue. I really don't think we want to associate these DisplayItems with whatever happens to be first on each line. These items really do belong to the block frame. We should definitely not use the style of any child frame in any way to display these markers. That would be wrong per spec. Could we instead generate unique keys by making mIndex consecutive? (mIndex is currently 0/1 for the start/end marker of all lines, but maybe we could use 0/1 on the 1st line, 2/3 on the 2nd line etc)
Attachment #8904768 - Flags: review?(mats) → review-
We really want the index to be stable, so that if we do a partial display list build that only includes the second line, we still mark the marker item as belonging to the second line and not the first. I couldn't see an easy way to determine what line 'number' we're processing, do you have any ideas?
Flags: needinfo?(mats)
Oh, I thought it only needed to be unique within a single display list, not that it needed to be stable across separate paint requests. TBH, that seems like a rather burdensome requirement to put on Layout. We don't maintain line numbers, and I suspect trying to do so would be horribly inefficient. I guess we could try to mangle a number out of the aLine pointer if that's good enough? Could you describe what GetPerFrameKey() is used for?
Flags: needinfo?(mats) → needinfo?(matt.woodrow)
In the current m-c frame+GetPerFrameKey() is used to uniquely identify display items so that we can match that up to the corresponding items in the previous paint and retrieve layer assignment and invalidation data. With retained-dl, we build a partial display list (for the modified area), and then merge it into the old list. The key is again used to identify matching items, so that we only put each item into the merged list once. If we build a partial list containing an overflow marker for the second line, but has a key that matches the marker for the first line (last time), then we'll end up with two items painting markers at the second line's location, and none for the first. Mangling the aLine pointer would work, but I'm not sure how to do that without relying on implementation details of the allocator, which is a bit scary.
Flags: needinfo?(matt.woodrow)
Thanks for the description. AFAICT, with TYPE_BITS = 8 we have 24 bits to play with. The vast majority of text-overflow:ellipsis on the web will only ever have one marker (which is obviously always unique). Blocks with many thousands of lines with ellipsis markers is an extremely rare edge case. With 24 bits, it seems unlikely that we'd get collisions even for that edge case, although it may happen of course. We do have full control over all line allocations (NS_NewLineBox) though, in case that could help in some way. I wonder if we could change the matching on your side, say by having a DisplayItem::HasSamePerFrameKey(DisplayItem& aOtherItem) method that would be called for some selected display item types (only TEXT_OVERFLOW for now). Then we could keep the actual aLine pointer on the item itself and compare that in case of a PerFrameKey match. If it returns false you'd know it's a collision, and perhaps we could take a slow path for that case (e.g. invalidate the entire block)?
Flags: needinfo?(matt.woodrow)
Could we make nsBlockFrame::BuildDisplayList iterate all the lines if there is text-overflow present? (like we do if there descendant placeholders). That way aDrawnLines becomes a meaningful line number identifier, and we can encode it into the PerFrameKey. Having a virtual HasSamePerFrameKey() would fix the problem with retained-dl (where we have both the new and old items available), but doesn't help with the invalidation issue (the retained data isn't the display item, just a blob of geometry data hashed using the PerFrameKey).
Flags: needinfo?(matt.woodrow)
Attached patch text-overflow-alternate (obsolete) — Splinter Review
Something like this? We might need to clamp the index so that the overflow case just regresses back to what we have now, rather than clobbering the actual display item type.
Attachment #8907099 - Flags: review?(mats)
> Having a virtual HasSamePerFrameKey() [...] FWIW, I don't think it needs to be virtual; if you know the items' type when you compare them, then you can safely downcast to nsDisplayTextOverflowMarker.
Comment on attachment 8907099 [details] [diff] [review] text-overflow-alternate The general idea is OK I guess, although it does slow down painting blocks with text-overflow by not using the |cursor| optimization for that case. Probably not a big in deal in practice though. I wonder if we could pass along and use |lineCount| instead though: http://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/layout/generic/nsBlockFrame.cpp#6790 It seems a bit cleaner than overloading the meaning of |drawnLines|. And call the new DisplayLine() param aLineNumberForTextOverflow or something to hint that it shouldn't be used for anything else.
BTW, is the "gLamePaintMetrics" stuff actually useful to devs working on painting? If not, I suggest we remove it. |drawnLines| looks inaccurate anyway since it counts cases where we don't create items for the line, so it's just a "how many lines did we iterate" counter: http://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/layout/generic/nsBlockFrame.cpp#6674,6689
gLamePaintMetrics has never been useful for me at least, so I'd be ok with ripping it out. It's not a lot of code though.
Attachment #8904768 - Attachment is obsolete: true
Attachment #8907099 - Attachment is obsolete: true
Attachment #8907099 - Flags: review?(mats)
Attachment #8907179 - Flags: review?(mats)
Comment on attachment 8907179 [details] [diff] [review] Generate unique nsDisplayTextOverflowMarker items Looks great; one minor nit about the comment: >+ // to generate unique display items. Perhaps "to generate unique display item keys." is slightly clearer?
Attachment #8907179 - Flags: review?(mats) → review+
Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/14d1ec483997 Make sure we generate unique display item keys for nsDisplayTextOverflowMarker. r=mats
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: