Bug 143612 – [Mac] "Save to Downloads" does nothing for local images

• Component: WebKit Misc.
• Status: NEW

[Brent Fulgham]

The 'Save to Downloads' menu option does nothing when you are viewing a local image in the browser.

[Brent Fulgham]

Created attachment 250524 [details]
Patch

[Brent Fulgham]

<rdar://problem/18858089>

[WebKit Commit Bot]

Attachment 250524 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebView.mm:1851:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/mac/WebView/WebView.mm:1853:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:479:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:481:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Comment on attachment 250524 [details]
Patch

After talking to thorton and andersca about this, we will move this away from Download code into the menu handling or view.

[Brent Fulgham]

Created attachment 250525 [details]
Patch

[WebKit Commit Bot]

Attachment 250525 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebView.mm:1851:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/mac/WebView/WebView.mm:1853:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:479:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:481:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

(In reply to comment #4)
> Comment on attachment 250524 [details]
> Patch
> 
> After talking to thorton and andersca about this, we will move this away
> from Download code into the menu handling or view.

After looking at this, I think it's better to leave it in the WebProcessPool sources, because there are a number of entry points that use the "processPool().download(...)" method to invoke this operation.

I would have to add a test for the URL as a local file in each of the six places this happens, and dispatch to the "local file copy" or the original download method. The current patch hides that inside the download method.

Six locations:
1. Save image locally (Action menu)
2. Save image locally (Context menu)
3. Save media locally (Action menu)
4. Save media locally (Context menu)
5. Save link locally (Action menu)
6. Save link locally (Context menu).

[Brent Fulgham]

Created attachment 250531 [details]
Patch v2 (iOS Build Fix)

[Brent Fulgham]

Comment on attachment 250531 [details]
Patch v2 (iOS Build Fix)

The iOS build doesn't like NSInteger used as a format argument. I switched to using 'int' values for these simple counters.

[WebKit Commit Bot]

Attachment 250531 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebView.mm:1851:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/mac/WebView/WebView.mm:1853:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:479:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:481:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

(In reply to comment #10)
> Attachment 250531 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebKit/mac/WebView/WebView.mm:1851:  Weird number of spaces at
> line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> ERROR: Source/WebKit/mac/WebView/WebView.mm:1853:  Weird number of spaces at
> line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> ERROR: Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:479:  Weird
> number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> ERROR: Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:481:  Weird
> number of spaces at line-start.  Are you using a 4-space indent? 
> [whitespace/indent] [3]
> Total errors found: 4 in 6 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

This is just Objective C formatting.

[Anders Carlsson]

Comment on attachment 250531 [details]
Patch v2 (iOS Build Fix)

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

> Source/WebKit/mac/WebView/WebView.mm:1847
> +static void performLocalFileDownload(NSURL* sourceURL)

I think this should be called "save to downloads" or something along those lines.

[Brent Fulgham]

Created attachment 250545 [details]
Patch

[WebKit Commit Bot]

Attachment 250545 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebView/WebView.mm:1851:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/mac/WebView/WebView.mm:1853:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:479:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:481:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 250545 [details]
Patch

r=me but I don’t think this is quite right.

Did you test the case where the filename is already the maximum length and we can’t append the hyphen and number? What do we do in that case? I’m guessing we’ll just run an infinite loop.

Can we put this function somewhere we can use it in both cases so we don’t have to have two copies?

What prevents the app from being quit while it is still writing the file if it’s a long file that takes a long time to copy? I am guessing that in that case we’ll leave a partial file ("damaged image") in the file system. Also, do we need to do anything to prevent sudden termination while writing the file?

[Brent Fulgham]

(In reply to comment #15)
> Comment on attachment 250545 [details]
> Patch
> 
> r=me but I don’t think this is quite right.
> 
> Did you test the case where the filename is already the maximum length and
> we can’t append the hyphen and number? What do we do in that case? I’m
> guessing we’ll just run an infinite loop.

The loop only iterates if the error is that the file already exists. I'll have to test whether attempting to copy to a file name that is longer than the max length (and we have an existing file name that matches the proposed name -- up to the limit) fails with the 'file exists' error, or a different error (e.g., file name is too long).

> Can we put this function somewhere we can use it in both cases so we don’t
> have to have two copies?

Yes. It seems like this should be something attached to the existing FileName class.

> What prevents the app from being quit while it is still writing the file if
> it’s a long file that takes a long time to copy? I am guessing that in that
> case we’ll leave a partial file ("damaged image") in the file system. Also,
> do we need to do anything to prevent sudden termination while writing the
> file?

It seems like the downside of early termination is low -- the file already exists on the user's disk, so it's not as if some ephemeral resource could be lost forever.

However, I might want to use the 'atomic' flag on the file copy, so the OS only places the file in the final location if the copy completes. I can make that change as well.

[Brent Fulgham]

Created attachment 250622 [details]
Patch (revised per drain's comments)

[WebKit Commit Bot]

Attachment 250622 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:107:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:109:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:159:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:160:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:161:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:162:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:163:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:164:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 8 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Comment on attachment 250622 [details]
Patch (revised per drain's comments)

Updated patch to deal with:
1. Avoid partial/corrupt file copies by treating operations atomically:
(a) Initial copy to temp directory. If it fails, nothing is lost.
(b) Final move from temp directory to Downloads is an atomic operation.
2. Check for exceeding max path length.

[Brent Fulgham]

(In reply to comment #15)
> Comment on attachment 250545 [details]
> Patch
> 
> r=me but I don’t think this is quite right.

I uploaded a new patch -- but it has enough changes that I'd appreciate you giving it another look.

Thanks!

-Brent

[Brent Fulgham]

Created attachment 250641 [details]
Patch

[WebKit Commit Bot]

Attachment 250641 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:107:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:109:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:159:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:160:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:161:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:162:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:163:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:164:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 8 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

(In reply to comment #22)
> Attachment 250641 [details] did not pass style-queue:
> 
These are just complaints about our Objective C indentation style.

[Darin Adler]

Comment on attachment 250641 [details]
Patch

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

I know I already reviewed this once, but I think what we have here is the wrong approach. Safari, at least, doesn’t always download to the system downloads directory. I don’t think this can be done correctly without involving the host application. I think we want to work with the existing API or add some new API so we can plug this in to the existing downloads code in WebKit apps.

> Source/WebCore/platform/FileSystem.h:207
> +#if PLATFORM(MAC)

I like to keep the platform-specific stuff sorted in alphabetical order. So I would put MAC after GTK and before SOUP. And add a blank line for SOUP so it’s got its own paragraph.

> Source/WebCore/platform/FileSystem.h:208
> +class URL;

This forward declaration can be unconditional and be at the top of the file.

> Source/WebCore/platform/FileSystem.h:209
> +WEBCORE_EXPORT void saveToDownloadFolder(const URL&);

It’s a little peculiar that this uses URL and the rest of the file system stuff uses Strings for paths, but it’s certainly cleaner code this way. Since this is Mac-specific anyway, why not have it take an NSURL * directly?

> Source/WebCore/platform/FileSystem.h:211
>  } // namespace WebCore

Should add a blank line here.

> Source/WebCore/platform/mac/FileSystemMac.mm:119
> +    NSError *error = nil;
> +    NSURL *downloadDirectoryURL = [[NSFileManager defaultManager] URLForDirectory:NSDownloadsDirectory
> +                                                                         inDomain:NSUserDomainMask
> +                                                                appropriateForURL:nil
> +                                                                           create:NO
> +                                                                            error:&error];
> +    if (!downloadDirectoryURL) {
> +        LOG_ERROR("Unable to locate User's Download directory. %@", error);
> +        return;
> +    }
> +
> +    NSString *targetName = sourceURL.lastPathComponent;
> +    NSString *extension = sourceURL.pathExtension;
> +    NSInteger extensionLength = extension ? extension.length + 1 : 0;
> +    NSString *baseFileName = [targetName substringToIndex:targetName.length - extensionLength];

Should do all of this on the other thread too. No reason to do it on the main thread.

> Source/WebCore/platform/mac/FileSystemMac.mm:133
> +        // Copy file to temporary location:

It’s nice to do this atomically. But to get this right we also would want to make sure we don’t lose the file if we just happen to quit shortly after starting to write it. That can be done by synchronously waiting for the work to complete during app exit and by preventing sudden termination. This is done in IconDatabase, for example, although I’m not sure it will work properly when the work is done in the web process rather than the UI process; for our case, though, it’s the UI process. Outside of WebKit there is something I wrote called CoalescedAsynchronousWriter that deals with this too, but I can’t think of other code in WebKit that does this. To make it work we’d need to add a separate FileSystem.h function to call at quit time.

> Source/WebCore/platform/mac/FileSystemMac.mm:138
> +            LOG_ERROR("Error writing %@ to temporary location %@: %@", sourceURL, tempURL, copyError);
> +            return;

Not good to fail silently.

> Source/WebCore/platform/mac/FileSystemMac.mm:146
> +                return;

Not good to fail silently. Also, it will take a very long time to count up to 2^32, so this will probably hang the queue for a long time if this is happening.

> Source/WebCore/platform/mac/FileSystemMac.mm:154
> +            if (copyURL.absoluteString.length >= PATH_MAX) {
> +                LOG_ERROR("File path '%@' 'exceeds system path length. Unable to perform download.", copyURL);
> +                return;
> +            }

I don’t think this check is correct. For one thing, it’s checking the length of a URL against the path maximum, and the URL is longer than the file system path inside the URL. For another, PATH_MAX is the limit of the file system API, but not necessarily the actual practical limit of the file system itself. For example, I think there’s a 255 UTF-16 code unit limit on file names on HFS+ volumes. Not on the entire path, but just on the file name.

It’s also not good to have this fail silently. The real downloading machinery puts up a sheet if it fails.

> Source/WebCore/platform/mac/FileSystemMac.mm:166
> +            LOG_ERROR("Error copying '%@' from temporary location '%@' to final target '%@': %@", sourceURL, tempURL, copyURL, copyError);
> +            return;

Not good to fail silently.

> Source/WebKit/mac/WebView/WebView.mm:1852
> +    WebCore::URL coreUrl(URL);

WebKit style would be coreURL, not coreUrl.

[Brent Fulgham]

Comment on attachment 250641 [details]
Patch

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

>> Source/WebCore/platform/FileSystem.h:207
>> +#if PLATFORM(MAC)
> 
> I like to keep the platform-specific stuff sorted in alphabetical order. So I would put MAC after GTK and before SOUP. And add a blank line for SOUP so it’s got its own paragraph.

OK!

>> Source/WebCore/platform/FileSystem.h:208
>> +class URL;
> 
> This forward declaration can be unconditional and be at the top of the file.

OK!

>> Source/WebCore/platform/FileSystem.h:209
>> +WEBCORE_EXPORT void saveToDownloadFolder(const URL&);
> 
> It’s a little peculiar that this uses URL and the rest of the file system stuff uses Strings for paths, but it’s certainly cleaner code this way. Since this is Mac-specific anyway, why not have it take an NSURL * directly?

Sure, I could use an NSURL instead. Done!

>> Source/WebCore/platform/FileSystem.h:211
>>  } // namespace WebCore
> 
> Should add a blank line here.

OK!

>> Source/WebCore/platform/mac/FileSystemMac.mm:119
>> +    NSString *baseFileName = [targetName substringToIndex:targetName.length - extensionLength];
> 
> Should do all of this on the other thread too. No reason to do it on the main thread.

OK!

>> Source/WebKit/mac/WebView/WebView.mm:1852
>> +    WebCore::URL coreUrl(URL);
> 
> WebKit style would be coreURL, not coreUrl.

OK!

[Brent Fulgham]

Created attachment 250774 [details]
Patch

[WebKit Commit Bot]

Attachment 250774 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:107:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:109:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:165:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:166:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:167:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:168:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:169:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:170:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 8 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Created attachment 250775 [details]
Patch (v4 More improvements)

[Brent Fulgham]

Comment on attachment 250775 [details]
Patch (v4 More improvements)

This change includes a mechanism to give feedback to the caller (and delegates). This is sufficient, for example, to allow a registered download client to see that the download (file copy) failed due to disk space or other issues.

[WebKit Commit Bot]

Attachment 250775 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:107:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:109:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:165:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:166:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:167:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:168:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:169:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/mac/FileSystemMac.mm:170:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 8 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 250775 [details]
Patch (v4 More improvements)

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

We discussed these comments and many others in your office. This is getting closer to the right approach but still not something we should land.

> Source/WebKit2/UIProcess/WebProcessPool.cpp:883
> +        WebCore::saveToDownloadFolder(request.url(), ^(bool success, String path, NSError *error) {

These should use C++ lambdas instead of Objective-C blocks so we can see what is captured and make sure there are no threading mistakes. You can pass a lambda to a function that takes a block in Objective-C++.

> Source/WebKit2/UIProcess/WebProcessPool.cpp:885
> +            dispatch_async(dispatch_get_main_queue(), ^() {

These should use C++ lambdas instead of Objective-C blocks so we can see what is captured and make sure there are no threading mistakes. You can pass a lambda to a function that takes a block in Objective-C++.

> Source/WebKit/mac/WebView/WebView.mm:1861
> +        WebCore::saveToDownloadFolder(URL, ^(bool success, String destination, NSError *error) {

These should use C++ lambdas instead of Objective-C blocks so we can see what is captured and make sure there are no threading mistakes. You can pass a lambda to a function that takes a block in Objective-C++.

> Source/WebKit/mac/WebView/WebView.mm:1863
> +            dispatch_async(dispatch_get_main_queue(), ^{

These should use C++ lambdas instead of Objective-C blocks so we can see what is captured and make sure there are no threading mistakes. You can pass a lambda to a function that takes a block in Objective-C++.

> Source/WebKit/mac/WebView/WebView.mm:1865
> +                    [_private->downloadDelegate download:download didCreateDestination:destination];

I don’t think it’s safe to capture the String in this block. Instead I think you need to convert it to a RetainPtr<NSString> before calling dispatch_async, and capture that.

Similarly, I think we need to capture a RetainPtr<WebDownload>, not a raw pointer to a WebDownload, to guarantee we won’t be accessing a deleted object. Same thing for self.

[Jon Lee]

<rdar://problem/20644982>