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
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-04-13 10:52 PST by
Modified: 2012-06-07 15:41 PST (History)


Attachments
patch (9.08 KB, patch)
2012-04-13 10:59 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.39 MB, application/zip)
2012-04-13 12:43 PST, WebKit Review Bot
no flags Details
Fix test failures (9.14 KB, patch)
2012-04-13 16:35 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-13 10:52:29 PST
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 From 2012-04-13 10:59:34 PST -------
Created an attachment (id=137103) [details]
patch
------- Comment #2 From 2012-04-13 12:43:32 PST -------
(From update of attachment 137103 [details])
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 From 2012-04-13 12:43:38 PST -------
Created an attachment (id=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 From 2012-04-13 15:12:10 PST -------
(From update of attachment 137103 [details])
Yeah, those test failures are legitimate, removing review?
------- Comment #5 From 2012-04-13 16:35:34 PST -------
Created an attachment (id=137176) [details]
Fix test failures
------- Comment #6 From 2012-04-30 10:50:15 PST -------
(From update of attachment 137176 [details])
This looks great.
------- Comment #7 From 2012-04-30 11:50:45 PST -------
(From update of attachment 137176 [details])
Clearing flags on attachment: 137176

Committed r115654: <http://trac.webkit.org/changeset/115654>
------- Comment #8 From 2012-04-30 11:51:08 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #9 From 2012-06-05 11:43:40 PST -------
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 From 2012-06-06 15:29:11 PST -------
Regressions caused by this so far: bug 85275, bug 88428, bug 88436.
------- Comment #11 From 2012-06-07 15:18:10 PST -------
Do you think it's worth continuing to work though the regressions or should we consider backing out this patch?
------- Comment #12 From 2012-06-07 15:29:52 PST -------
(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 From 2012-06-07 15:41:48 PST -------
(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.