Last Comment Bug 656647 - File constructor should take nsIFile
: File constructor should take nsIFile
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla6
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-12 09:49 PDT by Kyle Huey [:khuey] (khuey@mozilla.com)
Modified: 2011-05-15 19:34 PDT (History)
2 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (3.69 KB, patch)
2011-05-12 09:50 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
jonas: review+
Details | Diff | Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-12 09:49:53 PDT

    
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-12 09:50:47 PDT
Created attachment 531960 [details] [diff] [review]
Patch
Comment 2 :Ms2ger 2011-05-12 13:21:50 PDT
Please declare rv closer to its use, and note that you shadow it in the string case.
Comment 3 Jonas Sicking (:sicking) 2011-05-12 22:36:08 PDT
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 [:khuey] (khuey@mozilla.com) 2011-05-14 16:47:09 PDT
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] 2011-05-15 19:34:52 PDT
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.

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