Bug 52570

Summary: [Gtk] No need to content sniff 304 Not Modified responses
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: WebKitGTKAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: mrobinson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch mrobinson: review+

Description Sergio Villar Senin 2011-01-17 05:48:31 PST
[Gtk] No need to content sniff 304 Not Modified responses
Comment 1 Sergio Villar Senin 2011-01-17 05:53:49 PST
Created attachment 79159 [details]
Patch
Comment 2 Sergio Villar Senin 2011-01-17 05:59:59 PST
Something like this is also done in the Mac port: https://bugs.webkit.org/show_bug.cgi?id=38032
Comment 3 Martin Robinson 2011-01-17 09:35:18 PST
Comment on attachment 79159 [details]
Patch

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

Looks good! If this doesn't change functionality, you should put a note in the ChangeLog where the tests line normally is. If it does change functionality, it might need a test.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:249
> +    if (handle->shouldContentSniff())
> +        // Avoid MIME type sniffing if the response comes back as 304 Not Modified.
> +        if (msg->status_code == SOUP_STATUS_NOT_MODIFIED) {
> +            soup_message_disable_feature(msg, SOUP_TYPE_CONTENT_SNIFFER);
> +            g_signal_handlers_disconnect_by_func(msg, reinterpret_cast<gpointer>(contentSniffedCallback), handle.get());
> +        } else
> +            return;

You should use curly braces with the outer loop since its body is more than one line.
Comment 4 Sergio Villar Senin 2011-01-17 12:47:49 PST
Committed r75967: <http://trac.webkit.org/changeset/75967>
Comment 5 Martin Robinson 2011-01-17 14:46:57 PST
I think this caused a failure in http/tests/inspector/extensions-resources-redirect.html . It doesn't look too worrying, but it might be nice to understand why. Here's the diff:

--- /home/slave/webkitgtk/gtk-linux-64-debug/build/layout-test-results/http/tests/inspector/extensions-resources-redirect-expected.txt	2011-01-17 14:34:29.877532178 -0800
+++ /home/slave/webkitgtk/gtk-linux-64-debug/build/layout-test-results/http/tests/inspector/extensions-resources-redirect-actual.txt	2011-01-17 14:34:29.869142149 -0800
@@ -8,8 +8,8 @@
 
 content: undefined, encoding: undefined
 RUNNING TEST: extension_testRedirectResourceFinished
-Finished resource: http://127.0.0.1:8000/loading/resources/redirect-methods-result.php?redirected=true
 Finished resource: http://127.0.0.1:8000/loading/resources/redirect-methods-result.php?status=302
+Finished resource: http://127.0.0.1:8000/loading/resources/redirect-methods-result.php?redirected=true
 RUNNING TEST: extension_testRedirectResourcesInHAR
 Resources in HAR:
 http://127.0.0.1:8000/inspector/extensions-resources-redirect.html
Comment 6 Martin Robinson 2011-01-17 15:32:27 PST
(In reply to comment #5)
> I think this caused a failure in http/tests/inspector/extensions-resources-redirect.html . It doesn't look too worrying, but it might be nice to understand why. Here's the diff:

Update: This test may just be flaky.