Bug 81516 – REGRESSION (r107435) URLs copied from WebKit apps aren't in the right pasteboard format

• Component: HTML Editing
• Status: RESOLVED FIXED

[Brady Eidson]

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>

[Brady Eidson]

Created attachment 132669 [details]
Patch with API test

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Enrica Casucci]

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.

[Brady Eidson]

(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.

[Brady Eidson]

(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?

[Enrica Casucci]

(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.

[Brady Eidson]

(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.  ;)

[Gustavo Noronha (kov)]

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

[Brady Eidson]

Created attachment 132690 [details]
Patch v2 - With API test

[Enrica Casucci]

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.

[Gustavo Noronha (kov)]

In case you're wondering the failure gtk-ews reported for the first patch is bogus.

[Brady Eidson]

(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.

[Brady Eidson]

(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!

[Brady Eidson]

Enrica reviewing the greatly simplified patch spawned from her suggestion on IRC.

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