Bug 75887 - Followup SubresourceLoader cleanup after r100311
Summary: Followup SubresourceLoader cleanup after r100311
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:
Blocks:
 
Reported: 2012-01-09 13:35 PST by Nate Chapin
Modified: 2012-01-11 15:54 PST (History)
5 users (show)

See Also:


Attachments
patch (10.29 KB, patch)
2012-01-09 13:51 PST, Nate Chapin
no flags Details | Formatted Diff | Diff
Diffing from the right directory this time (12.07 KB, patch)
2012-01-09 14:27 PST, Nate Chapin
abarth: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Merged to trunk. (12.02 KB, patch)
2012-01-09 15:47 PST, Nate Chapin
no flags Details | Formatted Diff | Diff
Patch for landing (12.49 KB, patch)
2012-01-11 14:13 PST, 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 2012-01-09 13:35:57 PST
Per https://bugs.webkit.org/show_bug.cgi?id=71149#c6 , there were some details that were deferred in http://trac.webkit.org/changeset/100311 :

1) Add some ASSERTS to the destructor that show we properly advance the state machine to the end (and that documenting becomes null).
2) Use a member variable to track whether we've incremented/decremented the request count to ensure we balance them properly (e.g., by ASSERTing in its destructor that we've balanced).  That will also remove a bunch of multipart related banches because we can tell this object to decrement the request count if it hasn't already done so.

In addition to these cleanups, I'd like to add: 
3) Remove the CachedImage multipart special case by adding an overridden setResponse() to CachedImage to do that work.
4) Remove the custom didReceiveData() call in the multipart logic of didReceiveResponse().  Have didReceiveData() get called normally, then do the multipart cleanup.

Patch shortly.
Comment 1 Nate Chapin 2012-01-09 13:51:55 PST
Created attachment 121723 [details]
patch
Comment 2 Adam Barth 2012-01-09 14:23:52 PST
Comment on attachment 121723 [details]
patch

ChangeLog?
Comment 3 Nate Chapin 2012-01-09 14:27:20 PST
Created attachment 121726 [details]
Diffing from the right directory this time
Comment 4 Adam Barth 2012-01-09 14:31:22 PST
Comment on attachment 121726 [details]
Diffing from the right directory this time

Very nice.
Comment 5 WebKit Review Bot 2012-01-09 14:33:18 PST
Comment on attachment 121726 [details]
Diffing from the right directory this time

Rejecting attachment 121726 [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:
re/loader/SubresourceLoader.cpp
Hunk #8 FAILED at 222.
Hunk #9 succeeded at 307 (offset 7 lines).
1 out of 9 hunks FAILED -- saving rejects to file Source/WebCore/loader/SubresourceLoader.cpp.rej
patching file Source/WebCore/loader/SubresourceLoader.h
Hunk #2 FAILED at 72.
1 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/loader/SubresourceLoader.h.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/11113585
Comment 6 Nate Chapin 2012-01-09 15:47:59 PST
Created attachment 121738 [details]
Merged to trunk.

Requesting a new approval since the merge was non-trivial.
Comment 7 WebKit Review Bot 2012-01-09 17:29:53 PST
Comment on attachment 121738 [details]
Merged to trunk.

Rejecting attachment 121738 [details] from commit-queue.

New failing tests:
tables/mozilla/bugs/bug50695-2.html
http/tests/multipart/invalid-image-data-standalone.html
editing/pasteboard/copy-standalone-image.html
editing/pasteboard/data-transfer-items-image-png.html
fast/events/standalone-image-drag-to-editable.html
Full output: http://queues.webkit.org/results/11188671
Comment 8 WebKit Review Bot 2012-01-09 18:11:47 PST
Comment on attachment 121738 [details]
Merged to trunk.

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

New failing tests:
tables/mozilla/bugs/bug50695-2.html
http/tests/multipart/invalid-image-data-standalone.html
editing/pasteboard/copy-standalone-image.html
editing/pasteboard/data-transfer-items-image-png.html
fast/events/standalone-image-drag-to-editable.html
Comment 9 Nate Chapin 2012-01-11 14:13:10 PST
Created attachment 122093 [details]
Patch for landing
Comment 10 WebKit Review Bot 2012-01-11 15:54:43 PST
Comment on attachment 122093 [details]
Patch for landing

Clearing flags on attachment: 122093

Committed r104756: <http://trac.webkit.org/changeset/104756>
Comment 11 WebKit Review Bot 2012-01-11 15:54:49 PST
All reviewed patches have been landed.  Closing bug.