- 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);
