Bug 152956

Summary: Make a way to test display-list drawing
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Tools / TestsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, lforschler, mmaxfield, rniwa, simon.fraser, thorton, zalan
Priority: P2    
Version: Safari 9   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 152808    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews102 for mac-yosemite
none
Archive of layout-test-results from ews114 for mac-yosemite
none
Patch
none
Patch darin: review+

Description Simon Fraser (smfr) 2016-01-09 22:42:33 PST
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?)
Comment 1 Simon Fraser (smfr) 2016-01-10 21:52:33 PST
Created attachment 268671 [details]
Patch
Comment 2 Simon Fraser (smfr) 2016-01-10 21:53:14 PST
This will probably fail; we need to move the recording and replay code into GraphicsLayer so all platforms can use it.
Comment 3 Simon Fraser (smfr) 2016-01-10 21:53:34 PST
^ fail on GTK and EFL
Comment 4 Build Bot 2016-01-10 22:46:48 PST
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
Comment 5 Build Bot 2016-01-10 22:46:51 PST
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
Comment 6 Build Bot 2016-01-10 22:49:07 PST
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
Comment 7 Build Bot 2016-01-10 22:49:09 PST
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
Comment 8 Ryosuke Niwa 2016-01-11 18:48:47 PST
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.
Comment 9 Simon Fraser (smfr) 2016-01-15 14:38:53 PST
Created attachment 269102 [details]
Patch
Comment 10 WebKit Commit Bot 2016-01-15 14:40:53 PST
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.
Comment 11 Simon Fraser (smfr) 2016-01-15 15:14:09 PST
Created attachment 269109 [details]
Patch
Comment 12 WebKit Commit Bot 2016-01-15 15:15:22 PST
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.
Comment 13 Simon Fraser (smfr) 2016-01-15 15:58:18 PST
https://trac.webkit.org/r195156
Comment 14 Darin Adler 2016-01-19 23:02:50 PST
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?