Bug 104969 – Merge MainResourceLoader into DocumentLoader

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

[Nate Chapin]

With bug 49246 landed, MainResourceLoader's purpose is no longer clearly defined, and is poorly named.  It isn't a ResourceLoader subclass (it implements the CachedRawResourceClient interface), and the organization of what DocumentLoader and MainResourceLoader each do is history rather than logical.  It's probably worth merging them into a single class.

[Nate Chapin]

Created attachment 188173 [details]
Monolithic version

This monster patch is my current thought for merging these 2 classes.  I'll split it up before asking for review, just posting to get EWS data.

[WebKit Review Bot]

Comment on attachment 188173 [details]
Monolithic version

Attachment 188173 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16514332

[WebKit Review Bot]

Comment on attachment 188173 [details]
Monolithic version

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 188173 [details]
Monolithic version

Attachment 188173 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16514438

[Nate Chapin]

Created attachment 193949 [details]
Finish the merger

[Adam Barth]

Comment on attachment 193949 [details]
Finish the merger

View in context: https://bugs.webkit.org/attachment.cgi?id=193949&action=review

Amazing.

> Source/WebCore/loader/DocumentLoader.cpp:1390
> +    DEFINE_STATIC_LOCAL(ResourceLoaderOptions, mainResourceLoadOptions,
> +        (SendCallbacks, SniffContent, BufferData, AllowStoredCredentials, AskClientForCrossOriginCredentials, SkipSecurityCheck));

I'm surprised we use a static here.  This is just a struct...

[WebKit Review Bot]

Comment on attachment 193949 [details]
Finish the merger

Clearing flags on attachment: 193949

Committed r146301: <http://trac.webkit.org/changeset/146301>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

It appears that this patch broke Windows (Debug) Tests:
http://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/50526
http://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/50527

[Ryosuke Niwa]

To make sure this is a real regression, I've ran the tests at the same revision on two distinct machines (apple-windows-3 and apple-windows-4 respectively):
http://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/50528
http://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/50529

[WebKit Review Bot]

Re-opened since this is blocked by bug 112776

[Ryosuke Niwa]

(In reply to comment #10)
> To make sure this is a real regression, I've ran the tests at the same revision on two distinct machines (apple-windows-3 and apple-windows-4 respectively):
> http://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/50528
> http://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/50529

I'll also note that I've rebooted those bots prior to running the tests.

[Ryosuke Niwa]

It appears that this patch caused assertions to be hit on almost all tests:

animations/additive-transform-animations.html		
animations/animation-add-events-in-handler.html		
animations/animation-border-overflow.html		
animations/animation-controller-drt-api.html		
animations/animation-css-rule-types.html		
animations/animation-delay-changed.html		
animations/animation-direction-alternate-reverse.html		
animations/animation-direction-reverse-fill-mode-hardware.html		
animations/animation-direction-reverse-fill-mode.html		
animations/animation-direction-reverse-hardware-opacity.html		
animations/animation-direction-reverse-hardware.html		
animations/animation-direction-reverse-non-hardware.html		
animations/animation-direction-reverse-timing-functions-hardware.html		
animations/animation-direction-reverse-timing-functions.html		
animations/animation-direction-reverse.html		
animations/animation-direction.html		
animations/animation-end-event-destroy-renderer.html		
animations/animation-end-event-short-iterations.html		
animations/animation-events-create.html		
animations/animation-hit-test-transform.html

[Ryosuke Niwa]

I'm sorry but I'm rolling out the patch for now as we're losing the entire test coverage on Windows Debug.

[Adam Barth]

Thanks.

[Ryosuke Niwa]

It appears that this was a build corruption issue.

http://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/50532
is the run that contains the rollout and this looks better but still a bunch of tests have failed. I've triggered a clean build after this as http://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/50536 and this one appears to be running just fine.

Can we coordinate tomorrow to reland this patch so that we can trigger a clean build as needed tomorrow?  Alternatively, you don't do anything, I'll try to reland it tomorrow morning and start a clean build as needed.

[Nate Chapin]

(In reply to comment #16)
> It appears that this was a build corruption issue.
> 
> http://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/50532
> is the run that contains the rollout and this looks better but still a bunch of tests have failed. I've triggered a clean build after this as http://build.webkit.org/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/50536 and this one appears to be running just fine.
> 
> Can we coordinate tomorrow to reland this patch so that we can trigger a clean build as needed tomorrow?  Alternatively, you don't do anything, I'll try to reland it tomorrow morning and start a clean build as needed.

If you want me around, it should probably wait until Thursday, as I'm OOO today and won't have IRC access until this afternoon.

[Ryosuke Niwa]

Committed r146449: <http://trac.webkit.org/changeset/146449>

[Ryosuke Niwa]

Sorry about the unwanted noise.

I've relanded your patch in http://trac.webkit.org/changeset/146449 after cleaning WebKitBuild directories in apple-windows-1 and apple-windows-2.

[Eric Seidel (no email)]

I'm very excited to see this land!

[Nate Chapin]

(In reply to comment #19)
> Sorry about the unwanted noise.
> 
> I've relanded your patch in http://trac.webkit.org/changeset/146449 after cleaning WebKitBuild directories in apple-windows-1 and apple-windows-2.

Thanks for relanding this! I really appreciate it.