78010 - Implement nsIClipboardHelper

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Dan Rosen]

Clipboard helper routines to copy simple strings, etc. to the clipboard are
strewn around the codebase. A clipboard helper (service?), nsIClipboardHelper,
would clean this up.

Comment 1

[Dan Rosen]

Bug 32358 would like this, but it's going to work around it. nsPresShell.cpp
would like this in a few places as well...

Comment 2

[Dan Rosen]

I have a working patch in my tree now. The interface right now is quite simple.
It consists of two methods: |copyString(AString)| and
|copyStringToClipboard(AString, long)|.

Looking around it looks like we've got a few places in the code right now where
we'd like to be using the new interface as it stands:

  - nsPresShell.cpp: DoCopyLinkLocation, DoCopyImageLocation
  - extensions/inspector/.../ClipboardUtils.js
  - msgHeaderViewOverlay.js: CopyEmailAddress

Also looks like we should fold nsCopySupport and nsTreeController into this, as
well as a couple other functions (just grepping for "nsIClipboard" in lxr)...

Patch on the way.

Comment 3

[Dan Rosen]

Just remembered, I probably also want to consolidate nsClipboard.js into here
sooner or later as well... :)

Comment 4

[Gervase Markham [:gerv]]

Any chance of the copyHTML function you mentioned becoming a helper function? 
And how about reading? nsClipboard.js has functions for that, and it would seem 
sensible for a clipboard helper to allow you to both read and write :-)

(I take it the helper will take care of the Unix Selection Clipboard details.)

Gerv

Comment 5

[Dan Rosen]

Attachment: Patch to docshell, layout, mailnews, webshell and widget: creates nsIClipboardHelper service, uses it in a few places and expands nsIClipboardCommands

Comment 6

[Dan Rosen]

Alright, I'm hoping that's everything. Note that you need the patch attached in
78824, to nsISupportsPrimitives, for this patch to compile. It seems to work
just fine for me, but I haven't tested on windows or mac yet.

The ugliest part to this seems to have been patching each platform's widget
factory -- it might be worth it in the long run to refactor these somehow...

(upping severity to major, since this patch touches 25 files :)

Comment 7

[Dan Rosen]

Gerv: Yeah. I'd like to get the interface landed first, before I build on it,
though. That way, at least you'll be able to use the basic "copy string to
clipboard" functionality right off the bat, without wasting any more time.

And yes, the helper hides the details of the unix selection clipboard. I patched
the mailnews callsite -- you can see how simple it is to use.

Comment 8

[Stuart Parmenter]

looks ok to me.  get pink to look at it too.  r=pavlov

Comment 9

[Mike Pinkerton (not reading bugmail)]

several comments:

- remove the checks you added for NS_ENSURE_ARG_POINTER in nsWebShell and 
nsClipboard
- the patch to msgHdrViewOverlay.js should call copyStringToClipboard() and pass 
the gloabl clipboard instead of just calling copyString(), which would wipe the 
selection clipboard (the old code didn't put it on the selection clip).
- placing the data on the selection clip w/in the impl of copyString should 
probably be ifdef'd unix, though i guess it doesn't have to be. no big deal 
there.
- why only replace the code in msgHdrViewOverlay.js? why not elsewhere (such as 
contextMenu.js, which that code you replace alludes to have come from)?

Comment 10

[Dan Rosen]

> - remove the checks you added for NS_ENSURE_ARG_POINTER in nsWebShell

Done.

> and nsClipboard

Not done. These should properly return failure even in opt builds where the
assertion would not be seen.

> - the patch to msgHdrViewOverlay.js should call copyStringToClipboard() and
> pass the gloabl clipboard instead of just calling copyString(), which would
> wipe the selection clipboard (the old code didn't put it on the selection
> clip).

I'll have to ask. I believe since the old js code here was ripped off from other
old js code, there was an error in the ripping-off which nobody caught. I
believe they want to be copying to the selection clipboard as well...

> - placing the data on the selection clip w/in the impl of copyString should
> probably be ifdef'd unix, though i guess it doesn't have to be. no big deal
> there.

Hm. I must need to comment CopyString a bit more thoroughly. The Right Thing to
do to determine whether we support the selection clipboard is not to do #ifdef
XP_UNIX (which is what we were doing before), but to ask
nsIClipboard::SupportsSelectionClipboard. I do this in CopyStringToClipboard,
but don't mention the logic in CopyString.

> - why only replace the code in msgHdrViewOverlay.js? why not elsewhere (such
> as contextMenu.js, which that code you replace alludes to have come from)?

There aren't too many other places that need to simply copy strings.
contextMenu.js doesn't even do this anymore -- PresShell does (in
DoCopyImageLocation and DoCopyLinkLocation, which is what was moved from
contextMenu.js), and I've patched that call. After this interface lands, at some
point, I'll fold functionality from nsClipboard.js (and maybe also from
PresShell::DoCopy) into the new interface and patch their respective callsites.

Comment 11

[Dan Rosen]

I've filed bug 79787 to enhance nsIClipboardHelper with useful methods.

Comment 12

[Mike Pinkerton (not reading bugmail)]

ok, r=pink

Comment 13

[Dan Rosen]

Attachment: Cleaned up patch, per pink. Also patches extensions/inspector.

Comment 14

[Joe Hewitt (gone)]

inspector changes are kosher. r=hewitt

Comment 15

[David Hyatt]

[s]r=hyatt

Comment 16

[Dan Rosen]

fixed. see widget/public/nsIClipboardHelper for the new api, and bug 79787 for
further work.

Comment 17

[Dan Rosen]

oops, really fixed.

Comment 18

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old