There are several details I'd like to deal with in CachedResource::load(): 1. Remove the incremental parameter and CachedResourceRequest::m_incremental. It's redundant, since its only purpose is to determine whether we call CachedResource::data() before we've received all data, and every CachedResource type that doesn't want incremental data starts its data() implementation with "if (!allDataReceived) return false". 2. Add the SecurityCheckPolicy to ResourceLoaderOptions. 3. Set CachedResource::m_options in the load() call rather than via a dedicated setter, as requested in https://bugs.webkit.org/show_bug.cgi?id=66018 4. Merge the two CachedResource::load() functions. I'm going to try to do this in one patch, but if it looks too complicated to review, I'm happy to split it up.
Created attachment 106039 [details] patch
Comment on attachment 106039 [details] patch Attachment 106039 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9578565
Comment on attachment 106039 [details] patch Attachment 106039 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9586205
Comment on attachment 106039 [details] patch Attachment 106039 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9586229
Comment on attachment 106039 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=106039&action=review r=me > Source/WebCore/ChangeLog:8 > + Clean up CachedResource::load(). > + https://bugs.webkit.org/show_bug.cgi?id=67443 > + > + Reviewed by NOBODY (OOPS!). > + > + No new tests, refactor only. It would be good to give a short overview here how this patch makes the world a better place. > Source/WebCore/loader/ResourceLoaderOptions.h:60 > + ResourceLoaderOptions(SendCallbackPolicy sendLoadCallbacksArg, ContentSniffingPolicy sniffContentArg, DataBufferingPolicy shouldBufferDataArg, StoredCredentials allowCredentialsArg, SecurityCheckPolicy securityCheckArg) : sendLoadCallbacks(sendLoadCallbacksArg), sniffContent(sniffContentArg), shouldBufferData(shouldBufferDataArg), allowCredentials(allowCredentialsArg), securityCheck(securityCheckArg) { } *Arg naming is not necessary, you can do : sendLoadCallbacks(sendLoadCallbacks) > Source/WebCore/loader/ResourceLoader.cpp:122 > + FrameLoader* fl = m_frame->loader(); fl -> frameLoader
Created attachment 106149 [details] Patch for landing
Comment on attachment 106149 [details] Patch for landing Rejecting attachment 106149 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: : Unexpected image mismatch : (5) fast/text/atsui-multiple-renderers.html = IMAGE fast/text/international/danda-space.html = IMAGE fast/text/international/thai-baht-space.html = IMAGE fast/text/international/thai-line-breaks.html = IMAGE platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE Regressions: Unexpected image and text mismatch : (2) fast/borders/border-image-repeat.html = IMAGE+TEXT svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT Full output: http://queues.webkit.org/results/9586510
Created attachment 108175 [details] Fix a couple of failing tests Two changes from previous version: 1. Call releaseResources() instead of cancel() in ResourceLoader::init(). Since we haven't called willSendRequest() yet, it's not really a cancel, and not all clients play nicely with cancel() before willSendRequest(). 2. In SubresourceLoader::create(), call DocumentLoader::addSubresourceLoader() after init(), instead of before. If we fail a security check and exit early, we won't ever call removeSubresourceLoader(), resulting in load events never firing. Sorry for the long delay in getting a new patch out.
Comment on attachment 108175 [details] Fix a couple of failing tests r=me, some more motivation in the ChangeLog entry would be nice.
Created attachment 108713 [details] Patch for landing
Comment on attachment 108713 [details] Patch for landing Rejecting attachment 108713 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 Last 500 characters of output: toinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain result = func(*args) File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open return self.do_open(conn_factory, req) File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open raise URLError(err) urllib2.URLError: <urlopen error [Errno 110] Connection timed out> Full output: http://queues.webkit.org/results/9883045
Comment on attachment 108713 [details] Patch for landing Clearing flags on attachment: 108713 Committed r96060: <http://trac.webkit.org/changeset/96060>
All reviewed patches have been landed. Closing bug.