Bug 161228

Summary: [Fetch API] Blob type should be set from Response/Request contentType header
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, buildbot, commit-queue, rniwa, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 151937    
Attachments:
Description Flags
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews107 for mac-yosemite-wk2
none
Fixing empty Content-Type
none
Patch for landing none

Description youenn fablet 2016-08-25 23:44:21 PDT
Currently, Content-Type information is not used at all
Comment 1 youenn fablet 2016-08-26 00:34:32 PDT
Created attachment 287076 [details]
Patch
Comment 2 Build Bot 2016-08-26 01:28:06 PDT
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
Comment 3 Build Bot 2016-08-26 01:28:08 PDT
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
Comment 4 youenn fablet 2016-08-26 02:51:50 PDT
Created attachment 287093 [details]
Fixing empty Content-Type
Comment 5 Alex Christensen 2016-08-26 10:21:08 PDT
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()) ?
Comment 6 youenn fablet 2016-08-26 11:02:57 PDT
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.
Comment 7 Alex Christensen 2016-08-26 11:28:10 PDT
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.
Comment 8 youenn fablet 2016-08-27 01:54:56 PDT
> 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.
Comment 9 WebKit Commit Bot 2016-08-27 02:17:07 PDT
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
Comment 10 youenn fablet 2016-08-27 03:10:07 PDT
Not sure why cq failed.
Let's retry reuploading/relanding the patch.
Comment 11 youenn fablet 2016-08-27 03:11:48 PDT
Created attachment 287200 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2016-08-27 03:42:25 PDT
Comment on attachment 287200 [details]
Patch for landing

Clearing flags on attachment: 287200

Committed r205076: <http://trac.webkit.org/changeset/205076>
Comment 13 WebKit Commit Bot 2016-08-27 03:42:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 youenn fablet 2016-08-27 06:21:45 PDT
(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?