Bug 192021 – Add SPI to publish NSProgress on active downloads

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[David Quesada]

<rdar://problem/44405661>

[David Quesada]

Created attachment 355968 [details]
Patch

[David Quesada]

Created attachment 355971 [details]
Patch v2

Try to fix the mac-wk2 build.

[David Quesada]

Created attachment 355976 [details]
Patch v3

Attempted build fixes for ios, ios-sim, and mac-wk2

[David Quesada]

Created attachment 355981 [details]
Patch v4

Add the _addSubscriber… and _removeSubscriber: methods to NSProgressSPI.h

[Alex Christensen]

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.

> 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?

> 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*

[David Quesada]

(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*

[David Quesada]

Created attachment 356020 [details]
Patch v5

w/ Alex's suggested changes

[WebKit Commit Bot]

Comment on attachment 356020 [details]
Patch v5

Clearing flags on attachment: 356020

Committed r238710: <https://trac.webkit.org/changeset/238710>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.