Bug 161228 – [Fetch API] Blob type should be set from Response/Request contentType header

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Currently, Content-Type information is not used at all

[youenn fablet]

Created attachment 287076 [details]
Patch

[Build Bot]

Comment on attachment 287076 [details]
Patch

Attachment 287076 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1945354

New failing tests:
imported/w3c/web-platform-tests/fetch/api/response/response-init-002.html

[Build Bot]

Created attachment 287080 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[youenn fablet]

Created attachment 287093 [details]
Fixing empty Content-Type

[Alex Christensen]

Comment on attachment 287093 [details]
Fixing empty Content-Type

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

> Source/WebCore/Modules/fetch/FetchBody.cpp:100
> +    if (!m_contentType.isNull())
> +        headers.fastSet(HTTPHeaderName::ContentType, m_contentType);

I think this will happen too often.  Shouldn't this be if (!m_contentType.isNull() && contentType.isNull()) ?

[youenn fablet]

Comment on attachment 287093 [details]
Fixing empty Content-Type

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

>> Source/WebCore/Modules/fetch/FetchBody.cpp:100
>> +        headers.fastSet(HTTPHeaderName::ContentType, m_contentType);
> 
> I think this will happen too often.  Shouldn't this be if (!m_contentType.isNull() && contentType.isNull()) ?

What does bring the second check? It is already tested before.

[Alex Christensen]

Comment on attachment 287093 [details]
Fixing empty Content-Type

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

>>> Source/WebCore/Modules/fetch/FetchBody.cpp:100
>>> +        headers.fastSet(HTTPHeaderName::ContentType, m_contentType);
>> 
>> I think this will happen too often.  Shouldn't this be if (!m_contentType.isNull() && contentType.isNull()) ?
> 
> What does bring the second check? It is already tested before.

Right now, if m_contentType is already set but contentType is not, then the header will be set.  This is good.
If m_contentType is set and contentType is also set, then m_contentType will be set.  This is also good.  But then the header will be set back to its original value.  This is a wasteful operation.  I guess there's not a string copy, though.  Just setting a value in a HashMap<enum, String>, so it might not be worth the check.

[youenn fablet]

> If m_contentType is set and contentType is also set, then m_contentType will
> be set.  This is also good.  But then the header will be set back to its original value.

If contentType is set, we exist early after m_contentType is set.
This ensures we do not set needlessly the header.

[WebKit Commit Bot]

Comment on attachment 287093 [details]
Fixing empty Content-Type

Rejecting attachment 287093 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 287093, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ayoutTests/imported/w3c/ChangeLog
error: Error building trees

Failed to run "['git', 'commit', '--all', '-F', '-']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

You have both LayoutTests/imported/w3c and LayoutTests/imported/w3c/ChangeLog
You have both LayoutTests/imported/w3c and LayoutTests/imported/w3c/ChangeLog
error: Error building trees

Failed to run "['git', 'commit', '--all', '-F', '-']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/1952740

[youenn fablet]

Not sure why cq failed.
Let's retry reuploading/relanding the patch.

[youenn fablet]

Created attachment 287200 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 287200 [details]
Patch for landing

Clearing flags on attachment: 287200

Committed r205076: <http://trac.webkit.org/changeset/205076>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

(In reply to comment #5)
> Comment on attachment 287093 [details]
> Fixing empty Content-Type
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287093&action=review
> 
> > Source/WebCore/Modules/fetch/FetchBody.cpp:100
> > +    if (!m_contentType.isNull())
> > +        headers.fastSet(HTTPHeaderName::ContentType, m_contentType);
> 
> I think this will happen too often.  Shouldn't this be if
> (!m_contentType.isNull() && contentType.isNull()) ?

We are sometimes doing a get then a set, if there is no content-type header. We could optimize this by using a single HashMap::add call.
Not sure whether that situation happens often enough on HTTPHeaders to add such code?