Use blob URL when pasting RTFD instead of overriding DocumentLoader

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Right now, the pasted content’s subresources are loaded by overriding document loader.
This is extremely hacky way of making paste work, and websites can’t access those sub resources.
Use blob URL instead.

[Ryosuke Niwa]

<rdar://problem/34542270>

[Ryosuke Niwa]

Created attachment 322500 [details]
Fixes the bug

[Ryosuke Niwa]

Created attachment 322554 [details]
Fixed builds

[Ryosuke Niwa]

Created attachment 322555 [details]
Fixed GTK+ builds

[Wenson Hsieh]

Comment on attachment 322555 [details]
Fixed GTK+ builds

View in context: https://bugs.webkit.org/attachment.cgi?id=322555&action=review

The general approach looks reasonable to me, but I'm not familiar with loading code at all. Perhaps Chris would have some additional input here?

> Source/WebCore/ChangeLog:10
> +        into the document using WebKit fake URL, and DocumentLoader was overridden to return the appropraite

appropraite => appropriate

> Source/WebCore/editing/markup.cpp:128
> +            if (element.attributeContainsURL(attribute) && !attribute.value().isEmpty()) {

How would this handle data: URLs, or app: URLs?

> Source/WebCore/editing/markup.cpp:131
> +                    changes.append(AttributeChange(&element, attribute.name(), replacement));

Nit - { } initialization here?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteRTFD.mm:46
> +    [[NSPasteboard generalPasteboard] declareTypes:[NSArray arrayWithObject:NSRTFDPboardType] owner:nil];

Nit - this would probably be more succinct using @[] instead of +[NSArray arrayWithObject]

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteRTFD.mm:61
> +    [[UIPasteboard generalPasteboard] setItems:@[[NSDictionary dictionaryWithObject:data forKey:(NSString *)kUTTypeFlatRTFD]]];

Nit - this would probably be more succinct using @{} instead of +[NSDictionary dictionaryWithObject]

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteRTFD.mm:72
> +    [webView paste:nil];

Did you mean to check that the editable root is empty after this?

[Ryosuke Niwa]

Comment on attachment 322555 [details]
Fixed GTK+ builds

View in context: https://bugs.webkit.org/attachment.cgi?id=322555&action=review

>> Source/WebCore/editing/markup.cpp:128
>> +            if (element.attributeContainsURL(attribute) && !attribute.value().isEmpty()) {
> 
> How would this handle data: URLs, or app: URLs?

They won't be in replacement map since those URLs shouldn't be appearing as sub resources in attributed attributes we just parsed.

>> Source/WebCore/editing/markup.cpp:131
>> +                    changes.append(AttributeChange(&element, attribute.name(), replacement));
> 
> Nit - { } initialization here?

Sure.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteRTFD.mm:46
>> +    [[NSPasteboard generalPasteboard] declareTypes:[NSArray arrayWithObject:NSRTFDPboardType] owner:nil];
> 
> Nit - this would probably be more succinct using @[] instead of +[NSArray arrayWithObject]

Fixed.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteRTFD.mm:61
>> +    [[UIPasteboard generalPasteboard] setItems:@[[NSDictionary dictionaryWithObject:data forKey:(NSString *)kUTTypeFlatRTFD]]];
> 
> Nit - this would probably be more succinct using @{} instead of +[NSDictionary dictionaryWithObject]

Fixed.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteRTFD.mm:72
>> +    [webView paste:nil];
> 
> Did you mean to check that the editable root is empty after this?

No. This test is simply here to make sure WebKit doesn't hit a debug assertion / crash when sharedBuffer is empty in getPasteboardBufferForType.

[Ryosuke Niwa]

Created attachment 322638 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 322638 [details]
Patch for landing

Wait for EWS.

[WebKit Commit Bot]

Comment on attachment 322638 [details]
Patch for landing

Clearing flags on attachment: 322638

Committed r222839: <http://trac.webkit.org/changeset/222839>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.