1407767 - wr-text: (re)implement text bullets

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Aria Beingessner [:Gankra]]

They're currently implemented using the old text-layers style (before I started on the path of TextDrawTarget). They also never(?) seem to succeed, so we always fallback.

We should just port this to TextDrawTarget, and maybe also remove non-webrender bullet layers.

Comment 1

[Aria Beingessner [:Gankra]]

Attachment: Bug 1407767 - reimplement text bullets to use TextDrawTarget.

This makes the implementation more robust and unifies all our text handling
under one implementation.

Also includes some refatoring of our CreateWebRenderCommands impl to divest
us from the layerful/stateful GetLayerState(). Future work should be done
here, but that's low priority for the moment.

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

Comment 2

[Aria Beingessner [:Gankra]]

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=70aa0c985b4bfbd107efdc41a10f87e612221f79

Comment 3

[Aria Beingessner [:Gankra]]

Comment on attachment 8919476 [details]
Bug 1407767 - reimplement text bullets to use TextDrawTarget.

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

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8919476 [details]
Bug 1407767 - reimplement text bullets to use TextDrawTarget.

https://reviewboard.mozilla.org/r/190326/#review195604

::: layout/generic/nsBulletFrame.cpp:679
(Diff revision 2)
>                                           wr::IpcResourceUpdateQueue& aResources,
>                                           const StackingContextHelper& aSc,
>                                           mozilla::layers::WebRenderLayerManager* aManager,
>                                           nsDisplayListBuilder* aDisplayListBuilder)
>  {
> -  ContainerLayerParameters parameter;
> +  if (!ShouldUseAdvancedLayer(aManager, gfxPrefs::LayersAllowBulletLayers)) {

Let's just drop this check completely. I don't think we have a reason to disable bullet layers.

Comment 5

[Aria Beingessner [:Gankra]]

Comment on attachment 8919476 [details]
Bug 1407767 - reimplement text bullets to use TextDrawTarget.

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

Comment 6

[Aria Beingessner [:Gankra]]

Comment on attachment 8919476 [details]
Bug 1407767 - reimplement text bullets to use TextDrawTarget.

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

Comment 7

[Aria Beingessner [:Gankra]]

Opt results were all unexpected passes, debug results were busted due to debug assertions.

New push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad5236e345504f0ecc09a8eb66e212b4d6c2ab74

Removed check as requested.

Also removed two assertions which are no longer valid (checking for stateful things we no longer use in the webrender path).

Comment 8

[Aria Beingessner [:Gankra]]

Attachment: Bug 1407767 - mark bullet text tests as passing again.

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

Comment 9

[Aria Beingessner [:Gankra]]

Ok looks stable and good. Added new passes in second commit.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8919541 [details]
Bug 1407767 - mark bullet text tests as passing again.

https://reviewboard.mozilla.org/r/190384/#review195844

Yay!

Comment 11

[Aria Beingessner [:Gankra]]

rebase needed

Comment 12

[Aria Beingessner [:Gankra]]

Comment on attachment 8919476 [details]
Bug 1407767 - reimplement text bullets to use TextDrawTarget.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190326/diff/4-5/

Comment 13

[Aria Beingessner [:Gankra]]

Comment on attachment 8919541 [details]
Bug 1407767 - mark bullet text tests as passing again.

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

Comment 14

[Aria Beingessner [:Gankra]]

rebased (no changes, modifications were to deleted code)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d3b2d4927ef
reimplement text bullets to use TextDrawTarget. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/8edd193955eb
mark bullet text tests as passing again. r=kats

Comment 16

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

Backed out for unexpectedly passing reftest layout/reftests/writing-mode/1111944-1-list-marker.html on Linux x64 QuantumRender:

https://hg.mozilla.org/integration/autoland/rev/cca075bb35a2075996743530365e6f92f19db741
https://hg.mozilla.org/integration/autoland/rev/a976cfead2c1896eb70b9845f163f57505be9dad

Log with unexpected pass: https://treeherder.mozilla.org/logviewer.html#?job_id=138261972&repo=autoland
REFTEST TEST-UNEXPECTED-PASS | file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/writing-mode/1111944-1-list-marker.html == file:///builds/worker/workspace/build/tests/reftest/tests/layout/reftests/writing-mode/1111944-1-list-marker-ref.html | image comparison, max difference: 0, number of differing pixels: 0

Comment 18

[Aria Beingessner [:Gankra]]

Comment on attachment 8919541 [details]
Bug 1407767 - mark bullet text tests as passing again.

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

Comment 19

[Aria Beingessner [:Gankra]]

Marked test as passing.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56eeab3db2d7
reimplement text bullets to use TextDrawTarget. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/a347e00ec6ad
mark bullet text tests as passing again. r=kats

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/56eeab3db2d7
https://hg.mozilla.org/mozilla-central/rev/a347e00ec6ad