Bug 67443 - Clean up CachedResource::load()
Summary: Clean up CachedResource::load()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on: 69004
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-01 15:01 PDT by Nate Chapin
Modified: 2011-09-28 08:25 PDT (History)
5 users (show)

See Also:


Attachments
patch (29.16 KB, patch)
2011-09-01 15:03 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Patch for landing (30.76 KB, patch)
2011-09-02 10:13 PDT, Nate Chapin
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Fix a couple of failing tests (30.06 KB, patch)
2011-09-21 10:23 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Patch for landing (33.22 KB, patch)
2011-09-26 12:57 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2011-09-01 15:01:46 PDT
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.
Comment 1 Nate Chapin 2011-09-01 15:03:52 PDT
Created attachment 106039 [details]
patch
Comment 2 WebKit Review Bot 2011-09-01 15:56:05 PDT
Comment on attachment 106039 [details]
patch

Attachment 106039 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9578565
Comment 3 WebKit Review Bot 2011-09-01 17:15:20 PDT
Comment on attachment 106039 [details]
patch

Attachment 106039 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9586205
Comment 4 WebKit Review Bot 2011-09-01 18:13:59 PDT
Comment on attachment 106039 [details]
patch

Attachment 106039 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9586229
Comment 5 Antti Koivisto 2011-09-02 02:42:52 PDT
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
Comment 6 Nate Chapin 2011-09-02 10:13:06 PDT
Created attachment 106149 [details]
Patch for landing
Comment 7 WebKit Review Bot 2011-09-02 11:21:12 PDT
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
Comment 8 Nate Chapin 2011-09-21 10:23:31 PDT
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 9 Antti Koivisto 2011-09-26 11:38:07 PDT
Comment on attachment 108175 [details]
Fix a couple of failing tests

r=me, some more motivation in the ChangeLog entry would be nice.
Comment 10 Nate Chapin 2011-09-26 12:57:57 PDT
Created attachment 108713 [details]
Patch for landing
Comment 11 WebKit Review Bot 2011-09-26 17:20:19 PDT
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 12 WebKit Review Bot 2011-09-26 18:28:27 PDT
Comment on attachment 108713 [details]
Patch for landing

Clearing flags on attachment: 108713

Committed r96060: <http://trac.webkit.org/changeset/96060>
Comment 13 WebKit Review Bot 2011-09-26 18:28:32 PDT
All reviewed patches have been landed.  Closing bug.