[Webkit-unassigned] [Bug 192021] Add SPI to publish NSProgress on active downloads

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Nov 29 09:56:33 PST 2018


https://bugs.webkit.org/show_bug.cgi?id=192021

--- Comment #6 from David Quesada <david_quesada at apple.com> ---
(In reply to Alex Christensen from comment #5)
> Comment on attachment 355981 [details]
> Patch v4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355981&action=review
> 
> > Source/WebKit/NetworkProcess/Downloads/DownloadManager.cpp:165
> > +    if (Download* download = m_downloads.get(downloadID))
> 
> auto*
> 
> > Source/WebKit/NetworkProcess/Downloads/cocoa/WKDownloadProgress.h:44
> > +- (instancetype)initWithDownloadTask:(NSURLSessionDownloadTask *)task download:(WebKit::Download *)download URL:(NSURL *)fileURL sandboxExtension:(WTF::RefPtr<WebKit::SandboxExtension>)sandboxExtension;
> 
> WTF:: should be unnecessary.
> 
> > Source/WebKit/NetworkProcess/Downloads/cocoa/WKDownloadProgress.mm:57
> > +#if !PLATFORM(MAC) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300
> 
> We're trying to give platform checks names in WebKit now.

Sure. Would adding

#define HAVE_NSPROGRESS_FILE_PROPERTIES (!PLATFORM(MAC) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300)

to the top of the file be ok?

> 
> > Source/WebKit/NetworkProcess/Downloads/cocoa/WKDownloadProgress.mm:62
> > +    [self setUserInfoObject:fileURL forKey:NSProgressFileURLKey];
> 
> Documentation is a little lacking in
> https://developer.apple.com/documentation/foundation/
> nsprogressfileurlkey?language=objc and
> https://developer.apple.com/documentation/foundation/nsprogress/1416782-
> publish?language=objc
> Could you help me understand what this file URL is used for?

In order to make an NSProgress available across processes, it needs to have an associated file URL to look it up. e.g. if process A says "I'm operating on file:///foo and file:///bar right now" and process B says "I want to display the progress of any operations on file:///bar", then the system will give process B an NSProgress that acts as a proxy to the NSProgress that process A published on file:///bar. (i.e. Updates that process A makes to the process will be sent to process B; and process B can -cancel, -pause, or -resume its NSProgress, and the system will invoke handlers on process A's NSProgress).

Without the file URLs (or other types of identifiers that would serve a similar purpose), the workflow above would be Process A saying "I'm operating on two things right now, but I'm not going to say what they are.", Process B saying "Let me know if somebody is working on a thing.", and the system having no idea what process B wants to be notified of or who process A is trying to share its progresses with.

> 
> > Source/WebKit/NetworkProcess/Downloads/cocoa/WKDownloadProgress.mm:66
> > +    WeakObjCPtr<WKDownloadProgress> weakSelf { self };
> 
> This can be inside the lambda capture.
> 
> > Source/WebKit/NetworkProcess/Downloads/cocoa/WKDownloadProgress.mm:74
> > +        WebKit::Download* download = strongSelf.get()->m_download;
> > +        if (download)
> 
> if (auto* download = ...)
> 
> > Source/WebKit/NetworkProcess/Downloads/cocoa/WKDownloadProgress.mm:116
> > +    [m_task.get() removeObserver:self forKeyPath:@"countOfBytesExpectedToReceive"];
> > +    [m_task.get() removeObserver:self forKeyPath:@"countOfBytesReceived"];
> 
> These strings should be given a name and instantiated once.
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKDownload.mm:52
> > +- (void)publishProgressAtURL:(NSURL *)URL WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA))
> 
> Availability macros only go in the header.
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKDownload.mm:54
> > +    _download->publishProgress({ URL });
> 
> initializer list is probably unnecessary.
> 
> > Source/WebKit/UIProcess/Downloads/DownloadProxy.cpp:110
> > +    if (NetworkProcessProxy* networkProcess = m_processPool->networkProcess()) {
> 
> auto*

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20181129/e5ac965d/attachment.html>


More information about the webkit-unassigned mailing list