Bug 152956 – Make a way to test display-list drawing

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Simon Fraser (smfr)]

Need to be able to test display-list drawing in tests. I propose:
1. Ability to dump the display list for a compositing layer
2. Ability to turn DL on and off for a given test (or layer?)

[Simon Fraser (smfr)]

Created attachment 268671 [details]
Patch

[Simon Fraser (smfr)]

This will probably fail; we need to move the recording and replay code into GraphicsLayer so all platforms can use it.

[Simon Fraser (smfr)]

^ fail on GTK and EFL

[Build Bot]

Comment on attachment 268671 [details]
Patch

Attachment 268671 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/677404

New failing tests:
displaylists/layer-dispay-list.html

[Build Bot]

Created attachment 268673 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268671 [details]
Patch

Attachment 268671 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/677401

New failing tests:
displaylists/layer-dispay-list.html

[Build Bot]

Created attachment 268674 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Ryosuke Niwa]

Comment on attachment 268671 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268671&action=review

r=me assuming you fix builds.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3275
> +    return m_displayList->textRepresentation();

Why don't we call this asText to match the naming convention elsewhere?

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:281
> +

Stray blank line?

> Source/WebCore/platform/graphics/displaylists/DisplayList.cpp:63
> +    TextStream ts;

Can we call this "stream" instead?  Abbreviations like "ts" makes me feel icky.

[Simon Fraser (smfr)]

Created attachment 269102 [details]
Patch

[WebKit Commit Bot]

Attachment 269102 [details] did not pass style-queue:


ERROR: Source/WebCore/testing/Internals.h:260:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Created attachment 269109 [details]
Patch

[WebKit Commit Bot]

Attachment 269109 [details] did not pass style-queue:


ERROR: Source/WebCore/testing/Internals.h:260:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

https://trac.webkit.org/r195156

[Darin Adler]

Comment on attachment 269109 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269109&action=review

Is the dump format terse enough? I worry that it's too "vertical" to read quickly. And there seems to be no special case for when extent and rect are the same. Worth doing a little work on the format before we make tons of tests that lock it in.

> Source/WebCore/platform/graphics/displaylists/DisplayList.cpp:66
> +            const auto& stateItem = downcast<SetState>(item);

Seems like we want the change flags in a  local variable, rather than just the casted item.

> Source/WebCore/platform/graphics/displaylists/DisplayList.cpp:75
> +            // FIXME: for now, only drop the item if the only state-change flags are platform-specific.
> +            if (stateItem.state().m_changeFlags == GraphicsContextState::ShouldSubpixelQuantizeFontsChange)
> +                return false;
> +
> +            if (stateItem.state().m_changeFlags == GraphicsContextState::AntialiasedFontDilationEnabledChange)
> +                return false;
> +
> +            if (stateItem.state().m_changeFlags == GraphicsContextState::ShouldSubpixelQuantizeFontsChange)
> +                return false;

Seems like a switch statement is what we want here, not three if statements.

> Source/WebCore/platform/graphics/displaylists/DisplayList.cpp:96
> +    size_t numItems = m_list.size();
> +    for (size_t i = 0; i < numItems; ++i) {
> +        const auto& item = m_list[i].get();

Why not use a modern for loop instead?

> Source/WebCore/platform/graphics/displaylists/DisplayList.h:47
> +enum AsTextFlag {
> +    None                            = 0,
> +    IncludesPlatformOperations      = 1 << 0,
> +};

Our coding style document specifically says we don’t do this, lining things up with spaces. You should either conform or get consensus that we should change the coding style document.

> Source/WebCore/testing/Internals.cpp:1980
> +    Document* document = contextDocument();

Seems a little strange to use contextDocument() here instead of element->document(), which can never be null.

> Source/WebCore/testing/Internals.cpp:2007
> +    Document* document = contextDocument();

Seems a little strange to use contextDocument() here instead of element->document(), which can never be null.

> Source/WebCore/testing/Internals.idl:232
> +    // Flags for displayListForElement.
> +    const unsigned short DISPLAY_LIST_INCLUDES_PLATFORM_OPERATIONS = 1;
> +    [RaisesException] DOMString displayListForElement(Element element, optional unsigned short flags);

Do we really need to use flags? How about two separate functions instead?