305688 - please look before leaping for filepicker directory

Status: NEW

(SeaMonkey :: UI Design, defect)

Description

[timeless]

Exception ``[Exception... "Component returned failure code: 0x8000ffff
(NS_ERROR_UNEXPECTED) [nsIPrefBranch.getComplexValue]" nsresult: "0x8000ffff
(NS_ERROR_UNEXPECTED)" location: "JS frame ::
chrome://communicator/content/contentAreaUtils.js :: poseFilePicker :: line 504"
data: no]'' thrown from function poseFilePicker(aFpP=Object:{8}) in
<chrome://communicator/content/contentAreaUtils.js> line 504.
Stopped for thrown exception.
#0: function poseFilePicker(aFpP=Object:{8}) in
<chrome://communicator/content/contentAreaUtils.js> line 504
502: // Try and pull in download directory pref
503: try {
504: dir = branch.getComplexValue(kDownloadDirPref, nsILocalFile);
505: } catch (e) { }
506:
Continuing from thrown exception.
Error ``dir has no properties'' [xs] in file
``chrome://communicator/content/contentAreaUtils.js'', line 522, character 0.
Stopped for error handler.
#0: function poseFilePicker(aFpP=Object:{8}) in
<chrome://communicator/content/contentAreaUtils.js> line 522
520:
521: try {
522: if (dir.exists())
523: fp.displayDirectory = dir;
524: } catch (e) { }

Comment 1

[timeless]

Attachment: look before leaping

Comment 2

[[e-mail]]

Comment on attachment 193623 [details] [diff] [review]
look before leaping

>   try {
>-    dir = branch.getComplexValue(kDownloadDirPref, nsILocalFile);
>+    if (branch.getPrefType(kDownloadDirPref) == branch.PREF_STRING))
>+      dir = branch.getComplexValue(kDownloadDirPref, nsILocalFile);
>   } catch (e) { }
I'm not convinced that this is a suitable use of the API e.g. if the pref used
to be set but was since reset either via about:config or by switching profile,
so I'm requesting a second opinion.

>   try {
>-    if (dir.exists())
>+    if (dir && dir.exists())
>       fp.displayDirectory = dir;
>   } catch (e) { }
I'd almost remove the try/catch at this point as the only thing that can stop
is now is OOM which I don't see the point of masking.

Comment 3

[Darin Fisher]

Comment on attachment 193623 [details] [diff] [review]
look before leaping

>   try {
>-    dir = branch.getComplexValue(kDownloadDirPref, nsILocalFile);
>+    if (branch.getPrefType(kDownloadDirPref) == branch.PREF_STRING))
>+      dir = branch.getComplexValue(kDownloadDirPref, nsILocalFile);
>   } catch (e) { }

Can you please explain why this part of the patch is needed?  It
seems like over-engineering to me since the call to getComplexValue
is in a try-catch block, but I'm probably missing something.

Comment 4

[Darin Fisher]

Comment on attachment 193623 [details] [diff] [review]
look before leaping

Marking r- to get attention.  Please see my previous comment.