<rdar://problem/36508702>
Created attachment 331311 [details] Patch
Comment on attachment 331311 [details] Patch Attachment 331311 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6072074 New failing tests: fast/attachment/attachment-default-icon.html fast/attachment/attachment-respects-css-size.html accessibility/mac/attributed-string-includes-highlighting.html fast/attachment/attachment-subtitle.html fast/attachment/attachment-uti.html fast/attachment/attachment-select-on-click.html fast/attachment/attachment-without-appearance.html fast/attachment/attachment-icon-from-file-extension.html fast/attachment/attachment-action.html fast/attachment/attachment-type-attribute.html fast/attachment/attachment-folder-icon.html
Created attachment 331313 [details] Archive of layout-test-results from ews114 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 331314 [details] Address assertions in WK1
Comment on attachment 331314 [details] Address assertions in WK1 Attachment 331314 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6072623 New failing tests: fast/text/combining-character-sequence-fallback-crash.html
Created attachment 331316 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 331316 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 I'm pretty sure this test failure is not relevant to this patch.
Comment on attachment 331314 [details] Address assertions in WK1 View in context: https://bugs.webkit.org/attachment.cgi?id=331314&action=review > Source/WebCore/editing/Editor.cpp:3783 > + // The document may have changed since the attachment element was inserted, in which case we will suppress notifying the client of inserted attachments. Seems strange. And why don't we do the same with the removals? > Source/WebCore/page/EditorClient.h:76 > + virtual void didInsertAttachment(const String&, const String&) { } Probably need to name these now.
Comment on attachment 331314 [details] Address assertions in WK1 View in context: https://bugs.webkit.org/attachment.cgi?id=331314&action=review >> Source/WebCore/editing/Editor.cpp:3783 >> + // The document may have changed since the attachment element was inserted, in which case we will suppress notifying the client of inserted attachments. > > Seems strange. And why don't we do the same with the removals? You're right, this logic isn't quite right. To put the concern in more concrete terms, the inconsistency in handling attachment removals vs. insertions here is that if script removes attachment elements and then changes the document immediately, we'll notify the client that attachments were removed; however, if attachment elements are inserted and the document is immediately changed, we won't notify the client that attachments were inserted. I think the right way to fix this is to notify the client about insertion and removal for attachment elements in the Frame's current document whenever the Frame's current document changes. Doing this would ensure that our attachment insertion and removal delegate calls are properly balanced — i.e. if we ever tell the client that an attachment element is inserted, we'll eventually balance that with a removal notification. The imbalance in delegate callbacks when script immediately changes a Frame's document after attachments are inserted or removed is really an existing bug that the WK1 layout test assertions earlier revealed, but I can fix that in this patch as well. >> Source/WebCore/page/EditorClient.h:76 >> + virtual void didInsertAttachment(const String&, const String&) { } > > Probably need to name these now. I agree, this looks confusing since they're both strings. I'll add parameter names within /* */, like we do in some other places. Since this has an empty implementation, I'd have to suppress any unused parameter warnings that might result.
Created attachment 331397 [details] Patch for landing
Comment on attachment 331397 [details] Patch for landing Clearing flags on attachment: 331397 Committed r226977: <https://trac.webkit.org/changeset/226977>