Bug 67443 – Clean up CachedResource::load()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

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.

[Nate Chapin]

Created attachment 106039 [details]
patch

[WebKit Review Bot]

Comment on attachment 106039 [details]
patch

Attachment 106039 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9578565

[WebKit Review Bot]

Comment on attachment 106039 [details]
patch

Attachment 106039 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9586205

[WebKit Review Bot]

Comment on attachment 106039 [details]
patch

Attachment 106039 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9586229

[Antti Koivisto]

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

[Nate Chapin]

Created attachment 106149 [details]
Patch for landing

[WebKit Review Bot]

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

[Nate Chapin]

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.

[Antti Koivisto]

Comment on attachment 108175 [details]
Fix a couple of failing tests

r=me, some more motivation in the ChangeLog entry would be nice.

[Nate Chapin]

Created attachment 108713 [details]
Patch for landing

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 108713 [details]
Patch for landing

Clearing flags on attachment: 108713

Committed r96060: <http://trac.webkit.org/changeset/96060>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.