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

[bugzilla-daemon at webkit.org]

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

--- Comment #5 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 355981
  --> https://bugs.webkit.org/attachment.cgi?id=355981
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*

-- 
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/70784ceb/attachment-0001.html>