Changeset 278614 in webkit

Timestamp:

Jun 8, 2021, 9:53:06 AM (5 years ago)

Author:

Devin Rousso

Message:

Require that callsites of SnapshotOptions specify a PixelFormat and DestinationColorSpace
​https://bugs.webkit.org/show_bug.cgi?id=226756

Reviewed by Sam Weinig.

Don't wrap PixelFormat or DestinationColorSpace with std::optional as we want each
callsite to explicity configure them. This makes it easier to find where values for each
are used as there's no implicit behavior.

No behavior change. Followup after r278565.

Source/WebCore:

• page/FrameSnapshotting.h:
• page/FrameSnapshotting.cpp:

(WebCore::snapshotFrameRect):
(WebCore::snapshotFrameRectWithClip):
(WebCore::snapshotNode):
(WebCore::snapshotSelection):

• inspector/agents/InspectorPageAgent.cpp:

(WebCore::InspectorPageAgent::snapshotNode):
(WebCore::InspectorPageAgent::snapshotRect):

• page/PageConsoleClient.cpp:

(WebCore::PageConsoleClient::screenshot):

• page/TextIndicator.cpp:

(WebCore::snapshotOptionsForTextIndicatorOptions):
(WebCore::takeSnapshots):

• platform/DragImage.cpp:

(WebCore::createDragImageForNode):
(WebCore::createDragImageForSelection):
(WebCore::createDragImageForRange):
(WebCore::createDragImageForImage):

Source/WebKit:

• WebProcess/WebPage/WebFrame.cpp:

(WebKit::WebFrame::createSelectionSnapshot const):

Location:

trunk/Source

Files:

• WebCore/ChangeLog (modified) (1 diff)
• WebCore/inspector/agents/InspectorPageAgent.cpp (modified) (2 diffs)
• WebCore/page/FrameSnapshotting.cpp (modified) (3 diffs)
• WebCore/page/FrameSnapshotting.h (modified) (3 diffs)
• WebCore/page/PageConsoleClient.cpp (modified) (2 diffs)
• WebCore/page/TextIndicator.cpp (modified) (3 diffs)
• WebCore/platform/DragImage.cpp (modified) (4 diffs)
• WebKit/ChangeLog (modified) (1 diff)
• WebKit/WebProcess/WebPage/WebFrame.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-06-08  Devin Rousso  <drousso@apple.com>
+
+        Require that callsites of `SnapshotOptions` specify a `PixelFormat` and `DestinationColorSpace`
+        https://bugs.webkit.org/show_bug.cgi?id=226756
+
+        Reviewed by Sam Weinig.
+
+        Don't wrap `PixelFormat` or `DestinationColorSpace` with `std::optional` as we want each
+        callsite to explicity configure them. This makes it easier to find where values for each
+        are used as there's no implicit behavior.
+
+        No behavior change. Followup after r278565.
+
+        * page/FrameSnapshotting.h:
+        * page/FrameSnapshotting.cpp:
+        (WebCore::snapshotFrameRect):
+        (WebCore::snapshotFrameRectWithClip):
+        (WebCore::snapshotNode):
+        (WebCore::snapshotSelection):
+
+        * inspector/agents/InspectorPageAgent.cpp:
+        (WebCore::InspectorPageAgent::snapshotNode):
+        (WebCore::InspectorPageAgent::snapshotRect):
+        * page/PageConsoleClient.cpp:
+        (WebCore::PageConsoleClient::screenshot):
+        * page/TextIndicator.cpp:
+        (WebCore::snapshotOptionsForTextIndicatorOptions):
+        (WebCore::takeSnapshots):
+        * platform/DragImage.cpp:
+        (WebCore::createDragImageForNode):
+        (WebCore::createDragImageForSelection):
+        (WebCore::createDragImageForRange):
+        (WebCore::createDragImageForImage):
+
 2021-06-08  Sam Weinig  <weinig@apple.com>
 

trunk/Source/WebCore/inspector/agents/InspectorPageAgent.cpp

@@ -1073 +1073 @@
         return makeUnexpected(errorString);
 
-    auto snapshot = WebCore::snapshotNode(m_inspectedPage.mainFrame(), *node);
+    auto snapshot = WebCore::snapshotNode(m_inspectedPage.mainFrame(), *node, { { }, PixelFormat::BGRA8, DestinationColorSpace::SRGB() });
     if (!snapshot)
         return makeUnexpected("Could not capture snapshot"_s);
@@ -1082 +1082 @@
 Protocol::ErrorStringOr<String> InspectorPageAgent::snapshotRect(int x, int y, int width, int height, Protocol::Page::CoordinateSystem coordinateSystem)
 {
-    SnapshotOptions options;
+    SnapshotOptions options { { }, PixelFormat::BGRA8, DestinationColorSpace::SRGB() };
     if (coordinateSystem == Protocol::Page::CoordinateSystem::Viewport)
         options.flags.add(SnapshotFlags::InViewCoordinates);

trunk/Source/WebCore/page/FrameSnapshotting.cpp

@@ -32 +32 @@
 #include "FrameSnapshotting.h"
 
-#include "DestinationColorSpace.h"
 #include "Document.h"
 #include "FloatRect.h"
@@ -41 +40 @@
 #include "ImageBuffer.h"
 #include "Page.h"
-#include "PixelFormat.h"
 #include "RenderObject.h"
 #include "Settings.h"
@@ -115 +113 @@
         scaleFactor = ceilf(scaleFactor);
 
-    auto buffer = ImageBuffer::create(imageRect.size(), RenderingMode::Unaccelerated, scaleFactor, options.colorSpace.value_or(DestinationColorSpace::SRGB()), options.pixelFormat.value_or(PixelFormat::BGRA8));
+    auto buffer = ImageBuffer::create(imageRect.size(), RenderingMode::Unaccelerated, scaleFactor, options.colorSpace, options.pixelFormat);
     if (!buffer)
         return nullptr;

trunk/Source/WebCore/page/FrameSnapshotting.h

@@ -30 +30 @@
 #pragma once
 
+#include "DestinationColorSpace.h"
+#include "PixelFormat.h"
 #include <memory>
-#include <optional>
 #include <wtf/Forward.h>
 
 namespace WebCore {
 
-class DestinationColorSpace;
 class FloatRect;
 class Frame;
@@ -42 +42 @@
 class ImageBuffer;
 class Node;
-enum class PixelFormat : uint8_t;
 
 enum class SnapshotFlags : uint8_t {
@@ -55 +54 @@
 
 struct SnapshotOptions {
-    OptionSet<SnapshotFlags> flags { };
-    std::optional<PixelFormat> pixelFormat { };
-    std::optional<DestinationColorSpace> colorSpace { };
+    OptionSet<SnapshotFlags> flags;
+    PixelFormat pixelFormat;
+    DestinationColorSpace colorSpace;
 };
 
-WEBCORE_EXPORT RefPtr<ImageBuffer> snapshotFrameRect(Frame&, const IntRect&, SnapshotOptions&& = { });
-RefPtr<ImageBuffer> snapshotFrameRectWithClip(Frame&, const IntRect&, const Vector<FloatRect>& clipRects, SnapshotOptions&& = { });
-RefPtr<ImageBuffer> snapshotNode(Frame&, Node&, SnapshotOptions&& = { });
-WEBCORE_EXPORT RefPtr<ImageBuffer> snapshotSelection(Frame&, SnapshotOptions&& = { });
+WEBCORE_EXPORT RefPtr<ImageBuffer> snapshotFrameRect(Frame&, const IntRect&, SnapshotOptions&&);
+RefPtr<ImageBuffer> snapshotFrameRectWithClip(Frame&, const IntRect&, const Vector<FloatRect>& clipRects, SnapshotOptions&&);
+RefPtr<ImageBuffer> snapshotNode(Frame&, Node&, SnapshotOptions&&);
+WEBCORE_EXPORT RefPtr<ImageBuffer> snapshotSelection(Frame&, SnapshotOptions&&);
 
 } // namespace WebCore

trunk/Source/WebCore/page/PageConsoleClient.cpp

@@ -359 +359 @@
                 if (dataURL.isEmpty()) {
                     if (!snapshot)
-                        snapshot = WebCore::snapshotNode(m_page.mainFrame(), *node);
+                        snapshot = WebCore::snapshotNode(m_page.mainFrame(), *node, { { }, PixelFormat::BGRA8, DestinationColorSpace::SRGB() });
 
                     if (snapshot)
@@ -405 +405 @@
             // If no target is provided, capture an image of the viewport.
             IntRect imageRect(IntPoint::zero(), m_page.mainFrame().view()->sizeForVisibleContent());
-            if (auto snapshot = WebCore::snapshotFrameRect(m_page.mainFrame(), imageRect, { { SnapshotFlags::InViewCoordinates } }))
+            if (auto snapshot = WebCore::snapshotFrameRect(m_page.mainFrame(), imageRect, { { SnapshotFlags::InViewCoordinates }, PixelFormat::BGRA8, DestinationColorSpace::SRGB() }))
                 dataURL = snapshot->toDataURL("image/png"_s, std::nullopt, PreserveResolution::Yes);
         }

trunk/Source/WebCore/page/TextIndicator.cpp

@@ -128 +128 @@
 static SnapshotOptions snapshotOptionsForTextIndicatorOptions(OptionSet<TextIndicatorOption> options)
 {
-    SnapshotOptions snapshotOptions;
-    snapshotOptions.flags.add(SnapshotFlags::PaintWithIntegralScaleFactor);
+    SnapshotOptions snapshotOptions { { SnapshotFlags::PaintWithIntegralScaleFactor }, PixelFormat::BGRA8, DestinationColorSpace::SRGB() };
 
     if (!options.contains(TextIndicatorOption::PaintAllContent)) {
@@ -163 +162 @@
     if (data.options.contains(TextIndicatorOption::IncludeSnapshotWithSelectionHighlight)) {
         float snapshotScaleFactor;
-        data.contentImageWithHighlight = takeSnapshot(frame, snapshotRect, { }, snapshotScaleFactor, clipRectsInDocumentCoordinates);
+        data.contentImageWithHighlight = takeSnapshot(frame, snapshotRect, { { }, PixelFormat::BGRA8, DestinationColorSpace::SRGB() }, snapshotScaleFactor, clipRectsInDocumentCoordinates);
         ASSERT(!data.contentImageWithHighlight || data.contentImageScaleFactor >= snapshotScaleFactor);
     }
@@ -170 +169 @@
         float snapshotScaleFactor;
         auto snapshotRect = frame.view()->visibleContentRect();
-        data.contentImageWithoutSelection = takeSnapshot(frame, snapshotRect, { { SnapshotFlags::PaintEverythingExcludingSelection } }, snapshotScaleFactor, { });
+        data.contentImageWithoutSelection = takeSnapshot(frame, snapshotRect, { { SnapshotFlags::PaintEverythingExcludingSelection }, PixelFormat::BGRA8, DestinationColorSpace::SRGB() }, snapshotScaleFactor, { });
         data.contentImageWithoutSelectionRectInRootViewCoordinates = frame.view()->contentsToRootView(snapshotRect);
     }

trunk/Source/WebCore/platform/DragImage.cpp

@@ -120 +120 @@
 {
     ScopedNodeDragEnabler enableDrag(frame, node);
-    return createDragImageFromSnapshot(snapshotNode(frame, node), &node);
+    return createDragImageFromSnapshot(snapshotNode(frame, node, { { }, PixelFormat::BGRA8, DestinationColorSpace::SRGB() }), &node);
 }
 
@@ -127 +127 @@
 DragImageRef createDragImageForSelection(Frame& frame, TextIndicatorData&, bool forceBlackText)
 {
-    SnapshotOptions options;
+    SnapshotOptions options { { }, PixelFormat::BGRA8, DestinationColorSpace::SRGB() };
     if (forceBlackText)
         options.flags.add(SnapshotFlags::ForceBlackText);
@@ -185 +185 @@
         return nullptr;
 
-    SnapshotOptions options;
-    options.flags.add(SnapshotFlags::PaintSelectionOnly);
+    SnapshotOptions options { { SnapshotFlags::PaintSelectionOnly }, PixelFormat::BGRA8, DestinationColorSpace::SRGB() };
     if (forceBlackText)
         options.flags.add(SnapshotFlags::ForceBlackText);
@@ -219 +218 @@
     imageRect = paintingRect;
 
-    return createDragImageFromSnapshot(snapshotNode(frame, node), &node);
+    return createDragImageFromSnapshot(snapshotNode(frame, node, { { }, PixelFormat::BGRA8, DestinationColorSpace::SRGB() }), &node);
 }
 

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2021-06-08  Devin Rousso  <drousso@apple.com>
+
+        Require that callsites of `SnapshotOptions` specify a `PixelFormat` and `DestinationColorSpace`
+        https://bugs.webkit.org/show_bug.cgi?id=226756
+
+        Reviewed by Sam Weinig.
+
+        Don't wrap `PixelFormat` or `DestinationColorSpace` with `std::optional` as we want each
+        callsite to explicity configure them. This makes it easier to find where values for each
+        are used as there's no implicit behavior.
+
+        No behavior change. Followup after r278565.
+
+        * WebProcess/WebPage/WebFrame.cpp:
+        (WebKit::WebFrame::createSelectionSnapshot const):
+
 2021-06-08  Jean-Yves Avenard  <jya@apple.com>
 

trunk/Source/WebKit/WebProcess/WebPage/WebFrame.cpp

@@ -843 +843 @@
 RefPtr<ShareableBitmap> WebFrame::createSelectionSnapshot() const
 {
-    auto snapshot = snapshotSelection(*coreFrame(), { { WebCore::SnapshotFlags::ForceBlackText } });
+    auto snapshot = snapshotSelection(*coreFrame(), { { WebCore::SnapshotFlags::ForceBlackText }, PixelFormat::BGRA8, DestinationColorSpace::SRGB() });
     if (!snapshot)
         return nullptr;