386225 - clean up clipboard service given drag service rewrite (385116), port fixes

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Josh Aas]

The last-minute patch for bug 385116 undid a lot of the work done by bug 358094. We need to re-do the re-factoring, but this time at a finer grain because of greater differences between the drag and clipboard services.

Comment 1

[Josh Aas]

Attachment: fix v1.0

In doing this I ended up rewriting the guts of the clipboard service. I fixed several bugs while optimizing and simplifying a bunch of code. I used a lot of the same lessons learned from rewriting the drag service recently, as well as my realization that the base clipboard service impl caches the transferrable given to it unlike the base drag service impl.

Some more detailed notes:
- This fixes a major ordering problem that could result in pasting the wrong data if somebody does a copy in gecko, a copy in another application, and then pastes into Gecko. Previously we might have used data from a cached transferable when we should have taken data from the clipboard that originated in another application.
- This rewrite doesn't put data on the native clipboard unnecessarily since we can depend on having cached any data that originated in gecko, which won't be of use to anyone else.

Comment 2

[Josh Aas]

+ void* clipboardDataPtr = (unsigned char*)malloc(dataLength);

This is obviously wrong - I forgot to remove the cast when I changed the data type. Will fix on checkin. Why doesn't gcc warn me about things like that?

Comment 3

[Josh Aas]

+      // We have never supported this on Mac OS X, we could someday but nobody does this. We want this
+      // test here so that we don't try to get this as a custom type.

Also, this comment is wrong, I forgot to change it.

Comment 4

[Stuart Morgan]

Comment on attachment 270658 [details] [diff] [review]
fix v1.0

r=me with the comment changes we discussed.

Comment 5

[Josh Aas]

Attachment: fix v1.1

addresses my own comments + smorgan's

Comment 6

[Stuart Parmenter]

Comment on attachment 270683 [details] [diff] [review]
fix v1.1

(sorry these comments are bottom-up)

+      // be nice to Carbon apps, normalize the receiver�s contents using Form C.

it is probably best if we don't use odd charsets for comments?


+    nsXPIDLCString flavorStr;
+    currentFlavor->ToString(getter_Copies(flavorStr)); // i has a flavr
+
+    // printf("looking for clipboard data of type %s\n", flavorStr.get());
+
+    if (strcmp(flavorStr, kUnicodeMime) == 0) {

this isn't safe.  What if flavorStr is shorter than kUnicodeMime?  I think you could do flavorStr.Equals() (not sure about xpidlstrings..)

+private:
+  int changeCount; // this is always set to the native change count after any clipboard modifications
 
do you want to use somethign like mChangeCount?  It is hard to tell that this is a member variable.

Comment 7

[Josh Aas]

Attachment: fix v1.2

Fixes pav's comments, plus gets rid of a whole bunch of other strcmp usage.

Comment 8

[Josh Aas]

landed on trunk