Bug 83055 - Move (most of) Archive handling from FrameLoader to DocumentLoader
: Move (most of) Archive handling from FrameLoader to DocumentLoader
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 83417
:
  Show dependency treegraph
 
Reported: 2012-04-03 12:15 PST by
Modified: 2012-04-12 12:28 PST (History)


Attachments
patch (13.83 KB, patch)
2012-04-03 12:20 PST, Nate Chapin
pnormand: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
fix gtk compile (15.08 KB, patch)
2012-04-03 13:48 PST, Nate Chapin
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (6.34 MB, application/zip)
2012-04-03 15:24 PST, WebKit Review Bot
no flags Details
Fix mhtml test failure (12.11 KB, patch)
2012-04-04 14:54 PST, Nate Chapin
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (12.61 KB, patch)
2012-04-06 14:01 PST, Nate Chapin
no flags Review Patch | Details | Formatted Diff | Diff
Move setBaseURLOverride call back to FrameLoader (12.14 KB, patch)
2012-04-12 10:14 PST, Nate Chapin
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (12.71 KB, patch)
2012-04-12 11:12 PST, Nate Chapin
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-03 12:15:37 PST
Per the comment in FrameLoader::finishedLoadingDocument(), there's a lot of work FrameLoader is doing that it should be delegating.  For that matter, FrameLoader::m_archive should really be hanging off the DocumentLoader, too.
------- Comment #1 From 2012-04-03 12:20:22 PST -------
Created an attachment (id=135389) [details]
patch
------- Comment #2 From 2012-04-03 12:35:46 PST -------
(From update of attachment 135389 [details])
Attachment 135389 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12320338
------- Comment #3 From 2012-04-03 13:48:48 PST -------
Created an attachment (id=135414) [details]
fix gtk compile
------- Comment #4 From 2012-04-03 15:24:43 PST -------
(From update of attachment 135414 [details])
Attachment 135414 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12317518

New failing tests:
mhtml/check_domain.mht
------- Comment #5 From 2012-04-03 15:24:49 PST -------
Created an attachment (id=135437) [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
------- Comment #6 From 2012-04-04 14:54:27 PST -------
Created an attachment (id=135690) [details]
Fix mhtml test failure
------- Comment #7 From 2012-04-06 13:19:10 PST -------
(From update of attachment 135690 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=135690&action=review

This patch makes too much sense!

> Source/WebCore/loader/DocumentLoader.h:136
>          void addAllArchiveResources(Archive*);

Can this be private now?

> Source/WebCore/loader/DocumentLoader.cpp:453
> +    // Give archive machinery a crack at this document. If the MIME type is not an archive type, it will return 0.

Give archive machinery -> Give the archive machinery
------- Comment #8 From 2012-04-06 13:23:00 PST -------
(In reply to comment #7)
> (From update of attachment 135690 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135690&action=review
> 
> This patch makes too much sense!
> 
> > Source/WebCore/loader/DocumentLoader.h:136
> >          void addAllArchiveResources(Archive*);
> 
> Can this be private now?

Nope. I didn't deal with the usage in FrameLoader::loadArchive(), and I see usages in PasteboardMac and WebKit/mac's WebDataSource.

> 
> > Source/WebCore/loader/DocumentLoader.cpp:453
> > +    // Give archive machinery a crack at this document. If the MIME type is not an archive type, it will return 0.
> 
> Give archive machinery -> Give the archive machinery

Ok.
------- Comment #9 From 2012-04-06 14:01:48 PST -------
Created an attachment (id=136063) [details]
Patch for landing
------- Comment #10 From 2012-04-06 17:43:33 PST -------
(From update of attachment 136063 [details])
Clearing flags on attachment: 136063

Committed r113526: <http://trac.webkit.org/changeset/113526>
------- Comment #11 From 2012-04-06 17:43:46 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #12 From 2012-04-06 21:39:36 PST -------
This broke one of the Chrome browser_tests: RenderViewHostTest.BaseURLParam. I've rolled it back. Can you take a look when you get a chance?
------- Comment #13 From 2012-04-12 10:12:45 PST -------
Reopening since this was rolled out.
------- Comment #14 From 2012-04-12 10:14:18 PST -------
Created an attachment (id=136923) [details]
Move setBaseURLOverride call back to FrameLoader

It turns out that setBaseURLOverride() needs to be called as soon as possible after an MHTML Document is created, and there is currently no way to do that in DocumentLoader.

My next planned cleanup is in the relationship between DocumentLoader, DocumentWriter and FrameLoader, so I will deal with this soon.
------- Comment #15 From 2012-04-12 11:12:20 PST -------
Created an attachment (id=136935) [details]
Patch for landing
------- Comment #16 From 2012-04-12 12:27:46 PST -------
(From update of attachment 136935 [details])
Clearing flags on attachment: 136935

Committed r114016: <http://trac.webkit.org/changeset/114016>
------- Comment #17 From 2012-04-12 12:28:01 PST -------
All reviewed patches have been landed.  Closing bug.