308305 - implement nsIFileProtocolHandler::ReadURLFile for GNOME

Status: RESOLVED FIXED

(Core :: Networking: File, defect)

Description

[Christian Persch (GNOME) (away; not receiving bug mail)]

Bug 69114 added ReadURLFile to read windows link files, and bug 287603
implemented it for OS/2. What's missing is an implementation of ReadURLFile for
GNOME/KDE-style .desktop files
[http://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-0.9.4.html].
What we're interested in here is .desktop files with Type=Link.

Steps to reproduce:
0) Open a web page in firefox
1) Drag a link to your GNOME desktop
2) Drag the newly created object from desktop into the browser

Results:
Tries to open the .desktop file with an external program, instead of loading the
link contained in the file.

Remaining problems with the patch:
- the URL bar will show the file:// URL of the dragged .desktop file instead of
the loaded page URL. This seems to be a problem somewhere in docshell?
- it only works if you drag .desktop from file:/// not from arbitrary gnome-vfs
locations

Comment 1

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: proposed fix

Comment 2

[Darin Fisher]

Comment on attachment 195856 [details] [diff] [review]
proposed fix

biesi: can you do first review on this?  thx!

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 195856 [details] [diff] [review]
proposed fix

+    nsresult rv;
+    nsCAutoString leafName;
+    rv = aFile->GetNativeLeafName(leafName);

please move the rv declaration to the line where it's first used

Hmm, I wonder why URLs are defined to be ASCII-only...

thanks for doing this. r=biesi

Comment 4

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: updated patch with change requested by the reviewer

Comment 5

[Darin Fisher]

Comment on attachment 195881 [details] [diff] [review]
updated patch with change requested by the reviewer

>Index: netwerk/protocol/file/src/nsFileProtocolHandler.cpp

>+        !StringEndsWith(leafName, NS_LITERAL_CSTRING(".desktop"),
>+                        nsCaseInsensitiveCStringComparator()))

case insensitive really?  is there by chance a specification somewhere
that you can link to in the code?  a comment with an URL would be nice.


>+    nsCOMPtr<nsILocalFile> file(do_QueryInterface(aFile, &rv));
>+    if (NS_FAILED(rv))
>+        return NS_ERROR_NOT_AVAILABLE;
>+
>+    nsINIParser parser;
>+    rv = parser.Init(file);
>+    if (NS_FAILED(rv))
>+        return NS_ERROR_NOT_AVAILABLE;

It's generally better to propogate error codes unless there is good
reason to map them all to one error code like this.  The API says:

  * @throw NS_ERROR_NOT_AVAILABLE if the OS does not support such files.
  * @throw NS_ERROR_NOT_AVAILABLE if this file is not an internet shortcut.

I don't think failure for nsIFile to QI to nsILocalFile is covered
by either of those cases, so I would just propogate |rv|.  Same goes
for nsINIParser::Init failing.

Comment 6

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #5)
> (From update of attachment 195881 [details] [diff] [review] [edit])
> >Index: netwerk/protocol/file/src/nsFileProtocolHandler.cpp
> 
> >+        !StringEndsWith(leafName, NS_LITERAL_CSTRING(".desktop"),
> >+                        nsCaseInsensitiveCStringComparator()))
> 
> case insensitive really?  is there by chance a specification somewhere
> that you can link to in the code?  a comment with an URL would be nice.

http://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-0.9.4.html#basic-format
says:
" These desktop entry files should have the extension .desktop. Determining file
type on basis of extension makes determining the file type very easy and quick.
When no file extension is present, the desktop system should fall back to
recognition via "magic detection"."

I talked with biesi about this on irc, and we agreed that it would be okay to
support only files with the extension, without doing the magic detection. I
could check it case insensitively though, if you think checking only lowercase
might be a problem? Like this:

    nsAutoString path;
    nsresult rv = aFile->GetPath(path);
    if (NS_FAILED(rv))
        return rv;
    if (path.Length() < 9)
        return NS_ERROR_NOT_AVAILABLE;
    if (!StringTail(path, 8).LowerCaseEqualsLiteral(".desktop"))
        return NS_ERROR_NOT_AVAILABLE;

> It's generally better to propogate error codes unless there is good
> reason to map them all to one error code like this.  The API says:
> 
>   * @throw NS_ERROR_NOT_AVAILABLE if the OS does not support such files.
>   * @throw NS_ERROR_NOT_AVAILABLE if this file is not an internet shortcut.
> 
> I don't think failure for nsIFile to QI to nsILocalFile is covered
> by either of those cases, so I would just propogate |rv|.  Same goes
> for nsINIParser::Init failing.

Fixed in my tree.

(In reply to comment #3)
> Hmm, I wonder why URLs are defined to be ASCII-only...

Interestingly, Nautilus already creates .desktop files with UTF-8 in the URL
field, for example when I drag a link to a non-punycoded IDN domain...
It could be an oversight, I'll ask the relevant list
[http://freedesktop.org/mailman/listinfo/xdg] . 

Comment 7

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: updated patch

Changed to check for ".desktop" extension case-sensitively, and to propagate
return value. Add a link to the spec in a comment.

Comment 8

[Darin Fisher]

Comment on attachment 195917 [details] [diff] [review]
updated patch

Looks good to me, sr=darin

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in netwerk/protocol/file/src/nsFileProtocolHandler.cpp;
/cvsroot/mozilla/netwerk/protocol/file/src/nsFileProtocolHandler.cpp,v  <-- 
nsFileProtocolHandler.cpp
new revision: 1.65; previous revision: 1.64
done

Comment 10

[Christian Persch (GNOME) (away; not receiving bug mail)]

I filed bug 309781 as follow-up for the problem with the location bar that was
mentioned in comment 0.