Bug 228875 – SandboxExtension::Handle creation should return std::optional instead of bool

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

[Alex Christensen]

SandboxExtension::Handle creation should return std::optional instead of bool

[Alex Christensen]

Created attachment 435089 [details]
Patch

[Sam Weinig]

Nice!

[Chris Dumez]

EWS is all red.

[Alex Christensen]

Created attachment 435452 [details]
Patch

[Alex Christensen]

Created attachment 435460 [details]
Patch

[youenn fablet]

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

[Alex Christensen]

Created attachment 435502 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/81914833>

[Darin Adler]

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 { }".

[Alex Christensen]

Perhaps.  The compilers were unable to infer the meaning of {{{ }, { }}} so I was just completely explicit.

[Darin Adler]

(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.

[Zan Dobersek]

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.