Title: [275587] trunk
Revision
275587
Author
mmaxfi...@apple.com
Date
2021-04-06 22:56:24 -0700 (Tue, 06 Apr 2021)

Log Message

[GPU Process] Simplify DisplayList::Iterator part 5: Tweak the return type of DisplayList::Iterator::operator*()
https://bugs.webkit.org/show_bug.cgi?id=224148

Reviewed by Wenson Hsieh.

Source/WebCore:

This patch migrates from

struct Value {
    Optional<ItemHandle> item;
    Optional<FloatRect> extent;
    size_t itemSizeInBuffer { 0 };
};
Value operator*() const;

to

struct Value {
    ItemHandle item;
    Optional<FloatRect> extent;
    size_t itemSizeInBuffer { 0 };
};
Optional<Value> operator*() const

There are two reasons for this:
1. Philosophically, if the item is nullopt, then all the stuff in the Value is also meaningless
2. Part of the iterator's API contract is that if the item is nullopt, you're not allowed to keep
       iterating - doing this will lead to an infinite loop. Promoting the optional makes it more
       likely that this API contract is followed in the future.

No new tests because there is no behavior change.

* platform/graphics/displaylists/DisplayList.cpp:
(WebCore::DisplayList::DisplayList::asText const):
(WebCore::DisplayList::DisplayList::dump const):
* platform/graphics/displaylists/DisplayList.h:
(WebCore::DisplayList::DisplayList::iterator::operator* const):
* platform/graphics/displaylists/DisplayListReplayer.cpp:
(WebCore::DisplayList::Replayer::replay):

Tools:

* TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp:
(TestWebKitAPI::TEST):

Modified Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (275586 => 275587)


--- trunk/Source/WebCore/ChangeLog	2021-04-07 04:44:02 UTC (rev 275586)
+++ trunk/Source/WebCore/ChangeLog	2021-04-07 05:56:24 UTC (rev 275587)
@@ -1,3 +1,44 @@
+2021-04-06  Myles C. Maxfield  <mmaxfi...@apple.com>
+
+        [GPU Process] Simplify DisplayList::Iterator part 5: Tweak the return type of DisplayList::Iterator::operator*()
+        https://bugs.webkit.org/show_bug.cgi?id=224148
+
+        Reviewed by Wenson Hsieh.
+
+        This patch migrates from
+
+        struct Value {
+            Optional<ItemHandle> item;
+            Optional<FloatRect> extent;
+            size_t itemSizeInBuffer { 0 };
+        };
+        Value operator*() const;
+
+        to
+
+        struct Value {
+            ItemHandle item;
+            Optional<FloatRect> extent;
+            size_t itemSizeInBuffer { 0 };
+        };
+        Optional<Value> operator*() const
+
+        There are two reasons for this:
+        1. Philosophically, if the item is nullopt, then all the stuff in the Value is also meaningless
+        2. Part of the iterator's API contract is that if the item is nullopt, you're not allowed to keep
+               iterating - doing this will lead to an infinite loop. Promoting the optional makes it more
+               likely that this API contract is followed in the future.
+
+        No new tests because there is no behavior change.
+
+        * platform/graphics/displaylists/DisplayList.cpp:
+        (WebCore::DisplayList::DisplayList::asText const):
+        (WebCore::DisplayList::DisplayList::dump const):
+        * platform/graphics/displaylists/DisplayList.h:
+        (WebCore::DisplayList::DisplayList::iterator::operator* const):
+        * platform/graphics/displaylists/DisplayListReplayer.cpp:
+        (WebCore::DisplayList::Replayer::replay):
+
 2021-04-06  Zalan Bujtas  <za...@apple.com>
 
         [LFC][IFC] InlineFormattingState::shrinkToFit should shrink InlineItems too

Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.cpp (275586 => 275587)


--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.cpp	2021-04-07 04:44:02 UTC (rev 275586)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.cpp	2021-04-07 05:56:24 UTC (rev 275587)
@@ -114,13 +114,14 @@
 String DisplayList::asText(AsTextFlags flags) const
 {
     TextStream stream(TextStream::LineMode::MultipleLine, TextStream::Formatting::SVGStyleRect);
-    for (auto [item, extent, itemSizeInBuffer] : *this) {
-        if (!shouldDumpForFlags(flags, *item))
+    for (auto displayListItem : *this) {
+        auto [item, extent, itemSizeInBuffer] = displayListItem.value();
+        if (!shouldDumpForFlags(flags, item))
             continue;
 
         TextStream::GroupScope group(stream);
         stream << item;
-        if (item->isDrawingItem())
+        if (item.isDrawingItem())
             stream << " extent " << extent;
     }
     return stream.release();
@@ -131,10 +132,11 @@
     TextStream::GroupScope group(ts);
     ts << "display list";
 
-    for (auto [item, extent, itemSizeInBuffer] : *this) {
+    for (auto displayListItem : *this) {
+        auto [item, extent, itemSizeInBuffer] = displayListItem.value();
         TextStream::GroupScope group(ts);
         ts << item;
-        if (item->isDrawingItem())
+        if (item.isDrawingItem())
             ts << " extent " << extent;
     }
     ts.startGroup();

Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.h (275586 => 275587)


--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.h	2021-04-07 04:44:02 UTC (rev 275586)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.h	2021-04-07 05:56:24 UTC (rev 275587)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2021 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -118,18 +118,20 @@
         void operator++() { advance(); }
 
         struct Value {
-            Optional<ItemHandle> item;
+            ItemHandle item;
             Optional<FloatRect> extent;
             size_t itemSizeInBuffer { 0 };
         };
 
-        Value operator*() const
+        Optional<Value> operator*() const
         {
-            return {
-                m_isValid ? makeOptional(ItemHandle { m_currentBufferForItem }) : WTF::nullopt,
+            if (!m_isValid)
+                return WTF::nullopt;
+            return {{
+                ItemHandle { m_currentBufferForItem },
                 m_currentExtent,
                 m_currentItemSizeInBuffer,
-            };
+            }};
         }
 
     private:

Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp (275586 => 275587)


--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp	2021-04-07 04:44:02 UTC (rev 275586)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp	2021-04-07 05:56:24 UTC (rev 275587)
@@ -188,7 +188,14 @@
     size_t i = 0;
 #endif
     ReplayResult result;
-    for (auto [item, extent, itemSizeInBuffer] : m_displayList) {
+    for (auto displayListItem : m_displayList) {
+        if (!displayListItem) {
+            result.reasonForStopping = StopReplayReason::InvalidItemOrExtent;
+            break;
+        }
+
+        auto [item, extent, itemSizeInBuffer] = displayListItem.value();
+
         if (!initialClip.isZero() && extent && !extent->intersects(initialClip)) {
             LOG_WITH_STREAM(DisplayLists, stream << "skipping " << i++ << " " << item);
             result.numberOfBytesRead += itemSizeInBuffer;
@@ -195,21 +202,16 @@
             continue;
         }
 
-        if (!item) {
-            result.reasonForStopping = StopReplayReason::InvalidItemOrExtent;
-            break;
-        }
-
         LOG_WITH_STREAM(DisplayLists, stream << "applying " << i++ << " " << item);
 
-        if (item->is<MetaCommandChangeDestinationImageBuffer>()) {
+        if (item.is<MetaCommandChangeDestinationImageBuffer>()) {
             result.numberOfBytesRead += itemSizeInBuffer;
             result.reasonForStopping = StopReplayReason::ChangeDestinationImageBuffer;
-            result.nextDestinationImageBuffer = item->get<MetaCommandChangeDestinationImageBuffer>().identifier();
+            result.nextDestinationImageBuffer = item.get<MetaCommandChangeDestinationImageBuffer>().identifier();
             break;
         }
 
-        if (auto [reasonForStopping, missingCachedResourceIdentifier] = applyItem(*item); reasonForStopping) {
+        if (auto [reasonForStopping, missingCachedResourceIdentifier] = applyItem(item); reasonForStopping) {
             result.reasonForStopping = *reasonForStopping;
             result.missingCachedResourceIdentifier = WTFMove(missingCachedResourceIdentifier);
             break;
@@ -218,8 +220,8 @@
         result.numberOfBytesRead += itemSizeInBuffer;
 
         if (UNLIKELY(trackReplayList)) {
-            replayList->append(*item);
-            if (item->isDrawingItem())
+            replayList->append(item);
+            if (item.isDrawingItem())
                 replayList->addDrawingItemExtent(WTFMove(extent));
         }
     }

Modified: trunk/Source/WebCore/platform/graphics/displaylists/InMemoryDisplayList.cpp (275586 => 275587)


--- trunk/Source/WebCore/platform/graphics/displaylists/InMemoryDisplayList.cpp	2021-04-07 04:44:02 UTC (rev 275586)
+++ trunk/Source/WebCore/platform/graphics/displaylists/InMemoryDisplayList.cpp	2021-04-07 05:56:24 UTC (rev 275587)
@@ -61,11 +61,12 @@
 InMemoryDisplayList::~InMemoryDisplayList()
 {
     auto end = this->end();
-    for (auto [item, extent, size] : *this) {
+    for (auto displayListItem : *this) {
+        auto item = displayListItem->item;
         ASSERT(item);
         if (!item)
             break;
-        item->destroy();
+        item.destroy();
     }
 }
 

Modified: trunk/Tools/ChangeLog (275586 => 275587)


--- trunk/Tools/ChangeLog	2021-04-07 04:44:02 UTC (rev 275586)
+++ trunk/Tools/ChangeLog	2021-04-07 05:56:24 UTC (rev 275587)
@@ -1,3 +1,13 @@
+2021-04-06  Myles C. Maxfield  <mmaxfi...@apple.com>
+
+        [GPU Process] Simplify DisplayList::Iterator part 5: Tweak the return type of DisplayList::Iterator::operator*()
+        https://bugs.webkit.org/show_bug.cgi?id=224148
+
+        Reviewed by Wenson Hsieh.
+
+        * TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp:
+        (TestWebKitAPI::TEST):
+
 2021-04-06  Jean-Yves Avenard  <j...@apple.com>
 
         Add j...@apple.com as committer.

Modified: trunk/Tools/TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp (275586 => 275587)


--- trunk/Tools/TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp	2021-04-07 04:44:02 UTC (rev 275586)
+++ trunk/Tools/TestWebKitAPI/Tests/WebCore/DisplayListTests.cpp	2021-04-07 05:56:24 UTC (rev 275587)
@@ -79,42 +79,43 @@
     EXPECT_FALSE(list.isEmpty());
 
     bool observedUnexpectedItem = false;
-    for (auto [handle, extent, size] : list) {
-        switch (handle->type()) {
+    for (auto displayListItem : list) {
+        auto handle = displayListItem->item;
+        switch (handle.type()) {
         case ItemType::SetStrokeThickness: {
-            EXPECT_FALSE(handle->isDrawingItem());
-            EXPECT_TRUE(handle->is<SetStrokeThickness>());
-            auto& item = handle->get<SetStrokeThickness>();
+            EXPECT_FALSE(handle.isDrawingItem());
+            EXPECT_TRUE(handle.is<SetStrokeThickness>());
+            auto& item = handle.get<SetStrokeThickness>();
             EXPECT_EQ(item.thickness(), 1.5);
             break;
         }
         case ItemType::FillPath: {
-            EXPECT_TRUE(handle->isDrawingItem());
-            EXPECT_TRUE(handle->is<FillPath>());
-            auto& item = handle->get<FillPath>();
+            EXPECT_TRUE(handle.isDrawingItem());
+            EXPECT_TRUE(handle.is<FillPath>());
+            auto& item = handle.get<FillPath>();
             EXPECT_EQ(item.path().platformPath(), path.platformPath());
             break;
         }
         case ItemType::FillRectWithGradient: {
-            EXPECT_TRUE(handle->isDrawingItem());
-            EXPECT_TRUE(handle->is<FillRectWithGradient>());
-            auto& item = handle->get<FillRectWithGradient>();
+            EXPECT_TRUE(handle.isDrawingItem());
+            EXPECT_TRUE(handle.is<FillRectWithGradient>());
+            auto& item = handle.get<FillRectWithGradient>();
             EXPECT_EQ(item.rect(), FloatRect(1., 1., 10., 10.));
             EXPECT_EQ(&item.gradient(), gradient.ptr());
             break;
         }
         case ItemType::SetInlineFillColor: {
-            EXPECT_FALSE(handle->isDrawingItem());
-            EXPECT_TRUE(handle->is<SetInlineFillColor>());
-            auto& item = handle->get<SetInlineFillColor>();
+            EXPECT_FALSE(handle.isDrawingItem());
+            EXPECT_TRUE(handle.is<SetInlineFillColor>());
+            auto& item = handle.get<SetInlineFillColor>();
             EXPECT_EQ(item.color(), Color::red);
             break;
         }
 #if ENABLE(INLINE_PATH_DATA)
         case ItemType::StrokeInlinePath: {
-            EXPECT_TRUE(handle->isDrawingItem());
-            EXPECT_TRUE(handle->is<StrokeInlinePath>());
-            auto& item = handle->get<StrokeInlinePath>();
+            EXPECT_TRUE(handle.isDrawingItem());
+            EXPECT_TRUE(handle.is<StrokeInlinePath>());
+            auto& item = handle.get<StrokeInlinePath>();
             const auto path = item.path();
             auto& line = path.inlineData<LineData>();
             EXPECT_EQ(line.start, FloatPoint(0, 0));
@@ -148,11 +149,12 @@
 
     list.append<FillRectWithColor>(FloatRect { 0, 0, 100, 100 }, Color::black);
 
-    for (auto [handle, extent, size] : list) {
-        EXPECT_EQ(handle->type(), ItemType::FillRectWithColor);
-        EXPECT_TRUE(handle->is<FillRectWithColor>());
+    for (auto displayListItem : list) {
+        auto handle = displayListItem->item;
+        EXPECT_EQ(handle.type(), ItemType::FillRectWithColor);
+        EXPECT_TRUE(handle.is<FillRectWithColor>());
 
-        auto& item = handle->get<FillRectWithColor>();
+        auto& item = handle.get<FillRectWithColor>();
         EXPECT_EQ(*item.color().tryGetAsSRGBABytes(), Color::black);
         EXPECT_EQ(item.rect(), FloatRect(0, 0, 100, 100));
     }
@@ -224,8 +226,8 @@
     shallowCopy.setItemBufferReadingClient(&reader);
 
     Vector<ItemType> itemTypes;
-    for (auto [handle, extent, size] : shallowCopy)
-        itemTypes.append(handle->type());
+    for (auto displayListItem : shallowCopy)
+        itemTypes.append(displayListItem->item.type());
 
     EXPECT_FALSE(shallowCopy.isEmpty());
     EXPECT_EQ(itemTypes.size(), 4U);
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to