Bug 181638 – [Attachment Support] Provide the `src` of an attachment to the UI delegate when an attachment is inserted

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Wenson Hsieh]

<rdar://problem/36508702>

[Wenson Hsieh]

Created attachment 331311 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Wenson Hsieh]

Created attachment 331314 [details]
Address assertions in WK1

[EWS Watchlist]

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

[EWS Watchlist]

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

[Wenson Hsieh]

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.

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 331397 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 331397 [details]
Patch for landing

Clearing flags on attachment: 331397

Committed r226977: <https://trac.webkit.org/changeset/226977>