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

youenn fablet
Reported 2016-08-25 23:44:21 PDT
Currently, Content-Type information is not used at all
Attachments
Patch (12.96 KB, patch)
2016-08-26 00:34 PDT, youenn fablet
buildbot: commit-queue-
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.11 MB, application/zip)
2016-08-26 01:28 PDT, Build Bot
no flags
Fixing empty Content-Type (17.84 KB, patch)
2016-08-26 02:51 PDT, youenn fablet
no flags
Patch for landing (17.86 KB, patch)
2016-08-27 03:11 PDT, youenn fablet
no flags
youenn fablet
Comment 1 2016-08-26 00:34:32 PDT
Build Bot
Comment 2 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
Build Bot
Comment 3 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
youenn fablet
Comment 4 2016-08-26 02:51:50 PDT
Created attachment 287093 [details] Fixing empty Content-Type
Alex Christensen
Comment 5 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()) ?
youenn fablet
Comment 6 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.
Alex Christensen
Comment 7 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.
youenn fablet
Comment 8 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.
WebKit Commit Bot
Comment 9 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
youenn fablet
Comment 10 2016-08-27 03:10:07 PDT
Not sure why cq failed. Let's retry reuploading/relanding the patch.
youenn fablet
Comment 11 2016-08-27 03:11:48 PDT
Created attachment 287200 [details] Patch for landing
WebKit Commit Bot
Comment 12 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>
WebKit Commit Bot
Comment 13 2016-08-27 03:42:32 PDT
All reviewed patches have been landed. Closing bug.
youenn fablet
Comment 14 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?
Note You need to log in before you can comment on or make changes to this bug.