Bug 234803

Summary:

Undownloaded iCloud Photos are inserted as broken images when attachment element is enabled

Product:

WebKit

Reporter:

Wenson Hsieh <wenson_hsieh>

Component:

HTML Editing

Assignee:

Wenson Hsieh <wenson_hsieh>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

akeerthi, darin, ews-watchlist, hi, megan_gardner, mifenton, thorton, webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

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

Attachments:

Description	Flags
For EWS	none
Fix iOS build	none
For landing	none

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	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 attachment proposed patch, testcase, etc.

Wenson Hsieh

Comment 1 2022-01-02 17:20:06 PST Comment hidden (obsolete)

Created attachment 448192 [details] For EWS

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.