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

RESOLVED FIXED in mozilla1.9.3a1

Status

()

--
enhancement
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: tom.dyas, Assigned: tom.dyas)

Tracking

Trunk
mozilla1.9.3a1
All
macOS
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

10 years ago
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
(Assignee)

Comment 1

10 years ago
Posted patch patch v1 (obsolete) — Splinter Review
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.)
(Assignee)

Updated

10 years ago
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → tom.dyas
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Josh: want to find a reviewer for this?
(Assignee)

Comment 3

10 years ago
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.
Peterv or I could do the editor review. I can also review the event state manager part.
(Assignee)

Comment 5

9 years ago
Posted patch patch v2 (obsolete) — Splinter Review
Replicated one of the existing HTML editor tests to use the pasteTransferable contact command. Any other tests that should be added?  Comments in general?
Attachment #409256 - Attachment is obsolete: true
Attachment #415465 - Flags: review?(joshmoz)
Attachment #415465 - Flags: review?(Olli.Pettay)
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?
Attachment #415465 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 7

9 years ago
(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

9 years ago
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.
Attachment #415465 - Flags: review?(joshmoz) → review-

Comment 9

9 years ago
> 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.
(Assignee)

Comment 10

9 years ago
Posted patch patch v3 (obsolete) — Splinter Review
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?
Attachment #415465 - Attachment is obsolete: true
Attachment #419389 - Flags: review?(joshmoz)
Attachment #419389 - Flags: review?(Olli.Pettay)

Comment 11

9 years ago
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.
Attachment #419389 - Flags: review?(joshmoz) → review+
(Assignee)

Comment 12

9 years ago
Posted patch patch v3.1 (obsolete) — Splinter Review
Removed extra comment as per Josh's comment. Removed spurious #include in nsChildView.mm.
Attachment #419389 - Attachment is obsolete: true
Attachment #419428 - Flags: review?(Olli.Pettay)
Attachment #419389 - Flags: review?(Olli.Pettay)
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?
Attachment #419428 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 14

9 years ago
Posted patch patch v3.2Splinter Review
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?
Attachment #419428 - Attachment is obsolete: true

Comment 15

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

Updated

9 years ago
Attachment #419481 - Flags: superreview?(Olli.Pettay)
Tom, could you still list here ways to test this manually, so that QA can easily
verify that this works on OSX.
Attachment #419481 - Flags: superreview?(Olli.Pettay) → superreview+
(Assignee)

Comment 17

9 years ago
(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.
(Assignee)

Comment 18

9 years ago
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

9 years ago
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/bee6729280d8
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Target Milestone: --- → mozilla1.9.3a1

Comment 21

9 years ago
backed out, we should fix this problem and do a try server run with the fixed patch
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 22

9 years ago
(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.
(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).
(Assignee)

Comment 24

9 years ago
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.

Updated

9 years ago
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 26

9 years ago
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.
(Assignee)

Comment 27

9 years ago
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.
You need to log in before you can comment on or make changes to this bug.