Last Comment Bug 607114 - Present an easy way to create nsIDOMFiles in chrome
: Present an easy way to create nsIDOMFiles in chrome
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
Mentors:
Depends on: post2.0 668361
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-25 14:45 PDT by Anant Narayanan [:anant]
Modified: 2013-04-04 13:53 PDT (History)
10 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (9.49 KB, patch)
2010-10-25 16:45 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
jonas: review+
Details | Diff | Splinter Review
Patch (8.64 KB, patch)
2011-03-25 17:04 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
jonas: review-
Details | Diff | Splinter Review
Patch (8.41 KB, patch)
2011-04-11 12:05 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
jonas: review+
Details | Diff | Splinter Review

Description Anant Narayanan [:anant] 2010-10-25 14:45:01 PDT
Would be nice for extensions to have access to an easy way of creating an nsIDOMFile object given a local path.

"let domfile = new File(path)" would be awesome, but even exposing nsDOMFile would do.
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-10-25 16:45:53 PDT
Created attachment 485912 [details] [diff] [review]
Patch
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2010-10-28 05:16:45 PDT
Adding a parameter to a standard constructor really isn't acceptable. This makes it impossible for the standard to define a constructor with an argument. If we want to do this, we should do it through a ChromeFile interface or some such.
Comment 3 Anant Narayanan [:anant] 2010-10-28 15:59:49 PDT
Also, it would be nice to be able to create instances of nsIDOMFiles from C++ too. Let's not be "languageist" :)
Comment 4 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-10-28 16:33:03 PDT
You can already create them in C++, the constructors are right here:

http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsDOMFile.h
Comment 5 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-10-28 16:34:35 PDT
Ah, or do you mean using XPCOM? I suppose one way to get two birds with one stone would be to just expose a XPCOM contract-id and a init method. Though init methods are always tricky as we don't want it exposed to content, even by accident.
Comment 6 Anant Narayanan [:anant] 2010-10-28 17:39:46 PDT
Yes, through XPCOM so that add-ons can use it. I know atleast two mozilla labs projects that would benefit from it, I think many other add-on developers would too.

We would have be to careful about not exposing it to content, yes.
Comment 7 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-10-28 17:51:32 PDT
Unfortunately there isn't really a way to make a function exposed only to chrome javascript. What we'd have to do is make the init function exposed to everyone, but if called by content JS make it throw.
Comment 8 Anant Narayanan [:anant] 2010-10-28 18:13:29 PDT
Wouldn't something like

Cc["@mozilla.org/dom-file;1"].createInstance(Ci.nsIDOMFile)

be accessible to chrome javascript only?
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-10-28 18:47:19 PDT
Yes, but then you need to initialize it to an actual file, right? I.e. you'll want to hand it a path or an nsIFile instance or some such I would imagine?
Comment 10 Anant Narayanan [:anant] 2010-10-28 22:07:42 PDT
Yes. So something like:

let domFile = Components.Constructor("@mozilla.org/dom-file;1");
let tmp = new domFile(some_nsIFile);

I imagine this would also be inaccessible to content javascript unless I am missing something. Under the hood, there would an init function that will be called, but I don't see why that has to be exposed to non-chrome code?
Comment 11 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-11-10 12:23:04 PST
Kyle:
Do you want to write a new patch here to use an XPCOM ctor. I'm not actually sure that it is possible to write a XPCOM ctor that takes arguments like the proposal in comment 10.

I'd also be ok with adding a scriptable mozInit function which is only callable my chrome-script. 

Anant:
Would the current patch be ok with you if Kyle doesn't have time to write another patch.
Comment 12 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-11-10 12:26:56 PST
It's not possible to have an XPCOM ctor as clean as this.  I don't think this blocks what Anant is working on at the moment anymore though, so perhaps we just let this lie for a bit and raise the question of what a File() constructor should do in the appropriate standards body.
Comment 13 Anant Narayanan [:anant] 2010-11-10 12:37:57 PST
Hmm, I'm afraid I'm not very familiar with XPCOM internals to know why such a constructor would not be possible. As Kyle mentioned, however, I am currently creating an nsIDOMFile via the nsIDOMHTMLInputElement interface, and so this bug is not blocking :)

In general though, it would be nice to have a clean way to create nsIDOMFile instances both from C++ and JS. I suspect many add-on authors would appreciate it.
Comment 14 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-11-10 13:09:56 PST
(In reply to comment #13)
> Hmm, I'm afraid I'm not very familiar with XPCOM internals to know why such a
> constructor would not be possible. As Kyle mentioned, however, I am currently
> creating an nsIDOMFile via the nsIDOMHTMLInputElement interface, and so this
> bug is not blocking :)

You'd need an init method or similar.  XPConnect handles this magic for you from JS.  It's all possible, just a bit uglier.

> In general though, it would be nice to have a clean way to create nsIDOMFile
> instances both from C++ and JS. I suspect many add-on authors would appreciate
> it.

I agree, at least for JS, though I'm sympathetic to Ms2ger's concerns about not boxing in the FileAPI folks at the standard level.  I'm not sure it's that useful from C++ (unlike, say, XHR), because you can only do "DOMish" things with it and especially since we're trying to move people away from binary addons as much as possible.
Comment 15 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-11-10 14:20:00 PST
The specs will not cover initializing a File using a path as they only cover things that are safe for the web platform.

So I think we should feel free to come up with whatever API we like the best.
Comment 16 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-11-10 14:38:42 PST
Comment on attachment 485912 [details] [diff] [review]
Patch

reviewing this in case we want it (and to get it out of my queue)


>+nsDOMFile::NewFile(nsISupports* *aNewObject)
>+{
>+  nsRefPtr<nsDOMFile> file = new nsDOMFile(NULL);
>+  return CallQueryInterface(file, aNewObject);
>+}

s/NULL/nsnull/


>+nsDOMFile::Initialize(nsISupports* aOwner,
>+                      JSContext* aCx,
>+                      JSObject* aObj,
>+                      PRUint32 aArgc,
>+                      jsval* aArgv)
>+{
>+  if (!nsContentUtils::IsCallerChrome()) {
>+    return NS_ERROR_DOM_SECURITY_ERR; // Real short trip
>+  }
>+
>+  // We expect to get a path to represent as a File object
>+  JSString* str = JS_ValueToString(aCx, aArgv[0]);
>+  NS_ENSURE_TRUE(str, NS_ERROR_XPC_BAD_CONVERT_JS);

You need to check that aArgv > 0 first.

>+  nsCOMPtr<nsILocalFile> localFile;
>+  nsresult rv = NS_NewLocalFile(nsDependentJSString(str),
>+                                PR_FALSE, getter_AddRefs(localFile));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIFile> file = do_QueryInterface(localFile, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  mFile = file;
>+  return NS_OK;

Do we also want to check that the file exists?

r=me with those things fixed. Last one optional.
Comment 17 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-03-24 08:02:05 PDT
Unless anybody here has objections, I'm going to land this for Firefox 5/Gecko 3/2.2/whatever.  I think extensions will find this useful.
Comment 18 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-03-25 17:04:51 PDT
Created attachment 522032 [details] [diff] [review]
Patch

This makes the File constructor only available in the chrome scope and fixes Jonas's other comments.
Comment 19 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-03-25 17:44:13 PDT
Comment on attachment 522032 [details] [diff] [review]
Patch

This is no good. The problem is if chrome code reaches in to a content window and does |new contentWindow.File(...)| then the constructor will remain on the object forever allowing content code to access it.

Instead you should check the principal of the window on which the constructor is being looked up. Probably by passing aWeakOwner to FindConstructorFunc.

Hmm.. actually, check with mrbkap, I think he knows of another way we have of exposing chrome specific properties on windows.
Comment 20 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-04-05 23:34:56 PDT
*** Bug 647653 has been marked as a duplicate of this bug. ***
Comment 21 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-04-11 12:05:25 PDT
Created attachment 525125 [details] [diff] [review]
Patch

Updated with mrbkap's magic.
Comment 22 Alex Vincent [:WeirdAl] 2011-04-11 12:23:13 PDT
This could potentially conflict with jslib.mozdev.org, which has a File constructor as well for its File I/O library.
Comment 23 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-04-11 14:20:15 PDT
That's unrelated to this bug. We already added a File "constructor" object when we exposed the File interface in Firefox 3.5 (or maybe even Firefox 3.0).

This bug just makes that constructor callable. Previously the only thing you could do with it is access constants as well as the prototype for File instances.
Comment 24 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-04-13 20:00:59 PDT
So we realized that we can't hide the "File" object entirely, so we went back to the original solution of just throwing if the caller isn't chrome in the constructor.

http://hg.mozilla.org/mozilla-central/rev/07e66863b615
Comment 25 Eric Shepherd [:sheppy] 2011-04-14 05:49:02 PDT
So looking at the comments and test code here, looks like this just makes it so you can pass a path string to the File constructor if (and only if) you're in chrome code?
Comment 26 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-04-14 06:57:23 PDT
Correct.
Comment 27 Eric Shepherd [:sheppy] 2011-04-14 10:34:23 PDT
Documented:

https://developer.mozilla.org/en/Extensions/Using_the_DOM_File_API_in_chrome_code

Also added cross-links and notes to:

https://developer.mozilla.org/en/Using_files_from_web_applications
https://developer.mozilla.org/en/DOM/File

And a mention to Firefox 4 developers.
Comment 28 Nickolay_Ponomarev 2011-04-14 10:51:25 PDT
Sheppy, this is Firefox 6, not 5 as far as I can see.
Comment 29 Eric Shepherd [:sheppy] 2011-04-14 10:52:08 PDT
Oops, I misread the check-in date; I'll fix it.
Comment 30 Eric Shepherd [:sheppy] 2011-04-14 10:58:07 PDT
OK, version note on this page fixed:

https://developer.mozilla.org/en/Extensions/Using_the_DOM_File_API_in_chrome_code

And I've created Firefox 6 for developers to list this there:

https://developer.mozilla.org/en/Firefox_6_for_developers
Comment 31 Jan Gerber 2011-05-12 06:28:34 PDT
so how can this be used from xpcom components now? Trying with Firefox nightly, i get this:

var file = new File(path);

Error: uncaught exception: [Exception... "'[JavaScript Error: "File is not defined" {file: "..." line: ..}]' when calling method: [nsIFactory::createInstance]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"
Comment 32 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-12 06:32:50 PDT
(In reply to comment #31)
> so how can this be used from xpcom components now? Trying with Firefox
> nightly, i get this:
> 
> var file = new File(path);
> 
> Error: uncaught exception: [Exception... "'[JavaScript Error: "File is not
> defined" {file: "..." line: ..}]' when calling method:
> [nsIFactory::createInstance]"  nsresult: "0x80570021
> (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"

You have to be inside a Window.  This isn't defined for components.
Comment 33 Jan Gerber 2011-05-12 06:37:45 PDT
in that case marking bug #647653 a duplicate of this one was wrong, i need this in a component.

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