Bug 35300

Looks like in GTK the second redirection doesn't even work (I get a timeout). I'm taking a look at this.

So with the changes from bug 88961, we now make the correct series of requests. However, the test still fails because DRT doesn't print the right strings on redirect. Eg, -expected has: http://127.0.0.1:8000/loading/resources/post-to-303-target.php - willSendRequest <NSURLRequest URL http://127.0.0.1:8000/loading/resources/303-to-307-target.php, main document URL http://127.0.0.1:8000/loading/resources/303-to-307-target.php, http method GET> redirectResponse <NSURLResponse http://127.0.0.1:8000/loading/resources/post-to-303-target.php, http status code 303> but we output: http://127.0.0.1:8000/loading/resources/303-to-307-target.php - willSendRequest <NSURLRequest URL http://127.0.0.1:8000/loading/resources/303-to-307-target.php, main document URL http://127.0.0.1:8000/loading/resources/303-to-307-target.php, http method GET> redirectResponse <NSURLResponse http://127.0.0.1:8000/loading/resources/post-to-303-target.php, http status code 303> (The first URL is wrong.)

EFL has a similar bug (https://bugs.webkit.org/show_bug.cgi?id=93214) for which I submitted a patch. As the code is shared with Gtk (libsoup backend), this patch may be of interest. I actually ran DRT GTK with and without the patch. Without the patch, the last request is a POST (possible regression?). With the patch, the last request is a GET, as expected. As of the DRT URL bug, the WebKitWebResource private uri field is changed in FrameLoaderClient::dispatchWillSendRequest. I was wondering whether it would be a more appropriate model to keep this field unchanged accross redirections? If so, with a small change to willSendRequestCallback, this bug would probably be fixed. I can provide a patch along those lines.

Created attachment 196679 [details] Patch (In reply to comment #3) > As of the DRT URL bug, the WebKitWebResource private uri field is > changed in FrameLoaderClient::dispatchWillSendRequest. > I was wondering whether it would be a more appropriate model to keep > this field unchanged accross redirections? Yes, this actually seems to be the problem. Do we need to change that field? The identifier is supposed to be associated to the initial request. It works fine if we do it like that, here's the patch, I tried all http/tests and it doesn't seem to break anything.

Attachment 196679 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk-wk1/TestExpectations', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp', u'Tools/ChangeLog', u'Tools/DumpRenderTree/gtk/DumpRenderTree.cpp']" exit_code: 1 Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1284: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

One other possibility if we don't want to change the current behavior is to add a new 'original-uri' property to the WebKitWebResource class. I can easily prepare a patch. (In reply to comment #4) > Do we need to change that field? The identifier is supposed to be > associated to the initial request. One thing to note, though, is that we're currently updating the uri property (which is read-only) by directly accessing the private structure from a different class... Gustavo wrote the original code (bug 29134), adding him to Cc.

Attachment 196679 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk-wk1/TestExpectations', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp', u'Tools/ChangeLog', u'Tools/DumpRenderTree/gtk/DumpRenderTree.cpp']" exit_code: 1 Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1284: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #6) > One other possibility if we don't want to change the current behavior > is to add a new 'original-uri' property to the WebKitWebResource > class. Does that new property be useful outside this test? If created for the purpose of the test, I would avoid doing that. A temporary workaround may be to create an expected file for that test. We would at least check that there is no regression in the POST-303-307 behavior. > One thing to note, though, is that we're currently updating the uri > property (which is read-only) by directly accessing the private > structure from a different class... Yes, when reading that code, I thought it might be better to encapsulate this as a function call, if this code is to remain.

Comment on attachment 196679 [details] Patch Yeah, the URI is to remain read-only for API users, it should only be updated by internal code, that is why it's done the way it is. I'm OK with adding an internal call to update it instead of poking the private structure, though. I think adding a new property for original-uri is a good idea, since it maintains the API expectations while still providing complete information. All that said, wk1 is in maintenance mode and there was a decision not to add new API to it unless strictly necessary, so I wonder, do we really want to fix this test at this point? r- for the change in API expectation, adding a new property sounds like a better solution.

(In reply to comment #9) > All that said, wk1 is in maintenance mode and there was a decision > not to add new API to it unless strictly necessary, so I wonder, do > we really want to fix this test at this point? I think you're right, actually.

Closing since this looks like an old WK1 bug.