Summary: | Make a way to test display-list drawing | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||
Component: | Tools / Tests | Assignee: | 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
Simon Fraser (smfr)
2016-01-09 22:42:33 PST
Created attachment 268671 [details]
Patch
This will probably fail; we need to move the recording and replay code into GraphicsLayer so all platforms can use it. ^ fail on GTK and EFL 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 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 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 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 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. Created attachment 269102 [details]
Patch
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.
Created attachment 269109 [details]
Patch
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 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? |