RESOLVED FIXED 234803
Undownloaded iCloud Photos are inserted as broken images when attachment element is enabled 
https://bugs.webkit.org/show_bug.cgi?id=234803
Summary Undownloaded iCloud Photos are inserted as broken images when attachment elem...
Wenson Hsieh
Reported 2022-01-02 16:49:46 PST
rdar://82318259
Attachments
For EWS (6.61 KB, patch)
2022-01-02 17:20 PST, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Fix iOS build (6.39 KB, patch)
2022-01-02 18:51 PST, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
For landing (6.85 KB, patch)
2022-01-03 09:11 PST, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Wenson Hsieh
Comment 1 2022-01-02 17:20:06 PST Comment hidden (obsolete)
Wenson Hsieh
Comment 2 2022-01-02 18:51:25 PST
Created attachment 448200 [details] Fix iOS build
Darin Adler
Comment 3 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.
Wenson Hsieh
Comment 4 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.
Wenson Hsieh
Comment 5 2022-01-03 09:11:47 PST
Created attachment 448246 [details] For landing
EWS
Comment 6 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].
Note You need to log in before you can comment on or make changes to this bug.