Bug 37382 - [Qt] XFrameOptions/x-frame-options-deny-meta-tag* tests have extra favicon.ico request
Summary: [Qt] XFrameOptions/x-frame-options-deny-meta-tag* tests have extra favicon.ic...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-04-10 03:09 PDT by Robert Hogan
Modified: 2010-05-18 10:57 PDT (History)
2 users (show)

See Also:


Attachments
Patch (15.50 KB, patch)
2010-05-08 08:22 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (15.15 KB, patch)
2010-05-17 12:24 PDT, Robert Hogan
kenneth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2010-04-10 03:09:09 PDT
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-in-body.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-parent-same-origin-deny.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag.html

These tests have an extra line in the expected results:
http://127.0.0.1:8000/favicon.ico - willSendRequest <NSURLRequest URL http://127.0.0.1:8000/favicon.ico, main document URL http://127.0.0.1:8000/security/XFrameOptions/x-frame-options-deny-meta-tag-in-body.html, http method GET> redirectResponse (null)

This is probably innocuous?
Comment 1 Robert Hogan 2010-05-08 08:22:56 PDT
Created attachment 55478 [details]
Patch
Comment 2 Robert Hogan 2010-05-08 08:27:09 PDT
Was going to see about supporting dumpDOMAsWebArchive in order to pass LayoutTests/webarchive/test-link-rel-icon.html too but notice that only Mac supports it. Also noticed that Chromium has:


CONSOLE MESSAGE: line 8: Uncaught TypeError: Object [object Object] has no method 'dumpDOMAsWebArchive'
layer at (0,0) size 800x600
  RenderView at (0,0) size 800x600
layer at (0,0) size 800x600
  RenderBlock {HTML} at (0,0) size 800x600
    RenderBody {BODY} at (8,8) size 784x584

as the expected results on Win and Linux for this test. So no intention to support the format it seems.

Should Qt do the same?
Comment 3 Jakub Wieczorek 2010-05-08 13:28:24 PDT
> +        * platform/qt/http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-expected.txt: Added. Qt returns 5 instead of -999.
> +        * platform/qt/http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-in-body-expected.txt: Added. Qt returns 5 instead of -999.
> +        * platform/qt/http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-parent-same-origin-deny-expected.txt: Added. Qt returns 5 instead of -999.

Instead of adding separate test results, you could change FrameLoaderClientQt to rewrite 5 to -999 in the code that prints these callbacks. It should be acceptable as the Windows port seems to be doing a similar thing in their DRT:

// Convert the winsock error code to an NSURLError code.
if (code == WSAEADDRNOTAVAIL)
    code = -1004; // NSURLErrorCannotConnectToHose;

(In reply to comment #2)
> Was going to see about supporting dumpDOMAsWebArchive in order to pass
> LayoutTests/webarchive/test-link-rel-icon.html too but notice that only Mac
> supports it. Also noticed that Chromium has:
> 
> CONSOLE MESSAGE: line 8: Uncaught TypeError: Object [object Object] has no
> method 'dumpDOMAsWebArchive'
> layer at (0,0) size 800x600
>   RenderView at (0,0) size 800x600
> layer at (0,0) size 800x600
>   RenderBlock {HTML} at (0,0) size 800x600
>     RenderBody {BODY} at (8,8) size 784x584
> 
> as the expected results on Win and Linux for this test. So no intention to
> support the format it seems.
> 
> Should Qt do the same?

I don't think there's a point in running tests for a feature that is not implemented and/or completely disabled. On the other hand, it might at least be useful to make sure that they don't crash.
Comment 4 Robert Hogan 2010-05-10 14:51:23 PDT
(In reply to comment #3)
> > +        * platform/qt/http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-expected.txt: Added. Qt returns 5 instead of -999.
> > +        * platform/qt/http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-in-body-expected.txt: Added. Qt returns 5 instead of -999.
> > +        * platform/qt/http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-parent-same-origin-deny-expected.txt: Added. Qt returns 5 instead of -999.
> 
> Instead of adding separate test results, you could change FrameLoaderClientQt to rewrite 5 to -999 in the code that prints these callbacks. It should be acceptable as the Windows port seems to be doing a similar thing in their DRT:
> 
> // Convert the winsock error code to an NSURLError code.
> if (code == WSAEADDRNOTAVAIL)
>     code = -1004; // NSURLErrorCannotConnectToHose;
> 

I think platform-specific results are preferable here. 

> (In reply to comment #2)
> > Was going to see about supporting dumpDOMAsWebArchive in order to pass
> > LayoutTests/webarchive/test-link-rel-icon.html too but notice that only Mac
> > supports it. Also noticed that Chromium has:
> > 
> > CONSOLE MESSAGE: line 8: Uncaught TypeError: Object [object Object] has no
> > method 'dumpDOMAsWebArchive'
> > layer at (0,0) size 800x600
> >   RenderView at (0,0) size 800x600
> > layer at (0,0) size 800x600
> >   RenderBlock {HTML} at (0,0) size 800x600
> >     RenderBody {BODY} at (8,8) size 784x584
> > 
> > as the expected results on Win and Linux for this test. So no intention to
> > support the format it seems.
> > 
> > Should Qt do the same?
> 
> I don't think there's a point in running tests for a feature that is not implemented and/or completely disabled. On the other hand, it might at least be useful to make sure that they don't crash.

Presumably that's where the chromium guys are coming from.

This fix will also unskip LayoutTests/http/tests/misc/favicon-loads-with-images-disabled.html.
Comment 5 Kenneth Rohde Christiansen 2010-05-17 11:16:53 PDT
Comment on attachment 55478 [details]
Patch

r- after talking with Robert on irc.
Comment 6 Robert Hogan 2010-05-17 12:24:08 PDT
Created attachment 56256 [details]
Patch
Comment 7 Robert Hogan 2010-05-17 12:37:12 PDT
Committed r59623: <http://trac.webkit.org/changeset/59623>
Comment 8 Antonio Gomes 2010-05-18 04:51:52 PDT
just a sidenote: it is good when in the  changelog there is a match between the "bug title" and the "fix title"

[Qt] XFrameOptions/x-frame-options-deny-meta-tag* tests have extra favicon.ico request VS [Qt] Disable Icon Database by default in Qt DRT

the later in bugzilla could had catch more attention from others ...

you also did not just disabled IconDatabase but added support for image autoloading ...

(...)
+        [Qt] Disable Icon Database by default in Qt DRT
+
(...)
+
+        https://bugs.webkit.org/show_bug.cgi?id=37382

other than that looks really good to me too
Comment 9 Robert Hogan 2010-05-18 10:57:13 PDT
(In reply to comment #8)
> just a sidenote: it is good when in the  changelog there is a match between the "bug title" and the "fix title"
> 
> [Qt] XFrameOptions/x-frame-options-deny-meta-tag* tests have extra favicon.ico request VS [Qt] Disable Icon Database by default in Qt DRT
> 
> the later in bugzilla could had catch more attention from others ...
> 
> you also did not just disabled IconDatabase but added support for image autoloading ...
> 
> (...)
> +        [Qt] Disable Icon Database by default in Qt DRT
> +
> (...)
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=37382
> 
> other than that looks really good to me too

All points taken. Thanks Antonio!