Bug 176481 – [WK2] Notify client when downloads are redirected

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

We should notify client when downloads are redirected. The client has currently no way of knowing.

[Chris Dumez]

Created attachment 320078 [details]
Patch

[Build Bot]

Comment on attachment 320078 [details]
Patch

Attachment 320078 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4469115

New failing tests:
http/tests/download/anchor-download-redirect.html

[Build Bot]

Created attachment 320084 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320078 [details]
Patch

Attachment 320078 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4469201

New failing tests:
http/tests/download/anchor-download-redirect.html

[Build Bot]

Created attachment 320087 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Chris Dumez]

Created attachment 320089 [details]
Patch

[Chris Dumez]

Created attachment 320090 [details]
Patch

[Chris Dumez]

Created attachment 320095 [details]
Patch

[Chris Dumez]

<rdar://problem/34309065>

[Chris Dumez]

Created attachment 320126 [details]
Patch

[Geoffrey Garen]

Comment on attachment 320126 [details]
Patch

The API is called "...ClientRedirect...". What does the client see for server redirects?

[Chris Dumez]

(In reply to Geoffrey Garen from comment #11)
> Comment on attachment 320126 [details]
> Patch
> 
> The API is called "...ClientRedirect...". What does the client see for
> server redirects?

Hmm. I think this is badly named. We notify the client of HTTP server redirects.

[Chris Dumez]

(In reply to Chris Dumez from comment #12)
> (In reply to Geoffrey Garen from comment #11)
> > Comment on attachment 320126 [details]
> > Patch
> > 
> > The API is called "...ClientRedirect...". What does the client see for
> > server redirects?
> 
> Hmm. I think this is badly named. We notify the client of HTTP server
> redirects.

The equivalent delegate for navigation is named:
didReceiveServerRedirectForProvisionalNavigation

[Chris Dumez]

(In reply to Chris Dumez from comment #13)
> (In reply to Chris Dumez from comment #12)
> > (In reply to Geoffrey Garen from comment #11)
> > > Comment on attachment 320126 [details]
> > > Patch
> > > 
> > > The API is called "...ClientRedirect...". What does the client see for
> > > server redirects?
> > 
> > Hmm. I think this is badly named. We notify the client of HTTP server
> > redirects.
> 
> The equivalent delegate for navigation is named:
> didReceiveServerRedirectForProvisionalNavigation

What do you think about calling the delegate: "didReceiveServerRedirect" ?

[Chris Dumez]

(In reply to Chris Dumez from comment #14)
> (In reply to Chris Dumez from comment #13)
> > (In reply to Chris Dumez from comment #12)
> > > (In reply to Geoffrey Garen from comment #11)
> > > > Comment on attachment 320126 [details]
> > > > Patch
> > > > 
> > > > The API is called "...ClientRedirect...". What does the client see for
> > > > server redirects?
> > > 
> > > Hmm. I think this is badly named. We notify the client of HTTP server
> > > redirects.
> > 
> > The equivalent delegate for navigation is named:
> > didReceiveServerRedirectForProvisionalNavigation
> 
> What do you think about calling the delegate: "didReceiveServerRedirect" ?

Or didReceiveServerRedirectToURL

[Chris Dumez]

Created attachment 320145 [details]
Patch

[Geoffrey Garen]

> What do you think about calling the delegate: "didReceiveServerRedirect" ?

I like didReceiveServerRedirect.

I would leave off "ToURL" because the other names in this group leave off their direct objects as well.

[Chris Dumez]

Comment on attachment 320145 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKDownloadDelegate.h:37
> +- (void)_download:(_WKDownload *)download didReceiveServerRedirectToURL:(NSURL *)url WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

How about for the ObC API. Do you also prefer to omit the "toURL" suffix?

It seems other delegates do provide the name for the parameter, e.g.:
- (void)_download:(_WKDownload *)download didFailWithError:(NSError *)error;
- (NSString *)_download:(_WKDownload *)download decideDestinationWithSuggestedFilename:(NSString *)filename allowOverwrite:(BOOL *)allowOverwrite;
- (void)_download:(_WKDownload *)download didReceiveResponse:(NSURLResponse *)response;

[Chris Dumez]

Created attachment 320150 [details]
Patch

[Geoffrey Garen]

> > Source/WebKit/UIProcess/API/Cocoa/_WKDownloadDelegate.h:37
> > +- (void)_download:(_WKDownload *)download didReceiveServerRedirectToURL:(NSURL *)url WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> How about for the ObC API. Do you also prefer to omit the "toURL" suffix?

A related API in LegacyWebKit is named "willPerformClientRedirectToURL...".

I like ToURL for the ObjC API. It's a little more Cocoa-y to name your direct object (as you pointed out too).

[Chris Dumez]

Created attachment 320152 [details]
Patch

[Geoffrey Garen]

Comment on attachment 320152 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 320152 [details]
Patch

Clearing flags on attachment: 320152

Committed r221749: <http://trac.webkit.org/changeset/221749>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.