Bug 12959 - REGRESSION: Edit -> Copy not enabled on standalone images
Summary: REGRESSION: Edit -> Copy not enabled on standalone images
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL: http://webkit.org/images/icon-gold.png
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-03-03 12:26 PST by Matt Lilek
Modified: 2007-04-09 05:08 PDT (History)
0 users

See Also:

Attachments
Proposed patch (179.26 KB, patch)
2007-03-29 22:35 PDT, Andrew Wellington 		mitz: review-
Details | Formatted Diff | Diff
Proposed Patch 2 (79.47 KB, patch)
2007-03-31 01:03 PDT, Andrew Wellington 		andrew: review-
Details | Formatted Diff | Diff
Proposed patch 3 (79.42 KB, patch)
2007-03-31 04:31 PDT, Andrew Wellington 		justin.garcia: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Lilek 2007-03-03 12:26:29 PST 
Load a standalone image such as http://webkit.org/images/icon-gold.png.  The Edit -> Copy menu item isn't enabled.  This is a regression from Safari 2.0.4.
Comment 1 Mark Rowe (bdash) 2007-03-07 06:46:19 PST 
<rdar://problem/5045713>
Comment 2 John Sullivan 2007-03-12 21:40:31 PDT 
Actually this was already in radar as<rdar://problem/4044786>. This was caused by the shift to use HTML pages for standalone images.
Comment 3 Andrew Wellington 2007-03-29 22:35:44 PDT 
Created attachment 13884 [details]
Proposed patch

This patch adds a new pasteboard method to copy an image based on the image node and URL and then calls that method when copying in an ImageDocument.
Comment 4 mitz 2007-03-30 04:40:15 PDT 
Comment on attachment 13884 [details]
Proposed patch

Standalone image documents are mutable (via JS) after they're opened. This means that this can deref a 0 pointer or set image to 0:

+        Node *image = doc->body()->firstChild();

It also means that renderer() here may be 0 or not a RenderImage:

+    RenderImage* renderer = static_cast<RenderImage*>(imageNode->renderer());

Please don't add another image:
+        * editing/pasteboard/resources/icon-gold.png: Added.

You can use editing/resources/abe.jpg.

\ No newline at end of file

Please add it.

Stars should be next to C++ types:
+        Document *doc = m_frame->document();
+        Node *image = doc->body()->firstChild();

+    void writeImage(Node *imageNode, const KURL& url);

+void Pasteboard::writeImage(Node *imageNode, const KURL& url)

But next to Obj-C object variables' names:
+    NSArray* types = writableTypesForImage();

r- because in its current form, the patch makes it possible for a website to crash the browser.
Comment 5 Andrew Wellington 2007-03-31 01:03:43 PDT 
Created attachment 13901 [details]
Proposed Patch 2

Update based on comments from Mitz
Comment 6 Andrew Wellington 2007-03-31 04:31:00 PDT 
Comment on attachment 13901 [details]
Proposed Patch 2

Submitting an updated patch
Comment 7 Andrew Wellington 2007-03-31 04:31:08 PDT 
Created attachment 13906 [details]
Proposed patch 3

A few more changes after conversation with Mitz
Comment 8 mitz 2007-04-05 12:18:31 PDT 
Comment on attachment 13906 [details]
Proposed patch 3

Looks good to me, but I think someone else needs to review it.
Comment 9 Justin Garcia 2007-04-05 13:50:56 PDT 
Comment on attachment 13906 [details]
Proposed patch 3

I think some code sharing between Pasteboard::writeImage(const HitTestResult&) and writeImage(Node*, KURL) would have been possible, also you probably could have created a public getter for ImageDocument's m_imageElement instead of getting its body's first child, but r=me.
Comment 10 Andrew Wellington 2007-04-09 05:08:35 PDT 
Committed in r20802