Bug 161228

Created attachment 287076 [details] Patch

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

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

Created attachment 287093 [details] Fixing empty Content-Type

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 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 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.

> 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 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

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

Created attachment 287200 [details] Patch for landing

Comment on attachment 287200 [details] Patch for landing Clearing flags on attachment: 287200 Committed r205076: <http://trac.webkit.org/changeset/205076>

All reviewed patches have been landed. Closing bug.

(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?