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

VERIFIED FIXED in Firefox 54

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: carmudgeon, Assigned: spohl)

Tracking

({regression})

53 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 wontfix, firefox54 verified, firefox55 verified)

Details

(Whiteboard: tpi:+)

Attachments

(3 attachments, 3 obsolete attachments)

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.
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?
Status: UNCONFIRMED → NEW
Component: Untriaged → Widget: Cocoa
Ever confirmed: true
Flags: needinfo?(spohl.mozilla.bugs)
Keywords: regression
Product: Firefox → Core
Looking into it.
Assignee: nobody → spohl.mozilla.bugs
Duplicate of this bug: 1358797
Posted patch 1. PatchSplinter Review
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.
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #8867833 - Flags: review?(mstange)
Attachment #8867833 - Flags: review?(mstange) → review+
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.
Attachment #8868655 - Flags: review?(mstange)
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: tpi:+
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];
Addressed feedback. This looks much cleaner.

Note the manual retain in nsClipboard::IsStringType.
Attachment #8868655 - Attachment is obsolete: true
Attachment #8868655 - Flags: review?(mstange)
Attachment #8870157 - Flags: review?(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
Attachment #8870157 - Flags: review?(mstange) → review+
(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!
Attachment #8870157 - Attachment is obsolete: true
Attachment #8870177 - Flags: review?(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;
(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.
Attachment #8870177 - Attachment is obsolete: true
Attachment #8870177 - Flags: review?(mstange)
Attachment #8870231 - Flags: review?(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.
Attachment #8870231 - Flags: review?(mstange) → review+
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
https://hg.mozilla.org/mozilla-central/rev/06d42295ca42
https://hg.mozilla.org/mozilla-central/rev/c45eb8e4d71a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Please request uplift on this if you feel it's appropriate.
Flags: needinfo?(spohl.mozilla.bugs)
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
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #8867833 - Flags: approval-mozilla-aurora?
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 on attachment 8870231 [details] [diff] [review]
2. Dynamically generate UTIs when necessary

see comment 17
Attachment #8870231 - Flags: approval-mozilla-beta?
Comment on attachment 8867833 [details] [diff] [review]
1. Patch

see comment 16
Attachment #8867833 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Hi Emil,
Can you help check if this issue is fixed in the latest nightly?
Flags: needinfo?(emil.pasca)
Depends on: 1367720
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?
Flags: needinfo?(emil.pasca) → needinfo?(spohl.mozilla.bugs)
Able to reproduce, looking into it.
Flags: needinfo?(spohl.mozilla.bugs)
Depends on: 1368077
(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.
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')
Flags: needinfo?(spohl.mozilla.bugs)
(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.
(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.
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #8872485 - Flags: review+
Comment on attachment 8867833 [details] [diff] [review]
1. Patch

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

Should be in 54.0b13
Attachment #8867833 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8870231 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
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.

:-)
(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.
You need to log in before you can comment on or make changes to this bug.