Bug 226730

Summary: Convert WebCore::SnapshotOptions into an enum class
Product: WebKit Reporter: Devin Rousso <hi>
Component: New BugsAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, hi, joepeck, megan_gardner, sam, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=225942
Bug Depends on:    
Bug Blocks: 226756    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch none

Description Devin Rousso 2021-06-07 08:36:11 PDT 
.
Comment 1 Devin Rousso 2021-06-07 08:41:28 PDT 
Created attachment 430749 [details]
Patch
Comment 2 Wenson Hsieh 2021-06-07 08:45:34 PDT 
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`).
Comment 3 Devin Rousso 2021-06-07 08:52:29 PDT 
Created attachment 430751 [details]
Patch
Comment 4 Devin Rousso 2021-06-07 09:17:16 PDT 
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 :/
Comment 5 Wenson Hsieh 2021-06-07 09:17:59 PDT 
(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?
Comment 6 Devin Rousso 2021-06-07 09:29:10 PDT 
Created attachment 430755 [details]
Patch
Comment 7 EWS 2021-06-07 11:54:02 PDT 
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].
Comment 8 Radar WebKit Bug Importer 2021-06-07 11:55:15 PDT 
<rdar://problem/78956259>
Comment 9 Sam Weinig 2021-06-07 12:35:18 PDT 
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?
Comment 10 Devin Rousso 2021-06-07 22:04:41 PDT 
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`)