[webkit-reviews] review denied: [Bug 81153] WebKit2: create sandbox extensions for files that are dropped in an input control : [Attachment 131960] Patch4

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 81153: WebKit2: create sandbox extensions for files that are dropped in an
input control
https://bugs.webkit.org/show_bug.cgi?id=81153

Attachment 131960: Patch4
https://bugs.webkit.org/attachment.cgi?id=131960&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=131960&action=review


Looks good to me, too. Seems worth posting a patch with comments addressed for
final pass.

> Source/WebCore/page/DragActions.h:39
> +	   DragDestinationActionUpload	= 8,

It's quite confusing that this is a bit flag, but it's only used exclusively.
I'm not sure what to suggest to improve the design, but at least a comment
seems necessary here.

> Source/WebCore/page/DragActions.h:40
>	   DragDestinationActionAny	= UINT_MAX

What are the checks that match action to DragDestinationActionAny? Is it OK
that we now have an additional action that's matching DragDestinationActionAny?


> Source/WebKit2/ChangeLog:9
> +	   Now the the pasteboard access is performed only in the UI process,
it is

Typo "the the".

> Source/WebKit2/ChangeLog:10
> +	   necessary to create sanbox extensions for each file that is dropped
into

Typo: "sanbox".

> Source/WebKit2/Shared/SandboxExtension.h:115
> +inline const SandboxExtension::Handle&
SandboxExtension::HandleArray::operator[](size_t) const { return *m_data; }
> +inline SandboxExtension::Handle&
SandboxExtension::HandleArray::operator[](size_t) { return *m_data; }

This dereferences null, which is a bug.

> Source/WebKit2/Shared/mac/SandboxExtensionMac.mm:145
> +    for (size_t i = 0; i < size; i++)
> +	   decoder->decode(handles[i]);

Shouldn't this also check decoding result?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:866
> +    SandboxExtension::HandleArray sandboxExtensionHandleArray;

I like to use more descriptive names in such cases. Maybe "emptyArray"? Ditto
below.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1697
> +    handles.reserve([files count]);
> +    for (unsigned int i = 0; i < [files count]; i++) {

s/unsigned int/unsigned/.

The name "reserve" is incorrect here. Reserving space in a container doesn't
change its size, so handles[i] would still be a mistake after this reserve().

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1700
> +	   if (![[NSFileManager defaultManager] fileExistsAtPath:file])
> +	       continue;

Does this return YES for directories too? It's legitimate to drop directories
(especially bundles).