1395729 - text-layers: dragging some text trips assertion

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Aria Beingessner [:Gankra]]

Attachment: text.html

While using layers-free webrendest:

If you select the whole line of attached text, and then try to drag the selection it crashes with:

Assertion failure: mMergedFrames.IsEmpty(), at /Users/ABeingessner/dev/gecko/layout/generic/nsTextFrame.cpp:5211

Seems we're still a bit inconsistent in our checking here.

Comment 1

[Aria Beingessner [:Gankra]]

Related: https://bugzilla.mozilla.org/show_bug.cgi?id=1392171 and https://bugzilla.mozilla.org/show_bug.cgi?id=1391996

cc :ethlin, in case you know why dragged selections might be special here.

Comment 2

[Aria Beingessner [:Gankra]]

So I discussed this with :mstange a bit and this is a bit of a mess. 

TL;DR: I'm going to just disable text-frame merging, because it doesn't work and might not be worth the trouble to support.

Basically, if you drag text, the way that's implemented is the relevant text-frames are drawn to a different DrawTarget with the pre-advanced-layers nsDisplayText::Paint path. This path doesn't support frame merging, and so it throws an assertion.

I don't believe the frame can strictly tell if it's going to be used in this manner, and so I don't think it can actually block merging for this case reliably. Maybe we could check if there's a selection...? I don't know if that's sufficient.

There's also just the matter that it's unclear how valuable text-frame merging is with webrender. We don't create a bunch of layers for each piece of text, just some extra DisplayItems. Computing a merge is potentially more work than it even saves.

Comment 3

[Aria Beingessner [:Gankra]]

Attachment: Bug 1395729 - Disable frame merging for nsTextFrame.

Review commit: https://reviewboard.mozilla.org/r/176946/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/176946/

Comment 4

[Aria Beingessner [:Gankra]]

Hey matt, just checking in that my analysis above seems reasonable to you, since you seemed to write this code originally.

Comment 5

[Aria Beingessner [:Gankra]]

Comment on attachment 8905150 [details]
Bug 1395729 - Disable frame merging for nsTextFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/176946/diff/1-2/

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8905150 [details]
Bug 1395729 - Disable frame merging for nsTextFrame.

https://reviewboard.mozilla.org/r/176946/#review182014

Can you just remove the frame merging code entirely?

We're not using it for anything (nor do we plan to), so we might as well be rid of it.

r=me with TryMerge and mMergedFrames gone entirely.

Comment 7

[Aria Beingessner [:Gankra]]

Comment on attachment 8905150 [details]
Bug 1395729 - Disable frame merging for nsTextFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/176946/diff/2-3/

Comment 8

[Aria Beingessner [:Gankra]]

Review comments addressed!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74131a35a149
Disable frame merging for nsTextFrame. r=mattwoodrow

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/74131a35a149

Comment 11

[Markus Stange [:mstange]]

Part 5 in bug 1359584 was also going to get rid of nsDisplayText merging.