Bug 228875 - SandboxExtension::Handle creation should return std::optional instead of bool
Summary: SandboxExtension::Handle creation should return std::optional instead of bool
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks: 234975
  Show dependency treegraph
 
Reported: 2021-08-06 14:29 PDT by Alex Christensen
Modified: 2022-01-20 21:00 PST (History)
12 users (show)

See Also:


Attachments
Patch (82.04 KB, patch)
2021-08-06 14:31 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (81.74 KB, patch)
2021-08-12 15:21 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (81.83 KB, patch)
2021-08-12 17:52 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (81.76 KB, patch)
2021-08-13 13:08 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2021-08-06 14:29:42 PDT
SandboxExtension::Handle creation should return std::optional instead of bool
Comment 1 Alex Christensen 2021-08-06 14:31:07 PDT
Created attachment 435089 [details]
Patch
Comment 2 Sam Weinig 2021-08-07 11:28:56 PDT
Nice!
Comment 3 Chris Dumez 2021-08-09 08:49:35 PDT
EWS is all red.
Comment 4 Alex Christensen 2021-08-12 15:21:35 PDT
Created attachment 435452 [details]
Patch
Comment 5 Alex Christensen 2021-08-12 17:52:48 PDT
Created attachment 435460 [details]
Patch
Comment 6 youenn fablet 2021-08-13 00:01:04 PDT
Comment on attachment 435460 [details]
Patch

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

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:298
> +static SandboxExtension::HandleArray createHandlesForResources(const Vector<T>& resources, Function<std::optional<SandboxExtension::Handle>(const T&)>&& createFunction)

Does not seem we need a Function<>&& here.

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:319
> +    return createHandlesForResources(paths, Function<std::optional<Handle>(const String&)>([&logLabel] (const String& path) {

Would it work with:
return createHandlesForResources<String>(paths, [&logLabel](auto& path) { ... });

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:399
> +    return createHandlesForResources(services, Function<std::optional<Handle>(const ASCIILiteral&)>([auditToken, flags] (const ASCIILiteral& service) -> std::optional<Handle> {

Ditto here, createHandlesForResources<ASCIILiteral>(...) ?
Maybe below as well
Comment 7 Alex Christensen 2021-08-13 13:08:10 PDT
Created attachment 435502 [details]
Patch
Comment 8 EWS 2021-08-13 13:53:10 PDT
Committed r281032 (240512@main): <https://commits.webkit.org/240512@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 435502 [details].
Comment 9 Radar WebKit Bug Importer 2021-08-13 13:54:20 PDT
<rdar://problem/81914833>
Comment 10 Darin Adler 2021-08-13 17:34:16 PDT
Comment on attachment 435502 [details]
Patch

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

> Source/WebKit/Shared/SandboxExtension.h:169
> +inline auto SandboxExtension::createHandleForTemporaryFile(const String& /*prefix*/, Type) -> std::optional<std::pair<Handle, String>> { return std::optional<std::pair<Handle, String>> { std::pair<Handle, String> { Handle { }, String { } } }; }

Pretty sure we can omit "Handle { }, String { }".
Comment 11 Alex Christensen 2021-08-16 09:54:12 PDT
Perhaps.  The compilers were unable to infer the meaning of {{{ }, { }}} so I was just completely explicit.
Comment 12 Darin Adler 2021-08-17 09:43:29 PDT
(In reply to Alex Christensen from comment #11)
> Perhaps.  The compilers were unable to infer the meaning of {{{ }, { }}} so
> I was just completely explicit.

I’m really puzzled by this. I can’t reproduce this problem with a test program.
Comment 13 Zan Dobersek 2021-08-18 04:50:26 PDT
Just leaving #229233 as a reference here, the std::optional<Handle> retval construction triggered compilation errors on older GCC setups due to ambiguity between the desired constructor and the in-place constructor of std::optional.