restore the old behavior of cmd_copyImageContents

RESOLVED FIXED in mozilla1.8beta2

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: mano, Assigned: mano)

Tracking

Trunk
mozilla1.8beta2
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Apparently, only seamonkey needs the changes from Bug 135300.

While it's now possible to use it command params (in order to copy data/url/html
only), this isn't a general pactice for XUL apps.

The attached fix:
  * renames the /new/ copy image contnets to cmd_copyImage. By default, this
command copies both the link and the image contents. It's still possible to use
this command w/ command params.
  * restores the old behavior for cmd_copyImageContnets.
  * fixes xpfe to use cmd_copyImage
  * doesn't updete nsIClipboardCommands (frozen), embedors can either use
CopyImageContent/Location which it already provies, or use the copy-image
commands (like Camino).

CCing Camino owners, you should probably backout the fix comamnd params fix.
using command params won't break anything but it's not necessary.
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla1.8beta2
Created attachment 181255 [details] [diff] [review]
patch v1
Attachment #181255 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #181255 - Flags: review?(neil.parkwaycc.co.uk)

Comment 2

13 years ago
Comment on attachment 181255 [details] [diff] [review]
patch v1

>   if (!nsCRT::strcmp(sCopyImageLocationString, aCommandName))
>     return aEdit->CopyImage(nsIContentViewerEdit::COPY_IMAGE_TEXT);
>+  else if (!nsCRT::strcmp(sCopyImageContentsString, aCommandName))
>+    return aEdit->CopyImage(nsIContentViewerEdit::COPY_IMAGE_DATA);
"else" isn't required after "return".

>     <command id="cmd_copyImageLocation"/>
>-    <command id="cmd_copyImageContents"/>
>+    <command id="cmd_copyImage"/>
You only updated three of the five Suite files that use cmd_copyImageContents;
perhaps you could remove all the old uses of cmd_copyImageLocation too?

I think this should get separate review and superreview.
Attachment #181255 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #181255 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #181255 - Flags: review-

Comment 3

13 years ago
Or you could just leave all three in, for use by extensions.
extension authors can add them, if they really need it.
Created attachment 181263 [details] [diff] [review]
v1.1
Attachment #181255 - Attachment is obsolete: true
Attachment #181263 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #181263 - Flags: review?(bugmail)
Created attachment 181272 [details] [diff] [review]
v1.2
Attachment #181263 - Attachment is obsolete: true
Attachment #181272 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #181272 - Flags: review?(bugmail)
Attachment #181263 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #181263 - Flags: review?(bugmail)

Updated

13 years ago
Attachment #181272 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Summary: restore the old behavior for cmd_copyImageContents → restore the old behavior of cmd_copyImageContents
I'm a bit swamped at the moment, and this isn't code I know anyway so I suggest
finding someone else to review.
Comment on attachment 181272 [details] [diff] [review]
v1.2

OK, asking Boris who has reviwed the "orginal" patch.
Attachment #181272 - Flags: review?(bugmail) → review?(bzbarsky)
I'm a little confused here.  Given the quotes from Ben in bug 135300, I don't
see that the solutiong in bug 210043 is something he'd be too happy with.  And
if not, shouldn't this patch fix the corresponding Firefox files too?
(For the record note: I havne't talked to Ben about this change, just to Mike.
Per him, we don't want to change the following UE at this point)

Before bug 135300 checkins, FF's context menu had:
  * a Copy Image Location menu item.
  * a Copy Image menu item which copies the image contents (cmd_copyImageContents)

As far as I remember, the embedding iface nsIClipboardCommands still works this
way - It has CopyImageContents which copies the image alone and
CopyImageLocation which copies the image location.

However, after bug 135300 checkins, cmd_copyImageContents copies both the image
and its location. The attached fix still allows that using a new command name
(in order to avoid the abuse of the copyImageContents command).
Flags: blocking-aviary1.1?
Comment on attachment 181272 [details] [diff] [review]
v1.2

OK.  In that case, looks ok.  I assume you've carefully tested suite browser
and mailnews, right?  If you haven't please do before checking this in.
Attachment #181272 - Flags: review?(bzbarsky) → review+
Flags: blocking1.8b2?

Comment 12

13 years ago
Comment on attachment 181272 [details] [diff] [review]
v1.2

a=asa
Attachment #181272 - Flags: approval1.8b2? → approval1.8b2+
Checking in dom/src/base/nsGlobalWindowCommands.cpp;
/cvsroot/mozilla/dom/src/base/nsGlobalWindowCommands.cpp,v  <-- 
nsGlobalWindowCommands.cpp
new revision: 1.17; previous revision: 1.16
done
Checking in xpfe/browser/resources/content/navigatorOverlay.xul;
/cvsroot/mozilla/xpfe/browser/resources/content/navigatorOverlay.xul,v  <-- 
navigatorOverlay.xul
new revision: 1.308; previous revision: 1.307
done
Checking in xpfe/communicator/resources/content/contentAreaContextOverlay.xul;
/cvsroot/mozilla/xpfe/communicator/resources/content/contentAreaContextOverlay.xul,v
 <--  contentAreaContextOverlay.xul
new revision: 1.56; previous revision: 1.55
done
Checking in xpfe/communicator/resources/content/utilityOverlay.xul;
/cvsroot/mozilla/xpfe/communicator/resources/content/utilityOverlay.xul,v  <-- 
utilityOverlay.xul
new revision: 1.48; previous revision: 1.47
done
Checking in mailnews/base/resources/content/mailWindowOverlay.xul;
/cvsroot/mozilla/mailnews/base/resources/content/mailWindowOverlay.xul,v  <-- 
mailWindowOverlay.xul
new revision: 1.293; previous revision: 1.292
done
Checking in extensions/help/resources/content/help.xul;
/cvsroot/mozilla/extensions/help/resources/content/help.xul,v  <--  help.xul
new revision: 1.59; previous revision: 1.58
done
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Flags: blocking1.8b2?
Flags: blocking-aviary1.1?
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.