Summary: | [WK2] Notify client when downloads are redirected | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, buildbot, commit-queue, ggaren, rniwa, webkit-bug-importer | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2017-09-06 16:36:08 PDT
Created attachment 320078 [details]
Patch
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 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
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 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
Created attachment 320089 [details]
Patch
Created attachment 320090 [details]
Patch
Created attachment 320095 [details]
Patch
Created attachment 320126 [details]
Patch
Comment on attachment 320126 [details]
Patch
The API is called "...ClientRedirect...". What does the client see for server redirects?
(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. (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 (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" ? (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 Created attachment 320145 [details]
Patch
> 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.
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; Created attachment 320150 [details]
Patch
> > 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).
Created attachment 320152 [details]
Patch
Comment on attachment 320152 [details]
Patch
r=me
Comment on attachment 320152 [details] Patch Clearing flags on attachment: 320152 Committed r221749: <http://trac.webkit.org/changeset/221749> All reviewed patches have been landed. Closing bug. |