525389 - Support receiving data from Mac OS X Services into text and HTML editors

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, enhancement)

Description

[Tom Dyas]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.4) Gecko/20091016 Firefox/3.5.4
Build Identifier: 

The core supports sending data to Mac OS X System Services.  It should also support receiving data from a service and pasting that data into a text/html editor.

Reproducible: Always

Comment 1

[Tom Dyas]

Attachment: patch v1

This patch implements receiving data from Mac OS X Services.  It still needs test cases for the changes made to the nsIEditor interface.  (Testing the Services support itself is not feasible without an external program.)

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Josh: want to find a reviewer for this?

Comment 3

[Tom Dyas]

I still need to write some test cases before this patch is ready for review.  I assume it will probably need review by Josh or another Mac widget peer, an editor peer, and smaug (for the change to the event state manager) ?  I emailed the editor peers but never got a response as to who would be the proper reviewer.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Peterv or I could do the editor review. I can also review the event state manager part.

Comment 5

[Tom Dyas]

Attachment: patch v2

Replicated one of the existing HTML editor tests to use the pasteTransferable contact command. Any other tests that should be added?  Comments in general?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 415465 [details] [diff] [review]
patch v2

>@@ -4576,7 +4581,27 @@
>     NS_ENSURE_SUCCESS(rv, rv);
>     aEvent->mIsEnabled = canDoIt;
>     if (canDoIt && !aEvent->mOnlyEnabledCheck) {
>-      rv = controller->DoCommand(cmd);
>+      switch (aEvent->message) {
>+        default:
>+          rv = controller->DoCommand(cmd);
>+          break;
>+        case NS_CONTENT_COMMAND_PASTE_TRANSFERABLE: {
This looks a bit strange, default: before the case:


>+          nsCOMPtr<nsICommandController> commandController = do_QueryInterface(controller, &rv);
>+          if (NS_FAILED(rv) || !commandController)
>+            break;
I'd do
nsCOMPtr<nsICommandController> commandController = do_QueryInterface(controller);
NS_ENSURE_STATE(commandController);


>+
>+          nsCOMPtr<nsICommandParams> params = do_CreateInstance("@mozilla.org/embedcomp/command-params;1", &rv);
>+          if (NS_FAILED(rv))
>+            break;
NS_ENSURE_SUCCESS(rv, rv);


>+
>+          rv = params->SetISupportsValue("transferable", aEvent->mTransferable);
>+          if (NS_FAILED(rv))
>+            break;
NS_ENSURE_SUCCESS(rv, rv);

>+NS_IMETHODIMP
>+nsDOMWindowUtils::SendContentCommandEvent(const nsAString& aType,
>+                                          nsITransferable * aTransferable)
>+{
>+  PRBool hasCap = PR_FALSE;
>+  if (NS_FAILED(nsContentUtils::GetSecurityManager()->IsCapabilityEnabled("UniversalXPConnect", &hasCap))
>+      || !hasCap)
'||' should be at the end of the previous line, but since this style is used
elsewhere in the file too, no need to change.
                                  in boolean aTrusted);
>+
>+  /**
>+   * Generate a content command event.
>+   *
>+   * Cannot be accessed from unprivileged context (not content-accessible)
>+   * Will throw a DOM security error if called without UniversalXPConnect
>+   * privileges.
>+   *
>+   * @param aType Type of command content event to send.  Can be one of "cut",
>+   *   "copy", "paste", "delete", "undo", "redo", or "pasteTransferable".
>+   * @param aTransferable an instance of nsITransferable when aType is
>+   *   "pasteTransferable"
>+   */
Could you align the lines under @param so a bit differently.
Maybe
@param aType some text
       some more text 

>+  /** Paste the text in |aTransferable| at the cursor position, replacing the
>+    * selected text (if any).
>+    */
>+  void pasteTransferable(in nsITransferable aTransferable);
>+
>   /** Can we paste? True if the doc is modifiable, and we have
>     * pasteable data in the clipboard.
>     */
>   boolean canPaste(in long aSelectionType);
> 
>+  /** Can we paste |aTransferable| or, if |aTransferable| is null, is it
>+    * moot to call pasteTransferable later with a instance of
"moot"? Could some other word be better (sorry my bad English).


>+  // Peek in |aTransferable| to see if it contains a supported MIME type.
>+
>+  // the flavors that we can deal with (preferred order selectable for k*ImageMime)
>+  const char* textEditorFlavors[] = { kUnicodeMime, nsnull };
>+  const char* textHtmlEditorFlavors[] = { kUnicodeMime, kHTMLMime,
>+                                          kJPEGImageMime, kPNGImageMime,
>+                                          kGIFImageMime, nsnull };
I wonder if we could have these "flavors" in some more global place. 

Looks good, but I could re-read this.
Josh, could you review the OSX specific parts?

Comment 7

[Tom Dyas]

(In reply to comment #6)
> (From update of attachment 415465 [details] [diff] [review])
> >@@ -4576,7 +4581,27 @@
> >     NS_ENSURE_SUCCESS(rv, rv);
> >     aEvent->mIsEnabled = canDoIt;
> >     if (canDoIt && !aEvent->mOnlyEnabledCheck) {
> >-      rv = controller->DoCommand(cmd);
> >+      switch (aEvent->message) {
> >+        default:
> >+          rv = controller->DoCommand(cmd);
> >+          break;
> >+        case NS_CONTENT_COMMAND_PASTE_TRANSFERABLE: {
> This looks a bit strange, default: before the case:

I wanted that at the top of the switch since it is the most common case there. I can move it to the bottom of the switch if you'd like.

> >+  /** Can we paste |aTransferable| or, if |aTransferable| is null, is it
> >+    * moot to call pasteTransferable later with a instance of
> "moot"? Could some other word be better (sorry my bad English).

Not your English.  Just my legal training.  I end up using the word moot probably more than most native English speakers ...


> >+  // Peek in |aTransferable| to see if it contains a supported MIME type.
> >+
> >+  // the flavors that we can deal with (preferred order selectable for k*ImageMime)
> >+  const char* textEditorFlavors[] = { kUnicodeMime, nsnull };
> >+  const char* textHtmlEditorFlavors[] = { kUnicodeMime, kHTMLMime,
> >+                                          kJPEGImageMime, kPNGImageMime,
> >+                                          kGIFImageMime, nsnull };
> I wonder if we could have these "flavors" in some more global place. 

Since they are used in the nsHTMLDataTransfer.cpp, I can make them into a file-level static array.

Comment 8

[Josh Aas]

Comment on attachment 415465 [details] [diff] [review]
patch v2

>+#define IsStringType(typeStr) ([typeStr isEqual:NSStringPboardType] || [typeStr isEqual:NSHTMLPboardType])

This sounds like it would only check NSStringPboardType, maybe give it a more accurate name based on its actual usage?

>+      if (sendType != nil) {

I'd prefer that you just write "(sendType)" instead of writing out the nil check the long way.

> // not currently support replacing the current selection so just return NO.

Remove this comment, no longer true.

>+  return (command.mSucceeded && command.mIsEnabled) ? YES : NO;

No need for the "?:" stuff, just do "return (... && ...);".

>+  static nsresult TransferableFromPasteboard(nsITransferable *aTransferable, NSPasteboard 

This big method addition seems to duplicate a lot of code from "nsClipboard::GetNativeClipboardData". There must be a better way to re-use code here.

Comment 9

[Josh Aas]

> This looks a bit strange, default: before the case:

Also, I'm against having default before cases. IIRC the way most compilers deal with switch statements the ordering of options won't affect performance.

Comment 10

[Tom Dyas]

Attachment: patch v3

Modified as per reviewers' comments.

Josh: I made use of TransferableFromPasteboard() in the GetNativeClipboardData() function.  It still duplicates calling FlavorsTransferableCanImport though since that info is needed for the cached transferable check. Do you want that refactored as well?

Comment 11

[Josh Aas]

Comment on attachment 419389 [details] [diff] [review]
patch v3

Look good, thanks Tom.

One nit: as a result of this patch, though it is hard to see in the patch itself, the new "nsClipboard::TransferableFromPasteboard" method contains the comment:

  // at this point we can't satisfy the request from cache data so let's look
  // for things other people put on the system clipboard

which describes caller logic, not logic within TransferableFromPasteboard. This comment is made twice, remove it from TransferableFromPasteboard and leave it in the caller.

Comment 12

[Tom Dyas]

Attachment: patch v3.1

Removed extra comment as per Josh's comment. Removed spurious #include in nsChildView.mm.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 419428 [details] [diff] [review]
patch v3.1


>+  /** Can we paste |aTransferable| or, if |aTransferable| is null, will a call
>+    * to pasteTransferable later possibily succeed if given an instance of
"possibily"

Could you still add some checks to the testcase that "paste" event is dispatched?

Comment 14

[Tom Dyas]

Attachment: patch v3.2

Fix spelling error. Test that "paste" event is dispatched correctly and that the paste event handler can use preventDefault to stop a paste from occurring.

I assume the patch needs superreview now?

Comment 15

[Josh Aas]

Olli is a superreviewer iirc, so perhaps he can just mark sr here and we can land it.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Tom, could you still list here ways to test this manually, so that QA can easily
verify that this works on OSX.

Comment 17

[Tom Dyas]

(In reply to comment #16)
> Tom, could you still list here ways to test this manually, so that QA can
> easily verify that this works on OSX.

Basically, the QA person will need to have a Mac OS X System Service installed that can generate data and not just accept data from Firefox.  I developed a small utility to do just that on a consistent basis.  (It's actually an updated version of the "Pasteboard Inspector" that I attached to the original services bug.)  I'll post a copy of the source code in the next week or so once I've had a chance to clean up the code a bit.

Comment 18

[Tom Dyas]

Otherwise, the procedure is:

1.  Load a document that had an editable text field.
2.  Enter text into the editable text field and highlight some or all of the text.
3.  Activate any System Service that returns text data.
4.  The highlighted text should be replaced by the returned text data.

Comment 19

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/bee6729280d8

Comment 20

[Josh Aas]

this caused seamonkey bustage

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1262210436.1262210773.21520.gz&fulltext=1#err0

Comment 21

[Josh Aas]

backed out, we should fix this problem and do a try server run with the fixed patch

Comment 22

[Tom Dyas]

(In reply to comment #20)
> this caused seamonkey bustage
> 
> http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1262210436.1262210773.21520.gz&fulltext=1#err0

I provided dummy implementations in editor/libeditor/base/nsEditor.cpp.  However, nsEudoraEditor does not inherit from nsEditor, only nsIEditor.  (See http://hg.mozilla.org/comm-central/file/5c49bdbb4922/mailnews/import/eudora/src/nsEudoraEditor.cpp)

   394 // void paste (in long aSelectionType)
   395 NS_IMETHODIMP nsEudoraEditor::Paste(PRInt32 aSelectionType)
   396 {
   397   return NS_ERROR_NOT_IMPLEMENTED;
   398 }
   399 
   400 
   401 // boolean canPaste (in long aSelectionType)
   402 NS_IMETHODIMP nsEudoraEditor::CanPaste(PRInt32 aSelectionType, PRBool *_retval)
   403 {
   404   return NS_ERROR_NOT_IMPLEMENTED;
   405 }

The patch should be as simple as adding equivalent dummy implementations of CanPasteTransferable and PasteTransferable.

Comment 23

[Mark Banner (:standard8)]

(In reply to comment #22)
> The patch should be as simple as adding equivalent dummy implementations of
> CanPasteTransferable and PasteTransferable.

As long as you #ifndef MOZILLA_1_9_2_BRANCH them (Thunderbird now builds the comm-central repository alongside mozilla-1.9.2 and mozilla-central).

Comment 24

[Tom Dyas]

Attachment: comm-central fix v1

Add stub implementations of CanPasteTransferable() and PasteTransferable to EudoraEditor class in comm-central.

Can someone push both patches to a try-server for me?  Having trouble building comm-central on my machine.

Comment 25

[Josh Aas]

Pushed to mozilla-central and comm-central:

http://hg.mozilla.org/mozilla-central/rev/f1bf879c9c2d
http://hg.mozilla.org/comm-central/rev/c1d217cb6de9

Comment 26

[Tom Dyas]

Attachment: Pasteboard Inspector

This zip contains an Xcode project for "Pasteboard Inspector."  When installed in a known system location (under /Applications or in ~/Library/Services), it will provide system services to inspect the services pasteboard and to generate a test string. The test string is currently hard-coded and is stored in AppController.m.  Patches welcome.

Comment 27

[Tom Dyas]

Also, Pasteboard Inspector's services will not be visible unless you either log out and then in again or choose "Update Dynamic Services" from the application's "Inspect" menu.