Bug 52570 – [Gtk] No need to content sniff 304 Not Modified responses

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Sergio Villar Senin]

[Gtk] No need to content sniff 304 Not Modified responses

[Sergio Villar Senin]

Created attachment 79159 [details]
Patch

[Sergio Villar Senin]

Something like this is also done in the Mac port: https://bugs.webkit.org/show_bug.cgi?id=38032

[Martin Robinson]

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.

[Sergio Villar Senin]

Committed r75967: <http://trac.webkit.org/changeset/75967>

[Martin Robinson]

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

[Martin Robinson]

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