Bug 187002 – Templatize DisplayListItems and use std::tuple to store the arguments

• Status: NEW
• Component: Layout and Rendering

[Said Abou-Hallawa]

Make it easier to add new DisplayList item and make it more mechanical to record and replay back the DisplayListItems.

[Said Abou-Hallawa]

Created attachment 343504 [details]
Patch

[Said Abou-Hallawa]

Created attachment 343507 [details]
Patch

[Said Abou-Hallawa]

Created attachment 343512 [details]
Patch

[Said Abou-Hallawa]

Created attachment 343513 [details]
Patch

[Simon Fraser (smfr)]

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.

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 343754 [details]
Patch

[Said Abou-Hallawa]

Created attachment 343758 [details]
Patch

[Said Abou-Hallawa]

Created attachment 343762 [details]
Patch

[zalan]

(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?

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Simon Fraser (smfr)]

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.

[Said Abou-Hallawa]

(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.

[Said Abou-Hallawa]

(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.

[Said Abou-Hallawa]

Created attachment 345016 [details]
Patch

[Said Abou-Hallawa]

Created attachment 345019 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Said Abou-Hallawa]

Created attachment 345166 [details]
Patch

[Said Abou-Hallawa]

Created attachment 345168 [details]
Patch

[Said Abou-Hallawa]

Created attachment 345172 [details]
Patch

[Said Abou-Hallawa]

Created attachment 345175 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Said Abou-Hallawa]

Created attachment 345190 [details]
Patch

[Said Abou-Hallawa]

Created attachment 345191 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Said Abou-Hallawa]

Created attachment 345197 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/42552571>

[Jon Lee]

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.

[Said Abou-Hallawa]

Created attachment 345808 [details]
Patch

[Said Abou-Hallawa]

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().

[EWS Watchlist]

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

[EWS Watchlist]

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