[Cocoa] Upstream sandbox-related WebKitSystemInterface functions
Created attachment 321028 [details] Patch
Attachment 321028 [details] did not pass style-queue: ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:52: qtn_proc_apply_to_self is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:53: qtn_proc_init_with_self is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:54: qtn_proc_set_flags is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:55: qtn_proc_alloc is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:56: qtn_proc_free is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 5 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 321029 [details] Patch
Attachment 321029 [details] did not pass style-queue: ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:52: qtn_proc_apply_to_self is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:53: qtn_proc_init_with_self is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:54: qtn_proc_set_flags is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:55: qtn_proc_alloc is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:56: qtn_proc_free is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 5 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 321036 [details] Patch
Attachment 321036 [details] did not pass style-queue: ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:52: qtn_proc_apply_to_self is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:53: qtn_proc_init_with_self is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:54: qtn_proc_set_flags is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:55: qtn_proc_alloc is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/Platform/spi/mac/QuarantineSPI.h:56: qtn_proc_free is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 5 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 321036 [details] 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.
(In reply to Daniel Bates from comment #7) > > > Source/WebKit/Shared/mac/SandboxExtensionMac.mm:81 > > + const char* getSerializedFormat(size_t& length) > > Ditto. > I meant to write: Would it make sense to add an attribute to warn if the return value is unused?
Created attachment 321134 [details] Patch
(In reply to Daniel Bates from comment #7) > Comment on attachment 321036 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=321036&action=review > > 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. I used a ScopeGuard that calls qtn_proc_free(), which I think is a little cleaner than creating a custom deleter struct for unique_ptr. > > > 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? Done. > > > 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. Done. > > > Source/WebKit/Shared/mac/SandboxExtensionMac.mm:81 > > + const char* getSerializedFormat(size_t& length) > > Ditto. Marked with WARN_UNUSED_RETURN. > > > 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. Yes. > > > 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. I left this as-is. I think having a destructor calling free() is simpler than creating a custom deleter struct. The class is designed to only assign to m_token during construction and free it during destruction, so there aren't weird lifetime issues that unique_ptr would help solve.
Comment on attachment 321134 [details] Patch Clearing flags on attachment: 321134 Committed r222183: <http://trac.webkit.org/changeset/222183>
All reviewed patches have been landed. Closing bug.
Comment on attachment 321036 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321036&action=review >>> 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. > > I left this as-is. I think having a destructor calling free() is simpler than creating a custom deleter struct. The class is designed to only assign to m_token during construction and free it during destruction, so there aren't weird lifetime issues that unique_ptr would help solve. Would be safer if you deleted the assignment operator and copy constructor so we don’t accidentally make a copy of this.
<rdar://problem/34693377>