656647 - File constructor should take nsIFile

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Please declare rv closer to its use, and note that you shadow it in the string case.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 531960 [details] [diff] [review]
Patch

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

::: content/base/src/nsDOMFile.cpp
@@ +682,5 @@
>  
> +    // If it's not a JSObject, give up
> +    JSObject* obj = JSVAL_TO_OBJECT(aArgv[0]);
> +    if (!obj)
> +      return NS_ERROR_UNEXPECTED;

This doesn't actually work. If you hand JS_TO_OBJECT something that isn't an object it'll assert and perform an invalid cast.

But I believe the JSVAL_IS_PRIMITIVE is enough to ensure that you have an object. So just remove the if-check.

@@ +701,5 @@
> +      return NS_ERROR_XPC_BAD_CONVERT_JS;
> +    }
> +
> +    nsCOMPtr<nsILocalFile> localFile;
> +    nsresult rv = NS_NewLocalFile(xpcomStr,

Remove the 'nsresult' here, as Ms2ger pointed out.

@@ +706,5 @@
> +                                  PR_FALSE, getter_AddRefs(localFile));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    file = do_QueryInterface(localFile, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);

Generally speaking, don't use nsresults together with do_QIs, simply nullcheck the result instead.

Though in this case an assert should be enough.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I changed the JSVAL_IS_PRIMITIVE guard to JSVAL_IS_OBJECT.

Other comments addressed.

http://hg.mozilla.org/mozilla-central/rev/190b0bff3507
http://hg.mozilla.org/mozilla-central/rev/0b84815ad48e

Comment 5

[Eric Shepherd [:sheppy]]

Updated documentation:

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

And linked to from Firefox 6 for developers.