[webkit-reviews] review granted: [Bug 177047] [Cocoa] Upstream sandbox-related WebKitSystemInterface functions : [Attachment 321036] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 177047: [Cocoa] Upstream sandbox-related WebKitSystemInterface functions
https://bugs.webkit.org/show_bug.cgi?id=177047

Attachment 321036: Patch

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




--- Comment #7 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 321036
  --> https://bugs.webkit.org/attachment.cgi?id=321036
Patch

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

> Source/WebKit/ChangeLog:12
> +	   (): Deleted.

What was deleted? Please file a bug to fix prepare-ChangeLog.

> Source/WebKit/Shared/SandboxExtension.h:46
> +    enum class Type {

I take it you feel creating this Type namespace improves readability?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:86
> +    qtn_proc_t quarantineProperties = qtn_proc_alloc();

For your consideration I suggest we make this a std::unique_ptr with
qtn_proc_free() as its custom deleter. Then we can simplify the implementation
of this function because we can omit  the calls to qtn_proc_free() and this
makes the implementation less error prone in the unlikely event that a new
early return code path is added.

> Source/WebKit/Shared/mac/SandboxExtensionMac.mm:64
> +    bool consume()

Would it make sense to add an attribute to warn if the return value is unused?

> Source/WebKit/Shared/mac/SandboxExtensionMac.mm:76
> +	   int error = sandbox_extension_release(m_handle);

We could use std::exchange(m_handle, 0) here and then reduce this function to
one line.

> Source/WebKit/Shared/mac/SandboxExtensionMac.mm:81
> +    const char* getSerializedFormat(size_t& length)

Ditto.

> Source/WebKit/Shared/mac/SandboxExtensionMac.mm:92
> +	       m_token = sandbox_extension_issue_file(APP_SANDBOX_READ, path,
0);

I am not near my Mac. I am assuming the right-hand side expression allocates
and returns a string that you take ownership of.

> Source/WebKit/Shared/mac/SandboxExtensionMac.mm:103
> +    char* m_token { nullptr };

Can we make this a std::unique_ptr? Then we do not need an in-class data member
initializer (as unique_ptr default constructs to nullptr) and we can remove the
destructor.