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

RESOLVED FIXED

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

unspecified
x86
Mac OS X
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

23.03 KB, patch
Stuart Parmenter
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
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.
Flags: blocking1.9+
(Assignee)

Comment 1

11 years ago
Created attachment 270658 [details] [diff] [review]
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.
Attachment #270658 - Flags: review?(cbarrett)
(Assignee)

Updated

11 years ago
Attachment #270658 - Flags: review?(cbarrett) → review?(stuart.morgan)
(Assignee)

Comment 2

11 years ago
+ 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?
(Assignee)

Updated

11 years ago
Summary: re-factor shared code between cocoa clipboard and drag services → clean up clipboard service given changes in bug 385116, port fixes
(Assignee)

Updated

11 years ago
Summary: clean up clipboard service given changes in bug 385116, port fixes → clean up clipboard service given drag service rewrite (385116), port fixes
(Assignee)

Comment 3

11 years ago
+      // 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

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

r=me with the comment changes we discussed.
Attachment #270658 - Flags: review?(stuart.morgan) → review+
(Assignee)

Comment 5

11 years ago
Created attachment 270683 [details] [diff] [review]
fix v1.1

addresses my own comments + smorgan's
Attachment #270658 - Attachment is obsolete: true
Attachment #270683 - Flags: superreview?(pavlov)

Comment 6

11 years ago
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.
Attachment #270683 - Flags: superreview?(pavlov) → superreview-
(Assignee)

Comment 7

11 years ago
Created attachment 270698 [details] [diff] [review]
fix v1.2

Fixes pav's comments, plus gets rid of a whole bunch of other strcmp usage.
Attachment #270683 - Attachment is obsolete: true
Attachment #270698 - Flags: superreview?(pavlov)
(Assignee)

Updated

11 years ago
Attachment #270698 - Attachment is patch: true
Attachment #270698 - Attachment mime type: application/octet-stream → text/plain

Updated

11 years ago
Attachment #270698 - Flags: superreview?(pavlov) → superreview+
(Assignee)

Comment 8

11 years ago
landed on trunk
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.