1358075 - Fx53 no longer assigns user-friendly names to .webloc files on OS X

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[carmudgeon]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170413192749

Steps to reproduce:

Select site icon in URL bar, drag to location in Finder with Fx53.  Issue not exhibited in Fx52 or earlier.


Actual results:

Resulting .webloc file shortcut is now named using the raw URL, often truncated.


Expected results:

.webloc file should be assigned the name specified in the Page Title, as in previous versions, and other browsers.

A raw URL has scant descriptive properties, is not user-friendly, and with modern dynamic URLs, are often indecipherable.  This can cause a serious disruption to workflows for users who rely on such shortcuts.

Comment 1

[Emil Pasca [:emilpasca]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:55.0) Gecko/20100101 Firefox/55.0

I have tested this issue on Mac OS 10.11 with the latest Firefox release (53.0) and the latest Nightly (55.0a1-20170424030211) and managed to reproduce it.
When dragging a site icon into a location in Finder, a .webloc file shortcut is created, named using the raw URL, as mentioned in the description.

I've performed a regression, the results is below:

Last good revision: 09a4e9d9b8fa5b3ca47a9a582575db2ed38dc260
First bad revision: 82ca4696b3425b6b5674ef48422d93a63dda2554
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=09a4e9d9b8fa5b3ca47a9a582575db2ed38dc260&tochange=82ca4696b3425b6b5674ef48422d93a63dda2554

Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1235162

Stephen, can you please take a look at this?

Comment 2

[Stephen A Pohl [:spohl]]

Looking into it.

Comment 4

[Stephen A Pohl [:spohl]]

Attachment: 1. Patch

This is the first of two patches that restores the broken functionality. The second patch will do some general cleanup in our drag&drop code and create UTI types dynamically when necessary. I will post it as soon as I have it fully tested.

Comment 5

[Stephen A Pohl [:spohl]]

Attachment: 2. Dynamically create UTIs when necessary

This may be overkill right now, but would hopefully save us headaches in the future. The goal here is to provide a helper function |nsClipboard::CreateUTIFromPboardType| that takes responsibility for returning a valid UTI type to be used with the drag pasteboard. Right now, we only really need this for |kUrlsWithTitlesPboardType| (introduced in patch 1 in this bug), i.e. |@"WebURLsWithTitlesPboardType"|, but Apple's documentation says that pasteboard types will move to dynamic types in the future.

It can be difficult to figure out if a type is meant to be used directly (like |NSPasteboardTypeString|), or created dynamically (like |kUrlsWithTitlesPboardType|). I was hoping that abstracting this complexity away and putting it into |nsClipboard::CreateUTIFromPboardType| would help us in the future.

Since the native |UTTypeCreatePreferredIdentifierForTag| creates a CFStringRef, the caller is responsible for releasing the object. I haven't been able to come up with a better way than manually retaining the NSString* for types that are meant to be used directly, such as |NSPasteboardTypeString|. This makes for some cumbersome code. I thought the value of having this complexity abstracted outweighed the inconvenience of the cumbersome code with the manual release of NSString* objects, but I'm happy to consider better options.

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8868655 [details] [diff] [review]
2. Dynamically create UTIs when necessary

Review of attachment 8868655 [details] [diff] [review]:
-----------------------------------------------------------------

We could even add a new Objective C class called UTIString and convert nsClipboard::CreateUTIFromPboardType into a static method on that type.

So you'd do
  NSString* myUTI = [UTIString fromPboardType:myType];

and it would be more obvious that the result is autoreleased because that's how these class methods usually behave.

::: widget/cocoa/nsClipboard.mm
@@ +333,5 @@
>    NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
>  }
>  
> +NSString*
> +nsClipboard::CreateUTIFromPboardType(NSString* aPboardType)

I think autoreleasing would be a better match for us here.

Let's remove the "Create" prefix from the function name and add a comment that the return value is autoreleased. That way, callers can use it like this:

void someFunction()
{
  NSString* myUTI = nsClipboard::UTIFromPboardType(someType);
  // myUTI is going to survive for at least the duration of this function,
  // and it will automatically be destroyed some time afterwards

  // If I want to store myUTI somewhere, I need to retain it.
}

@@ +347,5 @@
> +      [aPboardType isEqualToString:NSPasteboardTypeHTML] ||
> +      [aPboardType isEqualToString:NSPasteboardTypeRTF] ||
> +      [aPboardType isEqualToString:NSPasteboardTypeTIFF] ||
> +      [aPboardType isEqualToString:NSPasteboardTypePNG]) {
> +    return [aPboardType retain];

and then make this

    return [NSString stringWithString:aPboardType];

because that's also going to give you something that's autoreleased.

@@ +353,5 @@
> +  NSString* dynamicType =
> +    (NSString*)UTTypeCreatePreferredIdentifierForTag(kUTTagClassNSPboardType,
> +                                                     (CFStringRef)aPboardType,
> +                                                     kUTTypeData);
> +  return dynamicType;

and make this

  return [dynamicType autorelease];

Comment 7

[Stephen A Pohl [:spohl]]

Attachment: 2. Dynamically generate UTIs when necessary

Addressed feedback. This looks much cleaner.

Note the manual retain in nsClipboard::IsStringType.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8870157 [details] [diff] [review]
2. Dynamically generate UTIs when necessary

Review of attachment 8870157 [details] [diff] [review]:
-----------------------------------------------------------------

Indeed, that looks much better.

I'm having second thoughts on the UTIString class though; the fact that [UTIString fromPboardType:...] doesn't return a UTIString object might be a bit surprising. Maybe we should make UTIString inherit from NSString and actually return a UTIString* from that method? Won't make a difference in practice, but it'll make the illusion a bit more complete.

::: widget/cocoa/nsChildView.mm
@@ +3269,2 @@
>  
>      [NSApp registerServicesMenuSendTypes:sendTypes returnTypes:returnTypes];

I think this could be simplified even more:
NSArray* types = @[stringType, htmlType];
[NSApp registerServicesMenuSendTypes:types returnTypes:types];
// types is autoreleased so don't call [types release] here

Comment 9

[Stephen A Pohl [:spohl]]

Attachment: 2. Dynamically generate UTIs when necessary

(In reply to Markus Stange [:mstange] from comment #8)
> Comment on attachment 8870157 [details] [diff] [review]
> 2. Dynamically generate UTIs when necessary
> 
> Review of attachment 8870157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Indeed, that looks much better.
> 
> I'm having second thoughts on the UTIString class though; the fact that
> [UTIString fromPboardType:...] doesn't return a UTIString object might be a
> bit surprising. Maybe we should make UTIString inherit from NSString and
> actually return a UTIString* from that method? Won't make a difference in
> practice, but it'll make the illusion a bit more complete.

Agreed. Sending back for a final look to make sure that this is what you had in mind.

> ::: widget/cocoa/nsChildView.mm
> @@ +3269,2 @@
> >  
> >      [NSApp registerServicesMenuSendTypes:sendTypes returnTypes:returnTypes];
> 
> I think this could be simplified even more:
> NSArray* types = @[stringType, htmlType];
> [NSApp registerServicesMenuSendTypes:types returnTypes:types];
> // types is autoreleased so don't call [types release] here

Fixed, thanks!

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8870177 [details] [diff] [review]
2. Dynamically generate UTIs when necessary

Review of attachment 8870177 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/cocoa/nsClipboard.mm
@@ +49,5 @@
> +      [aType isEqualToString:NSPasteboardTypeHTML] ||
> +      [aType isEqualToString:NSPasteboardTypeRTF] ||
> +      [aType isEqualToString:NSPasteboardTypeTIFF] ||
> +      [aType isEqualToString:NSPasteboardTypePNG]) {
> +    return (UTIString*)[NSString stringWithString:aType];

Does [UTIString stringWithString:aType] work? If not, can you add it and make it call through to the super implementation?

@@ +55,5 @@
> +  NSString* dynamicType =
> +    (NSString*)UTTypeCreatePreferredIdentifierForTag(kUTTagClassNSPboardType,
> +                                                     (CFStringRef)aType,
> +                                                     kUTTypeData);
> +  return (UTIString*)[dynamicType autorelease];

I'd prefer:
UTIString* result = [UTIString stringWithString:dynamicType];
[dynamicType release];
return result;

Comment 11

[Stephen A Pohl [:spohl]]

Attachment: 2. Dynamically generate UTIs when necessary

(In reply to Markus Stange [:mstange] from comment #10)
> Comment on attachment 8870177 [details] [diff] [review]
> 2. Dynamically generate UTIs when necessary
> 
> Review of attachment 8870177 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/cocoa/nsClipboard.mm
> @@ +49,5 @@
> > +      [aType isEqualToString:NSPasteboardTypeHTML] ||
> > +      [aType isEqualToString:NSPasteboardTypeRTF] ||
> > +      [aType isEqualToString:NSPasteboardTypeTIFF] ||
> > +      [aType isEqualToString:NSPasteboardTypePNG]) {
> > +    return (UTIString*)[NSString stringWithString:aType];
> 
> Does [UTIString stringWithString:aType] work? If not, can you add it and
> make it call through to the super implementation?

Neither of these two actually worked because I end up with Obj-C exceptions:
2017-05-22 21:24:55.719 firefox[42487:650592] Mozilla has caught an Obj-C exception [NSInvalidArgumentException: *** initialization method -initWithCharactersNoCopy:length:freeWhenDone: cannot be sent to an abstract object of class UTIString: Create a concrete instance!]

And on second thought, I definitely don't like the C-style cast in my previous patch.

Instead of trying to make this work, I thought a better option would be to rename the class to UTIHelper, with a static method of stringFromPboardType. This should set the proper expectation that an NSString* is returned. Is this okay with you?

> @@ +55,5 @@
> > +  NSString* dynamicType =
> > +    (NSString*)UTTypeCreatePreferredIdentifierForTag(kUTTagClassNSPboardType,
> > +                                                     (CFStringRef)aType,
> > +                                                     kUTTypeData);
> > +  return (UTIString*)[dynamicType autorelease];
> 
> I'd prefer:
> UTIString* result = [UTIString stringWithString:dynamicType];
> [dynamicType release];
> return result;

Fixed.

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8870231 [details] [diff] [review]
2. Dynamically generate UTIs when necessary

Review of attachment 8870231 [details] [diff] [review]:
-----------------------------------------------------------------

I like your solution.

Comment 13

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06d42295ca4263b18f876362477d4959bc3b14ef
Bug 1358075: Display friendly names for .webloc files on OSX after dragging URLs to Finder. r=mstange

https://hg.mozilla.org/integration/mozilla-inbound/rev/c45eb8e4d71a6cccd9d187fdf8480738b368977b
Bug 1358075: Generate UTI types dynamically when needed for the new drag & drop APIs on OSX. r=mstange

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/06d42295ca42
https://hg.mozilla.org/mozilla-central/rev/c45eb8e4d71a

Comment 15

[Nathan Froyd [:froydnj]]

Please request uplift on this if you feel it's appropriate.

Comment 16

[Stephen A Pohl [:spohl]]

Comment on attachment 8867833 [details] [diff] [review]
1. Patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1235162
[User impact if declined]: When URLs are dragged from the browser to Finder, the resulting .webloc files will not have user friendly file names like they used to.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes, preferably. Steps in comment 0.
[List of other uplifts needed for the feature/fix]: bug 1358755
[Is the change risky?]: minimally
[Why is the change risky/not risky?]: The existing code that is being removed in this patch was non-functional after moving to a new drag & drop API in bug 1235162. This patch has been tested and verified manually, but minimal risk remains.
[String changes made/needed]: none

Comment 17

[Stephen A Pohl [:spohl]]

Comment on attachment 8870231 [details] [diff] [review]
2. Dynamically generate UTIs when necessary

Approval Request Comment
[Feature/Bug causing the regression]: bug 1235162
[User impact if declined]: When URLs are dragged from the browser to Finder, the resulting .webloc files will not have user friendly file names like they used to.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes, preferably. Steps in comment 0.
[List of other uplifts needed for the feature/fix]: bug 1358755
[Is the change risky?]: minimally
[Why is the change risky/not risky?]: This patch introduces a helper class that is intended to avoid future regressions in drag & drop functionality on OSX/macOS. The patches in bug 1330470 and bug 1361116, for which I will request uplift approval shortly as well, rely on this patch.
[String changes made/needed]: none

Comment 18

[Julien Cristau [:jcristau]]

Comment on attachment 8870231 [details] [diff] [review]
2. Dynamically generate UTIs when necessary

see comment 17

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 8867833 [details] [diff] [review]
1. Patch

see comment 16

Comment 20

[Gerry Chang [:gchang]]

Hi Emil,
Can you help check if this issue is fixed in the latest nightly?

Comment 21

[Brindusa Tot, Desktop QA]

Tested on the latest Nightly 55.0a1, build ID 20170526030203 on Mac OS X 10.12 and for most of the sites it works correctly. 

But I noticed that for few sites, like https://www.thinkbroadband.com/download or https://dexonline.ro/, when dragging the site icon into a location in Finder, the .webloc file is NOT saved.
Note that for the two sites mentioned, on a Nightly 55.0a1 build from 05/24/2017, the .webloc file is saved, but with the wrong name as described in this bug.

Stephen, could you please take a look?

Comment 22

[Stephen A Pohl [:spohl]]

Able to reproduce, looking into it.

Comment 23

[Stephen A Pohl [:spohl]]

(In reply to Brindusa Tot[:brindusat] from comment #21)
> Tested on the latest Nightly 55.0a1, build ID 20170526030203 on Mac OS X
> 10.12 and for most of the sites it works correctly. 
> 
> But I noticed that for few sites, like
> https://www.thinkbroadband.com/download or https://dexonline.ro/, when
> dragging the site icon into a location in Finder, the .webloc file is NOT
> saved.
> Note that for the two sites mentioned, on a Nightly 55.0a1 build from
> 05/24/2017, the .webloc file is saved, but with the wrong name as described
> in this bug.
> 
> Stephen, could you please take a look?

I will be fixing this in bug 1368077 since the patch builds on other drag & drop code that has landed on m-c recently.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Doesn't apply to beta:
grafting 400837:c45eb8e4d71a "Bug 1358075: Generate UTI types dynamically when needed for the new drag & drop APIs on OSX. r=mstange"
merging widget/cocoa/nsChildView.mm
merging widget/cocoa/nsClipboard.mm
merging widget/cocoa/nsDragService.mm
warning: conflicts while merging widget/cocoa/nsClipboard.mm! (edit, then use 'hg resolve --mark')

Comment 25

[Julien Cristau [:jcristau]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #24)
> Doesn't apply to beta:
> grafting 400837:c45eb8e4d71a "Bug 1358075: Generate UTI types dynamically
> when needed for the new drag & drop APIs on OSX. r=mstange"
> merging widget/cocoa/nsChildView.mm
> merging widget/cocoa/nsClipboard.mm
> merging widget/cocoa/nsDragService.mm
> warning: conflicts while merging widget/cocoa/nsClipboard.mm! (edit, then
> use 'hg resolve --mark')

Looks like a trivial fixup around 9ed543a5a37a, as far as I can tell.

Comment 26

[Stephen A Pohl [:spohl]]

Attachment: Patch 2 for beta

(In reply to Julien Cristau [:jcristau] from comment #25)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #24)
> > Doesn't apply to beta:
> > grafting 400837:c45eb8e4d71a "Bug 1358075: Generate UTI types dynamically
> > when needed for the new drag & drop APIs on OSX. r=mstange"
> > merging widget/cocoa/nsChildView.mm
> > merging widget/cocoa/nsClipboard.mm
> > merging widget/cocoa/nsDragService.mm
> > warning: conflicts while merging widget/cocoa/nsClipboard.mm! (edit, then
> > use 'hg resolve --mark')
> 
> Looks like a trivial fixup around 9ed543a5a37a, as far as I can tell.

That was it. Thanks! This patch applies to beta.

Comment 27

[Julien Cristau [:jcristau]]

Comment on attachment 8867833 [details] [diff] [review]
1. Patch

fix dragging urls to osx finder, verified in nightly, beta54+

Should be in 54.0b13

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0c65e204f2f1
https://hg.mozilla.org/releases/mozilla-beta/rev/35ed9162cbe6

Comment 29

[Emil Ghitta, QA [:emilghitta]]

I have managed to reproduce the issue mentioned in comment 0 using Firefox 53.0.3 (Build Id:20170518000419).

This issue is verified fixed on Firefox 54.0 (Build Id:20170605134926) and Firefox 55.0a1 (Build Id:20170607030206) using macOS 10.12.1 and macOS 10.11.6.

Comment 30

[MrNiceGuy]

Hi All

Sorry I am not tecky at all, I apologize if I am butting in here.

I followed the thread from the Mozilla forums regarding this issue I have had for a couple months now which is still persisting.

I had a couple issues with Mozilla 1. YouTube vid not playing and 2. this issue not being able to drag pdf files to desktop so i uninstalled and re-installed 2 days ago. 

No 1. is fixed the second issue (this one) persists.

I am using:

Mozilla 53.0.3 (64 bit) sorry dont know how to find build.
MacOS Sierra 10.12.5

I see that from this thread there has been some resolution, should we expect that it will be fixed upon the update for Firefox 54?

Thank you all for your efforts.

:-)

Comment 31

[Stephen A Pohl [:spohl]]

(In reply to MrNiceGuy from comment #30)
> I see that from this thread there has been some resolution, should we expect
> that it will be fixed upon the update for Firefox 54?

Yes, the fix will be in Firefox 54.