Make it easier to add new DisplayList item and make it more mechanical to record and replay back the DisplayListItems.
Created attachment 343504 [details] Patch
Created attachment 343507 [details] Patch
Created attachment 343512 [details] Patch
Created attachment 343513 [details] Patch
Comment on attachment 343513 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343513&action=review I'm not convinced this is worthwhile. If we frequently added new drawing items it could be, but the list is pretty stable. > LayoutTests/displaylists/extent-includes-shadow-expected.txt:4 > - (x 0.00) > - (y 0.00)) > + (arguments(0) 0.00) > + (arguments(1) 0.00)) This seems like a distinct downside of this approach: you can't easily identify the properties in dumps any more.
Comment on attachment 343513 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343513&action=review I don't think the list of ListDisplayItems is fixed. In the patch https://bugs.webkit.org/attachment.cgi?id=335043&action=review, I add seven new items. To get the form controls working, we will need to add more items. Please also notice that <https://trac.webkit.org/changeset/229672>, a new recorder and new set of DisplayListItems were added to the Cairo port. A similar but more verbose approach was taken by this patch. The dump() functions in this patch, don't even print the arguments. They just print the function name. >> LayoutTests/displaylists/extent-includes-shadow-expected.txt:4 >> + (arguments(1) 0.00)) > > This seems like a distinct downside of this approach: you can't easily identify the properties in dumps any more. This can be fixed by passing the parameters' names as template parameters along with the types. I just did not do because I felt it is not worthy. But I can do it if this will make this patch worthy.
Created attachment 343754 [details] Patch
Created attachment 343758 [details] Patch
Created attachment 343762 [details] Patch
(In reply to Said Abou-Hallawa from comment #8) > Created attachment 343758 [details] > Patch When you are in a lldb session, does this change make debugging/printing variables more difficult?
Comment on attachment 343762 [details] Patch Attachment 343762 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8364123 New failing tests: inspector/canvas/recording-webgl.html displaylists/extent-includes-shadow.html displaylists/extent-includes-transforms.html
Created attachment 343772 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 343762 [details] Patch Attachment 343762 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8364237 Number of test failures exceeded the failure limit.
Created attachment 343773 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 343762 [details] Patch Attachment 343762 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8364109 Number of test failures exceeded the failure limit.
Created attachment 343777 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 343762 [details] Patch Attachment 343762 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8364342 New failing tests: fast/events/dropzone-001.html displaylists/extent-includes-shadow.html fast/events/drag-and-drop.html displaylists/extent-includes-transforms.html legacy-animation-engine/animations/play-state.html
Created attachment 343778 [details] Archive of layout-test-results from ews116 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 343762 [details] Patch Attachment 343762 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8365652 New failing tests: http/tests/preload/onload_event.html
Created attachment 343787 [details] Archive of layout-test-results from ews205 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
I still don't think this is a useful direction to go in. You fixed logging these classes, but debugging them is still going to be painful.
(In reply to Simon Fraser (smfr) from comment #21) > I still don't think this is a useful direction to go in. You fixed logging > these classes, but debugging them is still going to be painful. I don't think this is true unless I misunderstood the purpose of these classes or I misunderstood what you meant by "painful debugging". My understanding is these classes have very short life cycle and have very specific purpose. They are meant to pack some arguments when recording and unpack them when replaying back. I think the most useful way to debug them is through dumping the whole DisplayList. This is where, optimization opportunities could be found. Although I do not expect someone will debug inside these classes, you can still step inside the functions and set breakpoints as you wish. The only difference is there is one create() function and one apply() function in which we dispatch the call to GraphicsContext. If this is not what you want, you can set the breakpoint in DisplayListRecorder API, which are the creators of these classes. And you can set the breakpoint in the GraphicsContext functions which will be called from the apply() method of these classes. Also you can watch the item's contents the same way as you do today. Here is the expansion of the FillRect item in the Xcode watch window: this WebCore::DisplayList::FillRect * 0x11d60ae70 0x000000011d60ae70 WebCore::DisplayList::FillRectBase WebCore::DisplayList::FillRectBase WebCore::DisplayList::GeometryItem WebCore::DisplayList::GeometryItem WebCore::DisplayList::Item WebCore::DisplayList::Item WTF::RefCounted<WebCore::DisplayList::Item> WTF::RefCounted<WebCore::DisplayList::Item> m_type WebCore::DisplayList::ItemType FillRect m_extent std::optional<WebCore::FloatRect> WebCore::DisplayList::GraphicsItem<void (WebCore::GraphicsContext::*)(const WebCore::FloatRect &), void (WebCore::GraphicsContext::*)(const WebCore::FloatRect &), WebCore::FloatRect> WebCore::DisplayList::GraphicsItem<void (WebCore::GraphicsContext::*)(const WebCore::FloatRect &), void (WebCore::GraphicsContext::*)(const WebCore::FloatRect &), WebCore::FloatRect> m_arguments std::__1::tuple<WebCore::FloatRect> size=1 [0] WebCore::FloatRect m_location WebCore::FloatPoint m_x float 10 m_y float 10 m_size WebCore::FloatSize m_width float 55 m_height float 50 There are three extra nodes' levels using the new code: two because of the multiple inheritance and one because of packing all the arguments in std::tuple. If this is what you considered to be "painful debugging" I can collapse the class hierarchy although I would consider this a little bit unclean design. Alternatively I could add an Xcode extension to view these classes the way we want. For me this change is useful and worthwhile. 1. It eliminates 1300 lines of code. The old code is 2400 lines and the new is 1100 lines (DisplayListItems.h and DisplayListItems.cpp). 2. It makes adding and changing an item easier just in one place. It makes all the dump strings in one place. 3. It makes the relationship between the items and GraphicsContext more strongly typed. The types of the arguments have to match the types of the GraphicsContext function.
(In reply to zalan from comment #10) > (In reply to Said Abou-Hallawa from comment #8) > > Created attachment 343758 [details] > > Patch > When you are in a lldb session, does this change make debugging/printing > variables more difficult? No. It should not.
Created attachment 345016 [details] Patch
Created attachment 345019 [details] Patch
Comment on attachment 345019 [details] Patch Attachment 345019 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/8533348 Number of test failures exceeded the failure limit.
Created attachment 345022 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 345019 [details] Patch Attachment 345019 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8533432 New failing tests: fast/events/drag-display-none-element.html inspector/canvas/recording-webgl.html displaylists/extent-includes-shadow.html http/tests/misc/acid3.html displaylists/extent-includes-transforms.html
Created attachment 345024 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 345019 [details] Patch Attachment 345019 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8533460 Number of test failures exceeded the failure limit.
Created attachment 345026 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 345019 [details] Patch Attachment 345019 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8533600 New failing tests: fast/events/dropzone-001.html displaylists/extent-includes-shadow.html fast/events/drag-and-drop.html displaylists/extent-includes-transforms.html legacy-animation-engine/animations/play-state.html
Created attachment 345027 [details] Archive of layout-test-results from ews113 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 345019 [details] Patch Attachment 345019 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8536639 New failing tests: editing/pasteboard/drag-link-with-data-transfer-adds-trusted-link-to-pasteboard.html
Created attachment 345035 [details] Archive of layout-test-results from ews204 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 345019 [details] Patch Attachment 345019 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8536815 New failing tests: http/tests/security/video-poster-cross-origin-crash2.html
Created attachment 345036 [details] Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 345166 [details] Patch
Created attachment 345168 [details] Patch
Created attachment 345172 [details] Patch
Created attachment 345175 [details] Patch
Comment on attachment 345175 [details] Patch Attachment 345175 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8565875 New failing tests: displaylists/replay-skip-clipped-rect.html displaylists/extent-includes-shadow.html displaylists/extent-includes-transforms.html displaylists/layer-dispay-list.html
Created attachment 345183 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 345175 [details] Patch Attachment 345175 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8566148 New failing tests: displaylists/replay-skip-clipped-rect.html displaylists/extent-includes-shadow.html displaylists/extent-includes-transforms.html displaylists/layer-dispay-list.html
Created attachment 345187 [details] Archive of layout-test-results from ews112 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 345190 [details] Patch
Created attachment 345191 [details] Patch
Comment on attachment 345191 [details] Patch Attachment 345191 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/8567242 New failing tests: displaylists/extent-includes-shadow.html displaylists/extent-includes-transforms.html
Created attachment 345196 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 345197 [details] Patch
<rdar://problem/42552571>
Comment on attachment 345197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345197&action=review > Source/WebCore/ChangeLog:24 > + -- GraphicsItem: It's a template class which takes a pointer to a GraphicsContext Given this is another base class, "GraphicsItem" is confusing because it makes it sound like Item is its superclass. > Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:181 > + void iterarte(Functor&& functor) const Do you mean iterate? If it's a naming conflict, a slightly more verbose name should be preferred.
Created attachment 345808 [details] Patch
Comment on attachment 345197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345197&action=review >> Source/WebCore/ChangeLog:24 >> + -- GraphicsItem: It's a template class which takes a pointer to a GraphicsContext > > Given this is another base class, "GraphicsItem" is confusing because it makes it sound like Item is its superclass. I renamed it to GraphicsApplier. >> Source/WebCore/platform/graphics/displaylists/DisplayListItems.h:181 >> + void iterarte(Functor&& functor) const > > Do you mean iterate? If it's a naming conflict, a slightly more verbose name should be preferred. Yes it is typo. I renamed it to iterateOverArguments().
Comment on attachment 345808 [details] Patch Attachment 345808 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8665116 New failing tests: http/tests/security/local-video-source-from-remote.html
Created attachment 345872 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit