Bug 91920 – [WIN] Implement WebMutableURLRequest::setHTTPBody()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Patrick R. Gansterer]

[WIN] Implement WebMutableURLRequest::setHTTPBody()

[Patrick R. Gansterer]

Created attachment 153661 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 153661 [details]
Patch

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

> Source/WebKit/win/WebMutableURLRequest.cpp:298
> +    Vector<char, 1024> buffer(statstg.cbSize.QuadPart);

statstg.cbSize.QuadPart is a 64-bit unsigned integer. The Vector constructor takes a size_t, which is a 32-bit unsigned integer on 32-bit systems. So this will potentially overflow. We should probably return a failure result if overflow will occur.

> Source/WebKit/win/WebMutableURLRequest.cpp:303
> +    if (FAILED(data->Read(buffer.data(), buffer.size(), &bytesRead)))
> +        return E_FAIL;
> +
> +    m_request.setHTTPBody(FormData::create(buffer.data(), bytesRead));

It's too bad we can't read the data into the FormData directly. The extra copy is unfortunate.

[Patrick R. Gansterer]

Created attachment 153830 [details]
Patch

[WebKit Review Bot]

Attachment 153830 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/network/FormData.cpp:156:  Declaration has space between type name and * in char *target  [whitespace/declaration] [3]
Source/WebKit/win/WebMutableURLRequest.cpp:301:  Declaration has space between type name and * in char *formData  [whitespace/declaration] [3]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Patrick R. Gansterer]

Created attachment 153832 [details]
Patch

[Brent Fulgham]

Comment on attachment 153832 [details]
Patch

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

Looks good, but I think it could be clearer with some slight changes!

> Source/WebCore/platform/network/FormData.cpp:161
> +void FormData::appendUninitializedData(size_t size, char*& data)

I was initially confused about what this was supposed to do. I think it would be clearer if this method was called something like "expandDataStore".
Also, couldn't it return the pointer to the start of the new allocation? This would allow us to write something like:

void* target = expandDataStore(size);
memcpy(target, data, size);

> Source/WebKit/win/WebMutableURLRequest.cpp:303
> +    httpBody->appendUninitializedData(stat.cbSize.LowPart, formData);

... Then this would be:

void* formData = httpBody->expandDataStore(stat.cbSize.LowPart);

[Patrick R. Gansterer]

Created attachment 212355 [details]
Patch

[WebKit Commit Bot]

Attachment 212355 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/network/FormData.cpp', u'Source/WebCore/platform/network/FormData.h', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebMutableURLRequest.cpp']" exit_code: 1
Source/WebCore/platform/network/FormData.h:133:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Patrick R. Gansterer]

Created attachment 212357 [details]
Patch

[Brent Fulgham]

Comment on attachment 212357 [details]
Patch

Very nice!  r=me.

[WebKit Commit Bot]

Comment on attachment 212357 [details]
Patch

Clearing flags on attachment: 212357

Committed r156303: <http://trac.webkit.org/changeset/156303>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.