Bug 83908 - Move more of committing and starting to write a Document to DocumentLoader
: Move more of committing and starting to write a Document to DocumentLoader
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nate Chapin
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-13 10:52 PDT by Nate Chapin
Modified: 2012-06-07 15:41 PDT (History)
6 users (show)

See Also:


Attachments
patch (9.08 KB, patch)
2012-04-13 10:59 PDT, Nate Chapin
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (6.39 MB, application/zip)
2012-04-13 12:43 PDT, WebKit Review Bot
no flags Details
Fix test failures (9.14 KB, patch)
2012-04-13 16:35 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-13 10:52:29 PDT
1. DocumentLoader::m_gotFirstByte is weird.  It appears to be primarily be used to gate committing a provisional load, except that it's always true when commitIfReady() is called.  Remove that usage and instead use it to ensure we don't start document parsing more than once.
2. FrameLoader::m_hasReceivedFirstData effectively exists for this purpose currently, even though that's not what the name implies and it's per-DocumentLoader state living on FrameLoader.  Remove it.
3. After these changes, FrameLoader::willSetEncoding() just calls receivedFirstData(), so call receivedFirstData directly.  Do it from DocumentLoader instead of DocumentWriter.
4. Move some DocumentLoader calls from FrameLoader::receivedFirstData() into DocumentLoader::commitData().

This patch will remove the need for every FrameLoaderClient to call DocumentWriter::setEncoding() in finishedLoading() to create an empty document.  I will remove that dead code in a separate patch.
Comment 1 Nate Chapin 2012-04-13 10:59:34 PDT
Created attachment 137103 [details]
patch
Comment 2 WebKit Review Bot 2012-04-13 12:43:32 PDT
Comment on attachment 137103 [details]
patch

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

New failing tests:
mhtml/multi_frames_binary.mht
mhtml/page_with_css_and_js_unmht.mht
Comment 3 WebKit Review Bot 2012-04-13 12:43:38 PDT
Created attachment 137128 [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 4 Nate Chapin 2012-04-13 15:12:10 PDT
Comment on attachment 137103 [details]
patch

Yeah, those test failures are legitimate, removing review?
Comment 5 Nate Chapin 2012-04-13 16:35:34 PDT
Created attachment 137176 [details]
Fix test failures
Comment 6 Adam Barth 2012-04-30 10:50:15 PDT
Comment on attachment 137176 [details]
Fix test failures

This looks great.
Comment 7 WebKit Review Bot 2012-04-30 11:50:45 PDT
Comment on attachment 137176 [details]
Fix test failures

Clearing flags on attachment: 137176

Committed r115654: <http://trac.webkit.org/changeset/115654>
Comment 8 WebKit Review Bot 2012-04-30 11:51:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Darin Adler 2012-06-05 11:43:40 PDT
Looks like this change broke loading web archives in Safari; encodings specified in the archive are apparently not being respected. More details forthcoming as we diagnose further; probably a separate bug report too.
Comment 10 Alexey Proskuryakov 2012-06-06 15:29:11 PDT
Regressions caused by this so far: bug 85275, bug 88428, bug 88436.
Comment 11 Adam Barth 2012-06-07 15:18:10 PDT
Do you think it's worth continuing to work though the regressions or should we consider backing out this patch?
Comment 12 Nate Chapin 2012-06-07 15:29:52 PDT
(In reply to comment #11)
> Do you think it's worth continuing to work though the regressions or should we consider backing out this patch?

I'm disinclined to back it out now, especially since I'm close to a layout test to go with my patch for the last open regression, and reverting would entail backing out multiple patches that depend on it.

That said, I fully recognize that I'm biased :)
Comment 13 Brady Eidson 2012-06-07 15:41:48 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > Do you think it's worth continuing to work though the regressions or should we consider backing out this patch?
> 
> I'm disinclined to back it out now, especially since I'm close to a layout test to go with my patch for the last open regression, and reverting would entail backing out multiple patches that depend on it.
> 
> That said, I fully recognize that I'm biased :)

Especially since Nate is close to wrapping up the last known regression, I think we can stick with it.