Bug 25847 - Clean up ClipboardMac
Summary: Clean up ClipboardMac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-17 22:30 PDT by Eric Seidel (no email)
Modified: 2009-05-18 23:18 PDT (History)
2 users (show)

See Also:


Attachments
ClipboardMac cleanup (8.50 KB, patch)
2009-05-17 23:18 PDT, Eric Seidel (no email)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-05-17 22:30:33 PDT
Clean up ClipboardMac

I was in ClipboardMac today and decided to do a little cleanup.

I'm mostly just moving code, however there are 3 changes I made while moving.

1.  I broke out logic into absoluteURLsFromPasteboard which returns an NSArray (different!) so that we can use -[NSArray componentsJoinedByString] which is cleaner, and allows easier access to the full list of file urls from the pasteboard (which is more useful than the string concatenation of them).
2.      unsigned count = (type == "URL") ? 1 : [fileList count]; is now an explicit check for "URL", before it was a check for != "text/uri-list" which was much more confusing in my opinion (since text/uri-list and URL are the only two types which map to NSURLPboardType as far as I can tell.
3.  I removed an extra if (!types) check, right before [types count], in Obj-C messaging nil will return 0 (size of a pointer), so it's safe to message nil here and expect it to return 0.
Comment 1 Eric Seidel (no email) 2009-05-17 23:18:03 PDT
Created attachment 30437 [details]
ClipboardMac cleanup

 2 files changed, 90 insertions(+), 55 deletions(-)
Comment 2 Darin Adler 2009-05-18 09:07:38 PDT
Comment on attachment 30437 [details]
ClipboardMac cleanup

I think it's unnecessary to shout "NOTE".

There's no reason to have a default value for the "onlyFirstURL" boolean.

> +    if (!absoluteURLs) {
> +        if ([availableTypes containsObject:NSURLPboardType]) {

Using && here would avoid an otherwise-unnecessary level of if nesting. But I think early return would be even better. One of the advantages of breaking something into a separate function.

> +        // "URL" and "text/url-list" both map to NSURLPboardType in cocoaTypeFromMIMEType(), "URL" only wants the first URL

It's a little strange to have this same comment twice.

> +    unsigned count = [types count];
> +    for (unsigned i = 0; i < count; i++) {

As long as you are touching this code, the correct type here is NSUInteger rather than unsigned. Same for the other count and index in absoluteURLsFromPasteboardFilenames.

r=me
Comment 3 Eric Seidel (no email) 2009-05-18 21:17:52 PDT
I made all your suggested changes and ran the layout tests again.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/mac/ClipboardMac.mm
Committed r43850

Thanks!
Comment 4 David Kilzer (:ddkilzer) 2009-05-18 22:03:22 PDT
http://trac.webkit.org/changeset/43850

Tiger build fix:

$ git svn dcommit
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/mac/ClipboardMac.mm
Committed r43852

http://trac.webkit.org/changeset/43852
Comment 5 Eric Seidel (no email) 2009-05-18 23:18:31 PDT
Thank you David.