Bug 226730 – Convert WebCore::SnapshotOptions into an enum class

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Devin Rousso]

.

[Devin Rousso]

Created attachment 430749 [details]
Patch

[Wenson Hsieh]

Comment on attachment 430749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430749&action=review

r=mews

> Source/WebCore/page/FrameSnapshotting.h:57
> +    OptionSet<SnapshotFlags> flags = { };

Nit - you don't need the `= { }` here.

> Source/WebCore/page/FrameSnapshotting.h:59
> +    std::optional<PixelFormat> pixelFormat = std::nullopt;
> +    std::optional<DestinationColorSpace> colorSpace = std::nullopt;

Ditto (don't need the `= std::nullopt`).

[Devin Rousso]

Created attachment 430751 [details]
Patch

[Devin Rousso]

Comment on attachment 430749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430749&action=review

>> Source/WebCore/page/FrameSnapshotting.h:59
>> +    std::optional<DestinationColorSpace> colorSpace = std::nullopt;
> 
> Ditto (don't need the `= std::nullopt`).

without these I seem to be getting errors related to missing field initializers :/

[Wenson Hsieh]

(In reply to Devin Rousso from comment #4)
> Comment on attachment 430749 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430749&action=review
> 
> >> Source/WebCore/page/FrameSnapshotting.h:59
> >> +    std::optional<DestinationColorSpace> colorSpace = std::nullopt;
> > 
> > Ditto (don't need the `= std::nullopt`).
> 
> without these I seem to be getting errors related to missing field
> initializers :/

I see. Maybe just remove the `=` then?

[Devin Rousso]

Created attachment 430755 [details]
Patch

[EWS]

Committed r278565 (238563@main): <https://commits.webkit.org/238563@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430755 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78956259>

[Sam Weinig]

Comment on attachment 430755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430755&action=review

> Source/WebCore/page/FrameSnapshotting.h:59
> +    std::optional<PixelFormat> pixelFormat { };
> +    std::optional<DestinationColorSpace> colorSpace { };

We've been trying to move to making these kind of parameters non-optional, as you really always need to specify them. What was the rationale here for making them optional?

[Devin Rousso]

Comment on attachment 430755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430755&action=review

>> Source/WebCore/page/FrameSnapshotting.h:59
>> +    std::optional<DestinationColorSpace> colorSpace { };
> 
> We've been trying to move to making these kind of parameters non-optional, as you really always need to specify them. What was the rationale here for making them optional?

Oh!  I was not aware of that.  I'll change these to be non-optional and specify them at callsites :)

<https://webkit.org/b/226756> (Require that callsites of `SnapshotOptions` specify a `DestinationColorSpace` and `PixelFormat`)