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.
(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?
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
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
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
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
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
(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 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
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
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
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
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
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 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
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 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
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.
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().
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
2018-06-25 09:44 PDT, Said Abou-Hallawa
2018-06-25 10:14 PDT, Said Abou-Hallawa
2018-06-25 10:34 PDT, Said Abou-Hallawa
2018-06-25 10:48 PDT, Said Abou-Hallawa
2018-06-27 14:34 PDT, Said Abou-Hallawa
2018-06-27 15:14 PDT, Said Abou-Hallawa
2018-06-27 15:48 PDT, Said Abou-Hallawa
2018-06-27 17:02 PDT, EWS Watchlist
2018-06-27 17:05 PDT, EWS Watchlist
2018-06-27 17:29 PDT, EWS Watchlist
2018-06-27 17:41 PDT, EWS Watchlist
2018-06-27 20:31 PDT, EWS Watchlist
2018-07-13 20:22 PDT, Said Abou-Hallawa
2018-07-13 20:59 PDT, Said Abou-Hallawa
2018-07-13 21:58 PDT, EWS Watchlist
2018-07-13 22:12 PDT, EWS Watchlist
2018-07-13 22:51 PDT, EWS Watchlist
2018-07-13 22:54 PDT, EWS Watchlist
2018-07-14 08:50 PDT, EWS Watchlist
2018-07-14 09:33 PDT, EWS Watchlist
2018-07-17 10:41 PDT, Said Abou-Hallawa
2018-07-17 10:47 PDT, Said Abou-Hallawa
2018-07-17 11:22 PDT, Said Abou-Hallawa
2018-07-17 11:45 PDT, Said Abou-Hallawa
2018-07-17 12:50 PDT, EWS Watchlist
2018-07-17 13:47 PDT, EWS Watchlist
2018-07-17 14:01 PDT, Said Abou-Hallawa
2018-07-17 14:08 PDT, Said Abou-Hallawa
2018-07-17 15:25 PDT, EWS Watchlist
2018-07-17 15:26 PDT, Said Abou-Hallawa
2018-07-25 18:28 PDT, Said Abou-Hallawa
2018-07-26 14:37 PDT, EWS Watchlist