Bug 10725 - Image data in from RTFD clipboard data thrown away
Summary: Image data in from RTFD clipboard data thrown away
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Graham Dennis
Depends on:
Reported: 2006-09-04 04:03 PDT by Graham Dennis
Modified: 2007-01-30 06:05 PST (History)
2 users (show)

See Also:

patch (1.47 KB, patch)
2006-09-04 04:09 PDT, Graham Dennis
eric: review-
Details | Formatted Diff | Diff
patch 2 (1.02 KB, patch)
2006-09-06 01:55 PDT, Graham Dennis
eric: review-
Details | Formatted Diff | Diff
patch 3 (375.12 KB, patch)
2006-09-13 08:37 PDT, Graham Dennis
no flags Details | Formatted Diff | Diff
patch 4 (375.19 KB, patch)
2006-10-05 23:47 PDT, Graham Dennis
no flags Details | Formatted Diff | Diff
patch 5 (375.28 KB, patch)
2006-10-08 06:47 PDT, Graham Dennis
no flags Details | Formatted Diff | Diff
patch 6 (376.11 KB, patch)
2006-10-10 06:09 PDT, Graham Dennis
no flags Details | Formatted Diff | Diff
patch 6a (376.14 KB, patch)
2006-10-10 07:43 PDT, Graham Dennis
no flags Details | Formatted Diff | Diff
patch 7 (378.40 KB, patch)
2006-11-21 01:49 PST, Graham Dennis
no flags Details | Formatted Diff | Diff
patch 8 (375.43 KB, patch)
2006-11-21 04:28 PST, Graham Dennis
no flags Details | Formatted Diff | Diff
patch 9 (374.06 KB, patch)
2006-11-29 04:49 PST, Graham Dennis
mjs: review-
Details | Formatted Diff | Diff
patch 10 (code only) (17.01 KB, patch)
2007-01-05 18:08 PST, Graham Dennis
mjs: review+
Details | Formatted Diff | Diff
patch 11 (code only) (15.35 KB, patch)
2007-01-29 04:48 PST, Graham Dennis
no flags Details | Formatted Diff | Diff
patch 12 (code only) (14.10 KB, patch)
2007-01-29 16:32 PST, Graham Dennis
no flags Details | Formatted Diff | Diff
patch 13 (code only) (14.59 KB, patch)
2007-01-29 18:11 PST, Graham Dennis
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Graham Dennis 2006-09-04 04:03:47 PDT
In WebHTMLView's _documentFragmentFromPasteboard:... routine, subresources found in the RTFD part of the clipboard are not added as subresources to the data source so that the data from those resources can be accessed later.

I'm not sure how to construct a testcase for this, as I found this in Sandvox when trying to get the data for an image URL that came from RTFD pasteboard data.

Patch forthcoming.
Comment 1 Graham Dennis 2006-09-04 04:09:49 PDT
Created attachment 10389 [details]

Patch. Uses an enumerator to add each subresource in the subresources array to the data source.
Comment 2 Darin Adler 2006-09-04 17:07:25 PDT
Comment on attachment 10389 [details]

Looks good. Can we create a regression test for this?
Comment 3 Eric Seidel (no email) 2006-09-05 17:09:41 PDT
Comment on attachment 10389 [details]

Tabs.  r-

Otherwise looks fine.  I'm not super familiar with this code though.
Comment 4 Graham Dennis 2006-09-06 01:55:55 PDT
Created attachment 10411 [details]
patch 2

Silly me, I had changed SubEthaEdit to use spaces, but I forgot to change Xcode.

Updated patch.
Comment 5 Darin Adler 2006-09-07 08:43:24 PDT
Comment on attachment 10411 [details]
patch 2

Looks good. Can we create a regression test for this?
Comment 6 Graham Dennis 2006-09-07 15:15:38 PDT
(In reply to comment #5)
> (From update of attachment 10411 [details] [edit])
> Looks good. Can we create a regression test for this?
I'm not sure how. One way I would have thought about doing it would be to actually try and paste RTFD data with an image into a editable div in a regression test, and then it would be checked by a pixel test. However, even with this patch, WebKit still doesn't display images that are pasted in in editable divs. And this is a separate bug.

Any ideas?

Comment 7 Darin Adler 2006-09-08 10:17:46 PDT
Comment on attachment 10411 [details]
patch 2

I haven't reviewed this yet because I want to understand what kind of test we're going to land with it.

Also, the ChangeLog patch looks very short and strange. The entire patch looks wrong like it wasn't made with "svn diff".
Comment 8 Eric Seidel (no email) 2006-09-10 11:45:06 PDT
Comment on attachment 10411 [details]
patch 2

Using the copy/paste functionality inside DumpRenderTree, the dumped -expected.txt should be able to show this change.

The ChangeLog does look busted.  Per darin's comments (and lacking text case or copyright) I'm going to r- this.

Fee free to drop by the channel and perhaps one of us can give you a hand polishing this one off.
Comment 9 Graham Dennis 2006-09-13 08:37:33 PDT
Created attachment 10528 [details]
patch 3

Even when applying the previous patch, images pasted in from RTFD data would not be displayed in WebKit. The reason for this is that WebCore tries to load the applewebdata URLs for the images before the resources have been added to the WebUnarchivingState, and because it is an AppKit-private method that parses the RTFD data and creates the document fragment, we cannot add the resources to the WebUnarchivingState until after WebCore has parsed the document fragment and attempted to load the applewebdata URLs.

My method for fixing this is to delay loading applewebdata URLs while the AppKit method is parsing the RTFD data, and only load these URLs after the WebResources have been added to the WebUnarchivingState.

Fixing this caused an exception in Safari because it uses the _webDataRequestExternalURL of the webdata URL as the key in a dictionary for the activity window. So I changed _webDataRequestExternalURL to not return nil, and to return a more sensible default for displaying in the activity window.

To create a Layout test for this patch I had to load the appropriate data into the NSPasteboard, and the only way I could see how to do this was to use the objective-c plugin for DumpRenderTree and call various methods to load the data and put it into NSPasteboard. However, I needed to call the method -[NSPasteboard declareTypes:owner:] which requires an NSArray argument. Unfortunately, NSArray's are bridged to JS Array's, and when I try and pass a JS Array to the objc method, it receives a WebScriptObject instead of an NSArray, and NSPasteboard doesn't like that. To work around this, I added a convenience method to the DumpRenderTreePasteboard that accepts an NSString instead of an NSArray.

I added two test cases for this patch (one for TIFF pasteboard data and one for RTFD pasteboard data) because I came across bug #10314 by trying to paste TIFF pasteboard data, and the patch to 10314 was committed without a layout test.
Comment 10 Graham Dennis 2006-10-05 23:47:19 PDT
Created attachment 10942 [details]
patch 4

Patch 4. Fixes patch rot.
Comment 11 Graham Dennis 2006-10-08 06:47:53 PDT
Created attachment 10978 [details]
patch 5

I made a mistake in my previous patch where I assumed that the applewebdata url's had non-empty URL paths, in this case we return 'about:blank' for the external web data url as in the earlier code.
Comment 12 Justin Garcia 2006-10-09 21:26:13 PDT
Comment on attachment 10978 [details]
patch 5

Most of this good to me, but I'm not familiar enough with this code to review it.
Comment 13 Graham Dennis 2006-10-10 06:09:36 PDT
Created attachment 11013 [details]
patch 6

Patch 6. Fixes patch rot due to latest changes in the loader code.

Also fixed a problem with -[WebFrame _deliverArchivedResourcesAfterDelay] where it schedules a selector on itself that doesn't exist. (Changed to the correct name)
Comment 14 Graham Dennis 2006-10-10 07:43:32 PDT
Created attachment 11014 [details]
patch 6a

Patch 6a... I forgot to update the pixel results when I fixed patch rot. Fixed.
Comment 15 Graham Dennis 2006-11-21 01:49:03 PST
Created attachment 11591 [details]
patch 7

Updated patch fixing patch rot.
Works in r17866.
Comment 16 Graham Dennis 2006-11-21 04:28:02 PST
Created attachment 11592 [details]
patch 8

Patch 8.
Addresses Maciej's comments (via IRC) that we should avoid adding another defer-load mechanism if possible. Now, instead of deferring the NSURLConnection callbacks, the actual loading is deferred.

A reference to WebDataProtocol is still needed to work out when to bypass the cache policy (as the archive is the only way to get the data for WebDataProtocol URLs).

Also, this code removes the need for wkSetNSURLConnectionDefersCallbacks and WKSetNSURLConnectionDefersCallbacks which I think are both referred to in libWebKitSystemInterface.a, however I haven't removed the definitions of these functions just in case...
Comment 17 Graham Dennis 2006-11-29 04:49:56 PST
Created attachment 11663 [details]
patch 9

This patch removes the code in my previous patch that caused WebKit to refer to WebDataProtocol. I believe this patch now addresses all of Maciej's comments via IRC. There is still one part of the patch that modifies WebDataProtocol.mm, however this isn't needed for the layout tests to pass, but is only needed to prevent Safari crashing.
Comment 18 Maciej Stachowiak 2007-01-05 00:38:14 PST
Mostly this looks ok. However:

1) It needs to be updated to ToT.

2) Is the WebDataProtocol change definitely safe? There might be code that depends on getting nil in some cases. This change affects any NSURLRequest and some code might be using this specifically to detect applewebdata: requests. Some of the things in Safari that depend on data loads include: the view source window, the load failure error page, and the snippet editor.

Please do the update and either describe testing for #2 or figure out a way to do this w/o making that change.
Comment 19 Maciej Stachowiak 2007-01-05 00:42:14 PST
Comment on attachment 11663 [details]
patch 9

r- for above comments, also, sorry this took so long, hopefully we can get it in soon. Might be simpler to do future reivews if the LayoutTest bits are left out, assuming those don't change, since they form the bulk of the patch.
Comment 20 Graham Dennis 2007-01-05 18:08:54 PST
Created attachment 12253 [details]
patch 10 (code only)

Patch 10 (code only, the layout test is the same)

Updated to ToT.
The only reason the WebDataProtocol change is necessary is because Safari will crash trying to set an object for a nil key in a dictionary. This seems to be related to the activity viewer. That said, I did some testing to see if anything else broke as a result of this change. I tried opening the 'view source' window on a number of different pages in Safari, and that worked fine (is this just what you wanted checked, or was it something more specific). I tried using the snippet editor, except that it doesn't appear to work in ToT (and still doesn't work with 10725). I also tried entering 'applewebdata://<stuff>' URL's into Safari to see if a load error page would come up, except that Safari just said 'contacting server' in both ToT and with 10725. I also tried 'random://<stuff>' but that also didn't display a load error page (both applewebdata:// and random:// show load error pages in released Safari.).
Comment 21 Graham Dennis 2007-01-05 18:15:00 PST
I just noticed a commit that fixes the snippet editor. Will update and test.
Comment 22 Graham Dennis 2007-01-05 19:57:40 PST
(In reply to comment #21)
> I just noticed a commit that fixes the snippet editor. Will update and test.

The 'show' button of the snippet editor works in both ToT and with this patch, the 'Convert' button doesn't work in either situation. Also, 'random://' and 'applewebdata://' URLs both display load error pages both with ToT and with this patch.
Comment 23 Maciej Stachowiak 2007-01-08 13:23:46 PST
Comment on attachment 12253 [details]
patch 10 (code only)

Comment 24 Dan Wood 2007-01-08 13:48:00 PST
I hate to say this after waiting so long to get this landed ... but I just found a bug in the patch, I think.

I have sent a note to Graham privately to see if he can reproduce it and fully analyze it.  

So for now, can you hold off on this?  Mark it r- for now until we verify what is going on?

Comment 25 Dan Wood 2007-01-08 21:02:00 PST
Well, Graham is unavailable and we can't figure out what the problem is.  I say let's go ahead and land this (since it's been so hard to land) and then later if I can figure out what the problem is, I'll report a new case.
Comment 26 David Kilzer (:ddkilzer) 2007-01-28 14:07:10 PST
Attachment 12253 [details] (patch 10) has bit-rotted due to loader changes.  I tried to apply it and to clean up the rejects, but too much has changed for me to feel confident about the results.

Comment 27 Graham Dennis 2007-01-29 04:48:28 PST
Created attachment 12737 [details]
patch 11 (code only)

Patch fixing patch rot. Nothing else fundamental was changed between patch 10 and 11.

I've investigated the problem that Dan was having, and it seems to me to be unrelated to this patch as the problem still occurs when the (previous) patch for 10725 is not applied to WebKit revision 18653. I'm still investigating that issue and will file a new report for it.
Comment 28 Graham Dennis 2007-01-29 16:32:12 PST
Created attachment 12765 [details]
patch 12 (code only)

Patch fixing patch rot due to removal of WebDataProtocol. Apart from minor tweaking, this patch returns +[NSURL _web_uniqueWebDataURL] because AppKit uses it when turning RTF and RTFD data into a DOMDocument.
Comment 29 Maciej Stachowiak 2007-01-29 17:07:59 PST
Comment on attachment 12765 [details]
patch 12 (code only)

Comment 30 Graham Dennis 2007-01-29 18:11:58 PST
Created attachment 12768 [details]
patch 13 (code only)

Makes TIFF pasting work. (Changes fake URL scheme from -webkit-fake-url to webkit-fake-url.)
Comment 31 Maciej Stachowiak 2007-01-29 21:05:56 PST
Comment on attachment 12768 [details]
patch 13 (code only)

Comment 32 Graham Dennis 2007-01-29 21:17:30 PST
Committed in r19244.
Comment 33 Mark Rowe (bdash) 2007-01-30 06:05:26 PST
This patch introduced an assertion failure in editing/pasteboard/paste-RTFD.html.  See bug 12474 for more info.