Bug 83908 – Move more of committing and starting to write a Document to DocumentLoader

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

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.

[Nate Chapin]

Created attachment 137103 [details]
patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Nate Chapin]

Comment on attachment 137103 [details]
patch

Yeah, those test failures are legitimate, removing review?

[Nate Chapin]

Created attachment 137176 [details]
Fix test failures

[Adam Barth]

Comment on attachment 137176 [details]
Fix test failures

This looks great.

[WebKit Review Bot]

Comment on attachment 137176 [details]
Fix test failures

Clearing flags on attachment: 137176

Committed r115654: <http://trac.webkit.org/changeset/115654>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Alexey Proskuryakov]

Regressions caused by this so far: bug 85275, bug 88428, bug 88436.

[Adam Barth]

Do you think it's worth continuing to work though the regressions or should we consider backing out this patch?

[Nate Chapin]

(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 :)

[Brady Eidson]

(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.