[webkit-reviews] review granted: [Bug 211103] Improve SandboxExtension::HandleArray to reduce boilerplate : [Attachment 397866] Patch

[bugzilla-daemon at webkit.org]

Per Arne Vollan <pvollan at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 211103: Improve SandboxExtension::HandleArray to reduce boilerplate
https://bugs.webkit.org/show_bug.cgi?id=211103

Attachment 397866: Patch

https://bugs.webkit.org/attachment.cgi?id=397866&action=review




--- Comment #3 from Per Arne Vollan <pvollan at apple.com> ---
Comment on attachment 397866
  --> https://bugs.webkit.org/attachment.cgi?id=397866
Patch

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

I think this is a great idea. R=me.

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:296
> +Optional<SandboxExtension::HandleArray>
SandboxExtension::createHandlesForFiles(const String& logLabel, const
Vector<String>& paths, Type type)

Since it seems this method is only called with the ReadOnly type, maybe the
type argument can be removed?

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:299
> +    if (paths.isEmpty())
> +	   return WTF::nullopt;

If the method did not return an Optional, but only a HandleArray, this if
statement could be removed.

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:304
> +    for (size_t i = 0; i < paths.size(); ++i) {

Can this be a modern for loop?

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:306
> +	   if (!SandboxExtension::createHandle(paths[i], type, handleArray[i]))
{
> +	       // This can legitimately fail if a directory containing the file
is deleted after the file was chosen.

Perhaps you could add an ASSERT_NOT_REACHED() here, so we can catch it in the
debugger?

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:379
> +Optional<SandboxExtension::HandleArray>
SandboxExtension::createHandlesForMachLookup(const Vector<String>& services,
Optional<audit_token_t> auditToken, OptionSet<Flags> flags)

Could this simply return a HandleArray?

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:387
> +    for (size_t i = 0; i < services.size(); ++i) {

Can a modern for loop be used?

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:389
> +	   if (!SandboxExtension::createHandleForMachLookup(services[i],
auditToken, handleArray[i], flags))
> +	       return WTF::nullopt;

I think it would be good to keep going, even if this call should fail for a
service. Also, I think it would be good to add an ASSERT_NOT_REACHED() in the
case it fails.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:452
> +	   if (auto mediaExtensionHandles =
SandboxExtension::createHandlesForMachLookup(mediaRelatedMachServices(),
WTF::nullopt))
> +	       parameters.mediaExtensionHandles =
WTFMove(*mediaExtensionHandles);

If the function simply returned a HandleArray, this could be just one line.