Bug 163436 - Document request not updated after willSendRequest is called for a redirect
Summary: Document request not updated after willSendRequest is called for a redirect
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 163389
  Show dependency tree / graph
 
Reported: 2016-10-14 01:55 PDT by Carlos Garcia Campos
Modified: 2016-10-16 01:38 PDT (History)
8 users (show)

See Also:

Attachments
Patch (11.75 KB, patch)
2016-10-14 02:02 PDT, Carlos Garcia Campos 		mcatanzaro: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2016-10-14 01:55:08 PDT 
Adding unit tests for bug #146306 in the GTK+ port I've noticed that patch for bug #146306 still doesn't work when a request is modified by willSendRequest after a redirect. The thing is that DocumentLoader::setRequest is not called after willSendRequest is called for a redirect. The first willSendRequest happens before DocumentLoader::startLoadingMainResource, that calls setRequest, but the second one happens after DocumentLoader::redirectReceived() and then the request is never updated again.
Comment 1 Carlos Garcia Campos 2016-10-14 02:02:32 PDT 
Created attachment 291595 [details]
Patch

Added unit tests for GTK+ only, because I'm not sure it's even possible to test a server redirect with the cross-platform C API unit tests.
Comment 2 Michael Catanzaro 2016-10-14 11:15:22 PDT 
(In reply to comment #1)
> Created attachment 291595 [details]
> Patch
> 
> Added unit tests for GTK+ only, because I'm not sure it's even possible to
> test a server redirect with the cross-platform C API unit tests.

I think it's not currently possible, because we have no cross-platform equivalent to SoupServer.
Comment 3 Michael Catanzaro 2016-10-14 11:31:04 PDT 
Comment on attachment 291595 [details]
Patch

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

Great tests.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestLoaderClient.cpp:225
> +    enum State { Provisional, ProvisionalAfterRedirect, Commited, Finished };

Personally, I'd still favor using enum class even in a file-private unit test class, but your call.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestLoaderClient.cpp:254
> +    void checkURIAtState(State state, const char* uri)

Please rename the second parameter (maybe to "path") because it's not a URI.
Comment 4 Carlos Garcia Campos 2016-10-16 01:38:08 PDT 
Committed r207388: <http://trac.webkit.org/changeset/207388>