[webkit-reviews] review denied: [Bug 18343] [GTK] Soup backend must handle upload of multiple files : [Attachment 27154] updated proposed patch

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has denied Gustavo Noronha Silva
<gns at gnome.org>'s request for review:
Bug 18343: [GTK] Soup backend must handle upload of multiple files
https://bugs.webkit.org/show_bug.cgi?id=18343

Attachment 27154: updated proposed patch
https://bugs.webkit.org/attachment.cgi?id=27154&action=review

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
Some brief style comments since I'm not familiar with soup at all.
 
> +#include <fcntl.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +
>  #include "config.h"
>  #include "CString.h"
>  #include "ResourceHandle.h"

Per our style guidelines, config.h and ResourceHandle.h should be included
first in this file and grouped together, followed by other WebCore headers,
then other headers.  That is to say that the new includes should be moved down 
by the exiting gio / libsoup includes, and CString.h moved down into the other
WebCore includes. 

> @@ -58,6 +64,20 @@ enum
>      ERROR_BAD_NON_HTTP_METHOD
>  };
>  
> +struct mapping
> +{
> +    gpointer ptr;
> +    gsize length;
> +};
> +
> +static void
> +free_mapping(gpointer data)
> +{
> +    struct mapping* mapping = (struct mapping*)data;
> +    munmap (mapping->ptr, mapping->length);
> +    g_slice_free (struct mapping, mapping);
> +}
> +
>  static void cleanupGioOperation(ResourceHandleInternal* handle);

Types should be named with an initial capital letter.  The name "mapping" is
rather generic and could perhaps better describe what the type represents.  The
return type and linkage specifier of a function declaration should be on the
same line as the rest of the declaration.  Function names in WebCore should use
camelCase rather than lower_case_with_underscores.  The extra "struct"
qualifier when referring to the type should be omitted as it is not needed in
C++.  The type cast should use a C++-style cast rather than a C-style cast.

> +	   gsize num_elements = httpBody->elements().size();
> +

This should be of type size_t rather than gsize since it is only used to index
in to a WebCore type.  It should also be named in camelCase.

> +	       for (gsize count = 0; count < num_elements; count++) {

Should be size_t for the same reason as above.	"i" would probably be a more
idiomatic name than count. 

> +		   const FormDataElement& element =
httpBody->elements()[count];
> +
> +		   if (element.m_type == FormDataElement::data) {
> +		       soup_message_body_append(msg->request_body,
SOUP_MEMORY_TEMPORARY, element.m_data.data(), element.m_data.size());
> +		   } else {

The one-line body of the if should not have {}s around it.

> +		       /*
> +			* mapping for uploaded files code inspired by technique
used in
> +			* libsoup's simple-httpd test
> +			*/
> +		       SoupBuffer* soupBuffer;
> +		       struct stat statbuf;

These variables should be declared immediately before they're first needed.

> +		       struct mapping* mapping = g_slice_new (struct mapping);
> +		       int fd = open(element.m_filename.utf8().data(),
O_CLOEXEC|O_RDONLY);
> +
> +		       fstat(fd, &statbuf);
> +
> +		       mapping->ptr = mmap(NULL, statbuf.st_size, PROT_READ,
MAP_PRIVATE, fd, 0);
> +		       mapping->length = statbuf.st_size;
> +
> +		       close(fd);

This code really needs some error-handling.  If the file has been deleted since
the user selected it in the form control, opening the file will fail and we'll
probably end up segfaulting after mmap returns an error and we treat it as a
pointer.

I'm going to mark this as r- due to the lack of error handling and style
issues.  If someone familiar with libsoup can chime in on whether the usage is
correct then this can be r+'d when those issues are addressed.