Bug 83055 - Move (most of) Archive handling from FrameLoader to DocumentLoader
Summary: Move (most of) Archive handling from FrameLoader to DocumentLoader
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: 83417
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-03 12:15 PDT by Nate Chapin
Modified: 2012-04-12 12:28 PDT (History)
8 users (show)

See Also:


Attachments
patch (13.83 KB, patch)
2012-04-03 12:20 PDT, Nate Chapin
pnormand: commit-queue-
Details | Formatted Diff | Diff
fix gtk compile (15.08 KB, patch)
2012-04-03 13:48 PDT, Nate Chapin
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (6.34 MB, application/zip)
2012-04-03 15:24 PDT, WebKit Review Bot
no flags Details
Fix mhtml test failure (12.11 KB, patch)
2012-04-04 14:54 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Patch for landing (12.61 KB, patch)
2012-04-06 14:01 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Move setBaseURLOverride call back to FrameLoader (12.14 KB, patch)
2012-04-12 10:14 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Patch for landing (12.71 KB, patch)
2012-04-12 11:12 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 2012-04-03 12:15:37 PDT
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 Nate Chapin 2012-04-03 12:20:22 PDT
Created attachment 135389 [details]
patch
Comment 2 Philippe Normand 2012-04-03 12:35:46 PDT
Comment on attachment 135389 [details]
patch

Attachment 135389 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12320338
Comment 3 Nate Chapin 2012-04-03 13:48:48 PDT
Created attachment 135414 [details]
fix gtk compile
Comment 4 WebKit Review Bot 2012-04-03 15:24:43 PDT
Comment on attachment 135414 [details]
fix gtk compile

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 WebKit Review Bot 2012-04-03 15:24:49 PDT
Created attachment 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 Nate Chapin 2012-04-04 14:54:27 PDT
Created attachment 135690 [details]
Fix mhtml test failure
Comment 7 Adam Barth 2012-04-06 13:19:10 PDT
Comment on attachment 135690 [details]
Fix mhtml test failure

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 Nate Chapin 2012-04-06 13:23:00 PDT
(In reply to comment #7)
> (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?

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 Nate Chapin 2012-04-06 14:01:48 PDT
Created attachment 136063 [details]
Patch for landing
Comment 10 WebKit Review Bot 2012-04-06 17:43:33 PDT
Comment on attachment 136063 [details]
Patch for landing

Clearing flags on attachment: 136063

Committed r113526: <http://trac.webkit.org/changeset/113526>
Comment 11 WebKit Review Bot 2012-04-06 17:43:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 James Simonsen 2012-04-06 21:39:36 PDT
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 Nate Chapin 2012-04-12 10:12:45 PDT
Reopening since this was rolled out.
Comment 14 Nate Chapin 2012-04-12 10:14:18 PDT
Created attachment 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 Nate Chapin 2012-04-12 11:12:20 PDT
Created attachment 136935 [details]
Patch for landing
Comment 16 WebKit Review Bot 2012-04-12 12:27:46 PDT
Comment on attachment 136935 [details]
Patch for landing

Clearing flags on attachment: 136935

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