Bug 152956

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?