Closed Bug 992185 Opened 10 years ago Closed 10 years ago

Download location preference field is empty on reload

Categories

(Firefox :: Settings UI, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 31

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

Attachments

(1 file, 3 obsolete files)

See bug 738796 comment 82:

> Under General -> Downloads -> “Save files to” option selected, I see the next issue: if I selected any other folder: “/home/your_name/Folder_Name” appears. After reloading the tab, nothing appears.

So by default, the download location field shows "Downloads". However, when you change the directory to anything other than Downloads, the field shows the full path (probably not desired). When the preferences page is reloaded, the field is just empty.
Alright, I was able to fix the full path thing.

However, the emptyness issue is strange. I tracked it down to the `value` of the <preference> element being null on page load. This is strange, since I do see a value for the pref about:config.

When I looked at about:config, the pref was being stored as a string instead of as a "file": http://i.stack.imgur.com/DnTnL.png (chaîne is French for "string"). This might have something to do with the null value.
Attached patch Patch 1 (obsolete) — Splinter Review
Figured it out.

The downloads field now loads correctly. It also no longer shows the cramped, full path, and only shows the folder name.
Attachment #8402128 - Flags: review?(jaws)
QA Contact: camelia.badau
Comment on attachment 8402128 [details] [diff] [review]
Patch 1

>diff --git a/browser/components/preferences/in-content/main.js b/browser/components/preferences/in-content/main.js

>   _getDisplayNameOfFile: function (aFolder)

>-    return aFolder ? aFolder.path : "";
>+    return aFolder ? aFolder.leafName : "";

Is this really wanted? Not sure that showing only the leafName makes sense, makes it hard to see what you picked. Regardless, should probably be done separately from your bugfix. 

>diff --git a/browser/components/preferences/in-content/main.xul b/browser/components/preferences/in-content/main.xul

>+    <preference id="browser.download.folderList"
>+                name="browser.download.folderList"
>+                type="int"/>
>     <preference id="browser.download.dir"
>                 name="browser.download.dir"
>                 type="file"
>                 onchange="gMainPane.displayDownloadDirPref();"/>
>-    <preference id="browser.download.folderList"
>-                name="browser.download.folderList"
>-                type="int"/>

Why does this fix the weirdness?
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #3)

> Is this really wanted? Not sure that showing only the leafName makes sense,
> makes it hard to see what you picked. Regardless, should probably be done
> separately from your bugfix. 

Well, this was mentioned in Camelia's comment on the main bug. But yeah, this can be filed separately.


> Why does this fix the weirdness?

I'm not completely sure. It works, though.

As far as I can tell, when the tags are in the opposite order, and the onsyncfrompreference for forderList here[1] is called, the folderList preference has been loaded, but the dir preference has not, and calling the value in the method doesn't work.


 [1]: http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/main.xul#173
Summary: Selecting a download location other than the default behaves strangely on Ubuntu → Download location preference field is empty on reload
Filed bug 992456 for the full path vs leaf name issue.
Attachment #8402128 - Attachment is obsolete: true
Attachment #8402128 - Flags: review?(jaws)
Attachment #8402140 - Flags: review?(jaws)
Attachment #8402140 - Flags: feedback?(gavin.sharp)
As I understand things, the preferences should both be initialized before any onsyncfrompreference handlers are called. Is that not happening?
As far as I can tell, it's not happening. Put a debugger; at the top of `displayDownloadDirPref()` in main.js and have a look at `document.getElementById('browser.download.dir').value` both with and without the patch. The <preference> element has a null value when the patch isn't applied. I'm not sure if that's by design -- it *shouldn't* be, but I'm not familiar with the code behind preferences. We can file a separate bug about this if necessary.
OS: Linux → All
(In reply to Manish Goregaokar [:manishearth] from comment #8)
> As far as I can tell, it's not happening. Put a debugger; at the top of
> `displayDownloadDirPref()` in main.js and have a look at
> `document.getElementById('browser.download.dir').value`

What does the call stack to displayDownloadDirPref look like when the error case occurs? I.e. which of the two displayDownloadDirPref calls in mail.xul are the problem, onchange or onsyncfrompreference?
It seems to be onsyncfrompreference

Call stack:

gMainPane.displayDownloadDirPref
anonymous (preferences.xml:1)
setElementValue (preferences.xml:403)
updateElements (preferences.xml:524)
_setValue (preferences.xml:168)
preference_XBL_constructor (preferences.xml:118)

setElementValue contains:

> var event = document.createEvent("Events");
> event.initEvent("syncfrompreference", true, true);
> var f = new Function ("event", aElement.getAttribute("onsyncfrompreference"));
> rv = f.call(aElement, event);

near line 403. 

Also, the contents of updateElements:

> var elements = document.getElementsByAttribute("preference", this.id);
> for (var i = 0; i < elements.length; ++i) 
>  this.setElementValue(elements[i]);


So basically, the moment a <preference> object is constructed, all form controls on the page linked with that pref get instantiated too (either by copying the preference over, or by firing onsyncfrompreference). In this case, the latter is happening.


I guess this means that referring to preferences by id in an onsyncfrompreferences handler is not good, we should just load them via Services.
Cool, thanks for digging deeper - that makes sense. Do you want to adjust the patch to avoid using the pref element, then, and maybe add a clarifying comment?
(I'm surprised we don't run into this issue more often. Maybe we've just been lucky?)
I'm surprised we haven't run into this either, since the trick of referencing a <preference> element is used in almost all the onsyncfrompreferences, and it's rather convenient over using get*Pref.

I plan on adding a note to the MDN page that explicitly mentions this behavior of onsyncfrompreference.

I'm not sure if we should fix this by replacing it with a direct call to getComplexValue(), since the rest of the code seems to rely on precise ordering of <preference> elements anyway. I'll upload a patch for this case, though.

I think we should just swap the order for now, and then try and investigate ways of fixing onsyncfrompreference's strangeness in a separate bug. I haven't thought much about it yet, but there doesn't seem to be a straightforward alternate behavior for it, so this might require some discussion.
Here's the alternate patch, choose the one you like more :)
Attachment #8403516 - Flags: review?(gavin.sharp)
Whoops, forgot to qref.
Attachment #8403516 - Attachment is obsolete: true
Attachment #8403516 - Flags: review?(gavin.sharp)
Attachment #8403519 - Flags: review?(gavin.sharp)
Comment on attachment 8402140 [details] [diff] [review]
Patch 2 - split into two bugs

(clearing review request since Gavin has been working closer with you on this)
Attachment #8402140 - Flags: review?(jaws)
Comment on attachment 8402140 [details] [diff] [review]
Patch 2 - split into two bugs

I changed my mind, let's use this for now. This seems like a huge footgun but I'm not sure how to solve it offhand. Perhaps file a bug about this general problem (to audit for other such situations, find a global solution, etc.)?
Attachment #8402140 - Flags: feedback?(gavin.sharp) → review+
Attachment #8403519 - Flags: review?(gavin.sharp)
Alright, thanks.

I'll file a detailed bug in a while. A fix might require some radical changes to the internals, though.
Keywords: checkin-needed
Attachment #8403519 - Attachment is obsolete: true
See Also: → 997570
https://hg.mozilla.org/mozilla-central/rev/0820dbc1ab42
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.7.5 using Nightly 31.0a1 (buildID: 20140428030203).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.