WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
188933
[Attachment Support] Dropping and pasting images should insert inline image elements with _WKAttachments
https://bugs.webkit.org/show_bug.cgi?id=188933
Summary
[Attachment Support] Dropping and pasting images should insert inline image e...
Wenson Hsieh
Reported
2018-08-24 15:21:54 PDT
Dropping and pasting images should insert inline image elements with _WKAttachments.
Attachments
First pass
(94.02 KB, patch)
2018-08-24 16:42 PDT
,
Wenson Hsieh
ews-watchlist
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2
(3.30 MB, application/zip)
2018-08-24 17:43 PDT
,
EWS Watchlist
no flags
Details
Patch
(95.43 KB, patch)
2018-08-25 16:15 PDT
,
Wenson Hsieh
darin
: review+
Details
Formatted Diff
Diff
Patch for landing
(95.87 KB, patch)
2018-08-26 18:59 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2018-08-24 15:22:15 PDT
<
rdar://problem/43699724
>
Wenson Hsieh
Comment 2
2018-08-24 16:42:30 PDT
Created
attachment 348055
[details]
First pass
EWS Watchlist
Comment 3
2018-08-24 17:43:49 PDT
Comment hidden (obsolete)
Comment on
attachment 348055
[details]
First pass
Attachment 348055
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
https://webkit-queues.webkit.org/results/8977134
New failing tests: accessibility/smart-invert-reference.html
EWS Watchlist
Comment 4
2018-08-24 17:43:51 PDT
Comment hidden (obsolete)
Created
attachment 348064
[details]
Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Wenson Hsieh
Comment 5
2018-08-25 16:15:10 PDT
Created
attachment 348086
[details]
Patch
Darin Adler
Comment 6
2018-08-26 15:08:02 PDT
Comment on
attachment 348086
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=348086&action=review
> Source/WebCore/editing/markup.cpp:418 > + // These attributes are only intended for File deserialization, and are removed from the generated attachment > + // element after we've deserialized and set its backing File.
The comment here says "are removed", but doesn’t make it clear where the code that removes them is. Related, a comment like this that makes promises for code elsewhere ("are removed") is dangerous unless the code that does the removing also has a comment pointing back here.
> Source/WebCore/editing/markup.cpp:937 > + // We use a fake body element here to trick the HTML parser to using the InBody insertion mode.
"trick into" is the normal idiom, rather than "trick to" that we are using here
> Source/WebCore/page/DragController.cpp:1383 > + attachment = downcast<HTMLAttachmentElement>(&element);
Compiler probably gets this right without help, but it’s theoretically more efficient to do the "&" after casting, rather than converting to a pointer first. attachment = &downcast<HTMLAttachmentElement>(element);
> Source/WebCore/page/DragController.cpp:1385 > + if (is<HTMLImageElement>(element))
Maybe use else if here for clarity?
> Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:48 > + return UTTypeIsDeclared((CFStringRef)type) || UTTypeIsDynamic((CFStringRef)type);
Please use __bridge in these typecasts.
> Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:58 > + auto mimeType = adoptCF(UTTypeCopyPreferredTagWithClass((CFStringRef)contentType, kUTTagClassMIMEType)); > + return (NSString *)mimeType.autorelease();
There is no reason to use autorelease here. We can turn a CFString into a WTF::String without involving NSString. Should be: return adoptCF(UTTypeCopyPreferredTagWithClass((__bridge CFStringRef)contentType, kUTTagClassMIMEType)).get();
> Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:68 > + auto utiType = adoptCF(UTTypeCreatePreferredIdentifierForTag(kUTTagClassMIMEType, (CFStringRef)contentType, nil)); > + return (NSString *)utiType.autorelease();
There is no reason to use autorelease here. We can turn a CFString into a WTF::String without involving NSString. Also should use a __bridge cast and should not use "nil" for a non-Objective-C-object pointer. Should be: return adoptCF(UTTypeCreatePreferredIdentifierForTag(kUTTagClassMIMEType, (__bridge CFStringRef)contentType, nullptr));
Wenson Hsieh
Comment 7
2018-08-26 15:49:42 PDT
Comment on
attachment 348086
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=348086&action=review
>> Source/WebCore/editing/markup.cpp:418 >> + // element after we've deserialized and set its backing File. > > The comment here says "are removed", but doesn’t make it clear where the code that removes them is. Related, a comment like this that makes promises for code elsewhere ("are removed") is dangerous unless the code that does the removing also has a comment pointing back here.
Got it — added to this comment describing where we expect to remove these attributes, and also added a comment where we remove these attributes.
>> Source/WebCore/editing/markup.cpp:937 >> + // We use a fake body element here to trick the HTML parser to using the InBody insertion mode. > > "trick into" is the normal idiom, rather than "trick to" that we are using here
Fixed!
>> Source/WebCore/page/DragController.cpp:1383 >> + attachment = downcast<HTMLAttachmentElement>(&element); > > Compiler probably gets this right without help, but it’s theoretically more efficient to do the "&" after casting, rather than converting to a pointer first. > > attachment = &downcast<HTMLAttachmentElement>(element);
Fixed!
>> Source/WebCore/page/DragController.cpp:1385 >> + if (is<HTMLImageElement>(element)) > > Maybe use else if here for clarity?
Ok!
>> Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:48 >> + return UTTypeIsDeclared((CFStringRef)type) || UTTypeIsDynamic((CFStringRef)type); > > Please use __bridge in these typecasts.
Fixed!
>> Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:58 >> + return (NSString *)mimeType.autorelease(); > > There is no reason to use autorelease here. We can turn a CFString into a WTF::String without involving NSString. Should be: > > return adoptCF(UTTypeCopyPreferredTagWithClass((__bridge CFStringRef)contentType, kUTTagClassMIMEType)).get();
Good point — fixed!
>> Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:68 >> + return (NSString *)utiType.autorelease(); > > There is no reason to use autorelease here. We can turn a CFString into a WTF::String without involving NSString. Also should use a __bridge cast and should not use "nil" for a non-Objective-C-object pointer. Should be: > > return adoptCF(UTTypeCreatePreferredIdentifierForTag(kUTTagClassMIMEType, (__bridge CFStringRef)contentType, nullptr));
Fixed! (with a .get() at the end)
Wenson Hsieh
Comment 8
2018-08-26 18:59:48 PDT
Created
attachment 348109
[details]
Patch for landing
WebKit Commit Bot
Comment 9
2018-08-26 19:37:30 PDT
Comment on
attachment 348109
[details]
Patch for landing Clearing flags on attachment: 348109 Committed
r235343
: <
https://trac.webkit.org/changeset/235343
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug