Bug 234803 - Undownloaded iCloud Photos are inserted as broken images when attachment element is enabled
Summary: Undownloaded iCloud Photos are inserted as broken images when attachment elem...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-02 16:49 PST by Wenson Hsieh
Modified: 2022-01-04 10:50 PST (History)
9 users (show)

See Also:

Attachments
For EWS (6.61 KB, patch)
2022-01-02 17:20 PST, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Fix iOS build (6.39 KB, patch)
2022-01-02 18:51 PST, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
For landing (6.85 KB, patch)
2022-01-03 09:11 PST, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2022-01-02 16:49:46 PST 
rdar://82318259
Comment 1 Wenson Hsieh 2022-01-02 17:20:06 PST Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2022-01-02 18:51:25 PST 
Created attachment 448200 [details]
Fix iOS build
Comment 3 Darin Adler 2022-01-02 19:21:48 PST 
Comment on attachment 448200 [details]
Fix iOS build

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

> Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:729
> +    bool fileExists = FileSystem::fileExists(path);

Instead of making a separate file system call, can we save the result from fileTypeFollowingSymlinks? Always nice to reduce the number of separate file system accesses.

> Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:736
> +        attachment->setAttributeWithoutSynchronization(HTMLNames::progressAttr, "0"_s);

I think there’s a way to get the string “0” without allocating a new string. Part of how JavaScript optimizes numeric strings. Not sure it’s easily available, but maybe AtomString::number(0) would work.
Comment 4 Wenson Hsieh 2022-01-02 20:31:04 PST 
Comment on attachment 448200 [details]
Fix iOS build

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

Thanks for the review!

>> Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:729
>> +    bool fileExists = FileSystem::fileExists(path);
> 
> Instead of making a separate file system call, can we save the result from fileTypeFollowingSymlinks? Always nice to reduce the number of separate file system accesses.

Seems to work! Looks like the return value is `nullopt` in the case where the file doesn't exist.

>> Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:736
>> +        attachment->setAttributeWithoutSynchronization(HTMLNames::progressAttr, "0"_s);
> 
> I think there’s a way to get the string “0” without allocating a new string. Part of how JavaScript optimizes numeric strings. Not sure it’s easily available, but maybe AtomString::number(0) would work.

Yes, `AtomString::number(0)` works — it looks like we already use it right below, too.
Comment 5 Wenson Hsieh 2022-01-03 09:11:47 PST 
Created attachment 448246 [details]
For landing
Comment 6 EWS 2022-01-03 10:28:15 PST 
Committed r287547 (245682@main): <https://commits.webkit.org/245682@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448246 [details].