Bug 81516 - REGRESSION (r107435) URLs copied from WebKit apps aren't in the right pasteboard format
Summary: REGRESSION (r107435) URLs copied from WebKit apps aren't in the right pastebo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-03-19 09:14 PDT by Brady Eidson
Modified: 2012-03-19 16:21 PDT (History)
3 users (show)

See Also:


Attachments
Patch with API test (23.33 KB, patch)
2012-03-19 14:16 PDT, Brady Eidson
gns: commit-queue-
Details | Formatted Diff | Diff
Patch v2 - With API test (17.71 KB, patch)
2012-03-19 15:41 PDT, Brady Eidson
enrica: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2012-03-19 09:14:17 PDT
REGRESSION (r107435) URLs copied from WebKit apps aren't in the right pasteboard format


In writeURLForTypes in PasteboardMac.mm

Before 107435:
if ([types containsObject:NSURLPboardType])
    [cocoaURL writeToPasteboard:pasteboard];

After 107435:
if (types.contains(String(NSURLPboardType)))
    platformStrategies()->pasteboardStrategy()->setStringForType([cocoaURL absoluteString], NSURLPboardType, pasteboardName);

URLs have to paste themselves to the pasteboard to get the right plist format the system expects.

Patch forthcoming.

In radar as <rdar://problem/10848575>
Comment 1 Brady Eidson 2012-03-19 14:16:54 PDT
Created attachment 132669 [details]
Patch with API test
Comment 2 Ryosuke Niwa 2012-03-19 14:21:46 PDT
Comment on attachment 132669 [details]
Patch with API test

View in context: https://bugs.webkit.org/attachment.cgi?id=132669&action=review

The change looks good to me but I'll let mac port folks take a look.

> Source/WebCore/ChangeLog:4
> +        REGRESSION (r107435) URLs copied from WebKit apps aren't in the right pasteboard format

Summary should appear before the bug urls.
Also, people tend to put rdar and bugzilla urls on separate lines.
Comment 3 Ryosuke Niwa 2012-03-19 14:21:54 PDT
Comment on attachment 132669 [details]
Patch with API test

View in context: https://bugs.webkit.org/attachment.cgi?id=132669&action=review

The change looks good to me but I'll let mac port folks take a look.

> Source/WebCore/ChangeLog:4
> +        REGRESSION (r107435) URLs copied from WebKit apps aren't in the right pasteboard format

Summary should appear before the bug urls.
Also, people tend to put rdar and bugzilla urls on separate lines.
Comment 4 Enrica Casucci 2012-03-19 14:26:24 PDT
Comment on attachment 132669 [details]
Patch with API test

The patch is correct, but I think you could simplify it. There is no need to add a new message for webkit2, you can use the same message used to set the string type and implement a different behavior based on the pasteboard type.
This way you don't need to add a new message and you need no changes to platform strategies. The only changes required are in WebContextMac.mm in setPasteboardString.
Comment 5 Brady Eidson 2012-03-19 14:29:10 PDT
(In reply to comment #4)
> (From update of attachment 132669 [details])
> The patch is correct, but I think you could simplify it. There is no need to add a new message for webkit2, you can use the same message used to set the string type and implement a different behavior based on the pasteboard type.
> This way you don't need to add a new message and you need no changes to platform strategies. The only changes required are in WebContextMac.mm in setPasteboardString.

I can see that being reasonable.  It will - unfortunately - make this mechanism asymmetrical as the "pasting app" fix in http://trac.webkit.org/changeset/111080 added a new message.

I really wish we had the ability to message KURLs but adding that is outside the scope of this patch.

I'll do what you suggest.
Comment 6 Brady Eidson 2012-03-19 14:38:55 PDT
(In reply to comment #3)
> (From update of attachment 132669 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132669&action=review
> 
> The change looks good to me but I'll let mac port folks take a look.
> 
> > Source/WebCore/ChangeLog:4
> > +        REGRESSION (r107435) URLs copied from WebKit apps aren't in the right pasteboard format
> 
> Summary should appear before the bug urls.
> Also, people tend to put rdar and bugzilla urls on separate lines.

I've done my ChangeLogs like this for years in hundreds of patches.  It's also not remotely difficult to find many other contributors who've done the same.

I actually don't believe we have style guidelines along these lines, and a cursory glance at past ChangeLogs show there is no standard in practice.

Is there some standard we are now trying to enforce and I've missed where it was discussed/published?
Comment 7 Enrica Casucci 2012-03-19 14:48:31 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 132669 [details] [details])
> > The patch is correct, but I think you could simplify it. There is no need to add a new message for webkit2, you can use the same message used to set the string type and implement a different behavior based on the pasteboard type.
> > This way you don't need to add a new message and you need no changes to platform strategies. The only changes required are in WebContextMac.mm in setPasteboardString.
> 
> I can see that being reasonable.  It will - unfortunately - make this mechanism asymmetrical as the "pasting app" fix in http://trac.webkit.org/changeset/111080 added a new message.
> 
> I really wish we had the ability to message KURLs but adding that is outside the scope of this patch.
> 
> I'll do what you suggest.

I had not seen the other patch. I wish I had a chance to chime in.
Comment 8 Brady Eidson 2012-03-19 14:49:05 PDT
(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 132669 [details] [details] [details])
> > > The patch is correct, but I think you could simplify it. There is no need to add a new message for webkit2, you can use the same message used to set the string type and implement a different behavior based on the pasteboard type.
> > > This way you don't need to add a new message and you need no changes to platform strategies. The only changes required are in WebContextMac.mm in setPasteboardString.
> > 
> > I can see that being reasonable.  It will - unfortunately - make this mechanism asymmetrical as the "pasting app" fix in http://trac.webkit.org/changeset/111080 added a new message.
> > 
> > I really wish we had the ability to message KURLs but adding that is outside the scope of this patch.
> > 
> > I'll do what you suggest.
> 
> I had not seen the other patch. I wish I had a chance to chime in.

You were singing in New York.  ;)
Comment 9 Gustavo Noronha (kov) 2012-03-19 15:39:00 PDT
Comment on attachment 132669 [details]
Patch with API test

Attachment 132669 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11989371
Comment 10 Brady Eidson 2012-03-19 15:41:59 PDT
Created attachment 132690 [details]
Patch v2 - With API test
Comment 11 Enrica Casucci 2012-03-19 15:49:23 PDT
Comment on attachment 132690 [details]
Patch v2 - With API test

It just occurred to me that you can make it even simpler and just change PlatformPasteboard::setStringForType and hide that logic in there. But I'm fine either way.
Comment 12 Gustavo Noronha (kov) 2012-03-19 15:55:18 PDT
In case you're wondering the failure gtk-ews reported for the first patch is bogus.
Comment 13 Brady Eidson 2012-03-19 15:55:29 PDT
(In reply to comment #11)
> (From update of attachment 132690 [details])
> It just occurred to me that you can make it even simpler and just change PlatformPasteboard::setStringForType and hide that logic in there. But I'm fine either way.

*LOL*  YUP.  I'm going to do it that way.
Comment 14 Brady Eidson 2012-03-19 15:55:44 PDT
(In reply to comment #12)
> In case you're wondering the failure gtk-ews reported for the first patch is bogus.

It appeared to be... thanks for confirming!
Comment 15 Brady Eidson 2012-03-19 16:21:18 PDT
Enrica reviewing the greatly simplified patch spawned from her suggestion on IRC.

Landed in http://trac.webkit.org/changeset/111267