Last Comment Bug 682360 - Merge nsILocalFile and nsIFile interfaces
: Merge nsILocalFile and nsIFile interfaces
Status: RESOLVED FIXED
: addon-compat, arch, dev-doc-needed
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Geoff Lankow (:darktrojan)
:
: Nathan Froyd [:froydnj]
Mentors:
Depends on:
Blocks: 739001 749930
  Show dependency treegraph
 
Reported: 2011-08-26 12:06 PDT by Alex Vincent [:WeirdAl]
Modified: 2012-06-07 08:16 PDT (History)
12 users (show)
geoff: in‑testsuite-
geoff: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (17.66 KB, patch)
2012-03-24 16:48 PDT, Geoff Lankow (:darktrojan)
benjamin: review+
Details | Diff | Splinter Review

Description Alex Vincent [:WeirdAl] 2011-08-26 12:06:48 PDT
The nsILocalFile interface is the only one inheriting from nsIFile, and I believe every implementation of nsIFile in the Mozilla code also implements nsILocalFile.  The separation of these two interfaces is probably more painful than it should be.

This should be considered somewhat risky, as anything in the Mozilla code, or in extensions, dealing with file I/O could be affected.  If we do this, we might want to keep the deprecated interface around for a little while so external developers can update their code bases.
Comment 1 Geoff Lankow (:darktrojan) 2012-03-24 16:48:28 PDT
Created attachment 609045 [details] [diff] [review]
patch
Comment 2 Benjamin Smedberg [:bsmedberg] 2012-03-30 09:33:16 PDT
Comment on attachment 609045 [details] [diff] [review]
patch

There is no need to change NS_LOCAL_FILE_CID, please don't. Please add [builtinclass] to nsIFile so that nobody tries to implement it in script. r=me with those changes.
Comment 3 Alex Vincent [:WeirdAl] 2012-03-30 10:54:04 PDT
Would it be practical to add [deprecated] to nsILocalFile, as it is for nsIJSXMLHttpRequest?

http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIXMLHttpRequest.idl#398
Comment 4 Benjamin Smedberg [:bsmedberg] 2012-03-30 11:32:42 PDT
Perhaps later, but there are plenty of uses in the tree which would currently just spam warning noise for something harmless.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-03 13:53:32 PDT
Comment on attachment 609045 [details] [diff] [review]
patch

I don't think you need my review for this, Benjamin's is sufficient.
Comment 6 Geoff Lankow (:darktrojan) 2012-04-04 04:11:17 PDT
Okay then, landed with those changes
https://hg.mozilla.org/integration/mozilla-inbound/rev/82c7bdff95d4
Comment 7 Benjamin Smedberg [:bsmedberg] 2012-04-04 05:33:19 PDT
This change should have very little compat risk for JS addons or recompiled C++ addons, but we should nevertheless note it in the "Firefox 14 for Developers" page and update the interface docs on MDC. Geoff is that something you can do?
Comment 8 Geoff Lankow (:darktrojan) 2012-04-04 13:32:20 PDT
Yes, I'll do that when I get time.
Comment 10 Geoff Lankow (:darktrojan) 2012-04-12 06:22:22 PDT
Updated:
https://developer.mozilla.org/en/Firefox_14_for_developers
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsILocalFile
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIFile
(I've left the method descriptions on the nsILocalFile page for the time being to avoid duplication)
Comment 11 Alex Keybl [:akeybl] 2012-05-28 09:41:58 PDT
Just saw Geoff's post on dev-platform

> In most cases running 'sed s/nsILocalFile\b/nsIFile/g' on your code will be 
> sufficient, although you might end up with pointless do_QueryInterface calls that
> you should weed out.

If that's the case, can we leave some sort of shim around so that add-on developers don't need to make any changes and binary compatibility is maintained?

We typically find out about the impact of bugs that remove interfaces very late in the cycle, and backing out the change at that point requires a UUID change (which can hurt add-on compatibility even more).
Comment 12 Benjamin Smedberg [:bsmedberg] 2012-05-29 11:59:27 PDT
Note that we did not remove the nsILocalFile interface, we merely moved all of its guts into nsIFile. So nsILocalFile *is* the shim, and AFAIK there are no plans to remove it any time soon so that JS extensions will continue to work. C++ extensions may require changes not from this bug but from bug 749930 where we're changing some classes to use nsIFile exclusively. But I don't think that there is any additional shimming that we need to do for extensions.

Note You need to log in before you can comment on or make changes to this bug.