1475637 - Provide a useful logging mechanism to go from the gecko display list to the WR display list

Status: RESOLVED FIXED

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

Description

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

The ifdef in WebRenderLayerManager sort of does this, but it prints the gecko DL and the WR DL separately so it's hard to map items in one to items in the other. What I want is an interleaved dump, which prints out the WR DL items generated for each gecko DL item.

I have patches that do this.

Comment 1

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

Attachment: Part 0 - WR patch

Comment 2

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

Attachment: Part 1 - Dumping patch

Comment 3

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

Ugh, the part 1 patch doesn't put the wrap list display items in the right place. Need to fix that.

Comment 4

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

Attachment: Part 1 - Dumping patch

Comment 5

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

Attachment: Bug 1475637 - Add a mechanism for dumping an interleaved display list.

The prefs, when enabled, will dump the gecko DL items followed by the
WR DL items that were generated from that gecko item. This allows us to
easily go from a DOM element with known id/class attributes to e.g. an
ImageKey of an image that was generated for that element.

Also, this logging can be enabled in CI builds just like gecko display-list
dumping, instead of the ifdef that we previously had in WebRenderLayerManager.

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

Comment 6

[Code Review Bot [:reviewbot]]

Comment on attachment 8993463 [details]
Bug 1475637 - Add a mechanism for dumping an interleaved display list.

https://reviewboard.mozilla.org/r/258204/#review265214


Code analysis found 4 defects in this patch:
 - 4 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: gfx/layers/wr/WebRenderLayerManager.cpp:297
(Diff revision 1)
>  
>    mWidget->AddWindowOverlayWebRenderCommands(WrBridge(), builder, resourceUpdates);
>    mWindowOverlayChanged = false;
> +  if (dumpEnabled) {
> +    printf_stderr("(window overlay)\n");
> +    builderDumpIndex = builder.Dump(/*indent*/ 1, Some(builderDumpIndex), Nothing());

Warning: Value stored to 'builderDumpIndex' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

    builderDumpIndex = builder.Dump(/*indent*/ 1, Some(builderDumpIndex), Nothing());
    ^

::: gfx/layers/wr/WebRenderLayerManager.cpp:297
(Diff revision 1)
>  
>    mWidget->AddWindowOverlayWebRenderCommands(WrBridge(), builder, resourceUpdates);
>    mWindowOverlayChanged = false;
> +  if (dumpEnabled) {
> +    printf_stderr("(window overlay)\n");
> +    builderDumpIndex = builder.Dump(/*indent*/ 1, Some(builderDumpIndex), Nothing());

Warning: Value stored to 'builderDumpIndex' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

    builderDumpIndex = builder.Dump(/*indent*/ 1, Some(builderDumpIndex), Nothing());
    ^

::: gfx/webrender_bindings/WebRenderAPI.cpp:761
(Diff revision 1)
>  void DisplayListBuilder::Restore() { wr_dp_restore(mWrState); }
>  void DisplayListBuilder::ClearSave() { wr_dp_clear_save(mWrState); }
> -void DisplayListBuilder::Dump() { wr_dump_display_list(mWrState); }
>  
> +usize DisplayListBuilder::Dump(usize aIndent,
> +                              Maybe<usize> aStart,

Error: Type 'Maybe<mozilla::wr::usize>' (aka 'Maybe<unsigned long>') must not be used as parameter [clang-tidy: mozilla-non-memmovable-template-arg]

                              Maybe<usize> aStart,
                                           ^

::: gfx/webrender_bindings/WebRenderAPI.cpp:762
(Diff revision 1)
>  void DisplayListBuilder::ClearSave() { wr_dp_clear_save(mWrState); }
> -void DisplayListBuilder::Dump() { wr_dump_display_list(mWrState); }
>  
> +usize DisplayListBuilder::Dump(usize aIndent,
> +                              Maybe<usize> aStart,
> +                              Maybe<usize> aEnd)

Error: Type 'Maybe<mozilla::wr::usize>' (aka 'Maybe<unsigned long>') must not be used as parameter [clang-tidy: mozilla-non-memmovable-template-arg]

                              Maybe<usize> aEnd)
                                           ^

Comment 7

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

Comment on attachment 8993463 [details]
Bug 1475637 - Add a mechanism for dumping an interleaved display list.

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

Comment 8

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

Whoops, that updated patch is the same as the original patch.

Comment 9

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

Comment on attachment 8993463 [details]
Bug 1475637 - Add a mechanism for dumping an interleaved display list.

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

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8993463 [details]
Bug 1475637 - Add a mechanism for dumping an interleaved display list.

https://reviewboard.mozilla.org/r/258204/#review265226

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e38a0f11e03
Add a mechanism for dumping an interleaved display list. r=jrmuizel

Comment 12

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/6e38a0f11e03