291110 - restore the old behavior of cmd_copyImageContents

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Asaf Romano (gone)]

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.

Comment 1

[Asaf Romano (gone)]

Attachment: patch v1

Comment 2

[[e-mail]]

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.

Comment 3

[[e-mail]]

Or you could just leave all three in, for use by extensions.

Comment 4

[Asaf Romano (gone)]

extension authors can add them, if they really need it.

Comment 5

[Asaf Romano (gone)]

Attachment: v1.1

Comment 6

[Asaf Romano (gone)]

Attachment: v1.2

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 8

[Asaf Romano (gone)]

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

OK, asking Boris who has reviwed the "orginal" patch.

Comment 9

[Boris Zbarsky [: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?

Comment 10

[Asaf Romano (gone)]

(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).

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Asa Dotzler [:asa]]

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

a=asa

Comment 13

[Asaf Romano (gone)]

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