Last Comment Bug 451025 - <preference> element needs to be usable even outside of a <prefpane> element
: <preference> element needs to be usable even outside of a <prefpane> element
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Preferences (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Daniel Brooks [:db48x]
:
: Neil Deakin
Mentors:
: 293439 (view as bug list)
Depends on: 541959
Blocks: 436077
  Show dependency treegraph
 
Reported: 2008-08-18 03:33 PDT by Daniel Brooks [:db48x]
Modified: 2011-09-28 12:28 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
451025-1.diff (1.02 KB, patch)
2008-08-24 05:12 PDT, Daniel Brooks [:db48x]
no flags Details | Diff | Splinter Review
451025-2.diff (1.64 KB, patch)
2008-08-24 05:19 PDT, Daniel Brooks [:db48x]
no flags Details | Diff | Splinter Review
451025-3w.diff (6.27 KB, patch)
2008-08-26 11:11 PDT, Daniel Brooks [:db48x]
no flags Details | Diff | Splinter Review
451025-4w.diff (6.00 KB, patch)
2008-08-29 13:01 PDT, Daniel Brooks [:db48x]
no flags Details | Diff | Splinter Review
451025-2.1.diff (1.69 KB, patch)
2008-10-07 23:32 PDT, Daniel Brooks [:db48x]
gavin.sharp: review+
Details | Diff | Splinter Review
451025-2.2.diff (1.71 KB, patch)
2008-10-09 13:57 PDT, Daniel Brooks [:db48x]
no flags Details | Diff | Splinter Review
451025-2.3.diff (1.71 KB, patch)
2008-10-09 14:38 PDT, Daniel Brooks [:db48x]
no flags Details | Diff | Splinter Review
451025-2.4.diff (2.06 KB, patch)
2008-10-09 14:43 PDT, Daniel Brooks [:db48x]
gavin.sharp: review+
Details | Diff | Splinter Review

Description Daniel Brooks [:db48x] 2008-08-18 03:33:41 PDT
There might already be a bug on this, but I can't find it.

This is a regression from when the <preference> element was first introduced quite a while back (for FF1.5, as I recall). Not all apps that use preferences will include a separate dialog box with <prefpane>s in it as a user interface for modifying those prefs. Fennec is one example, and an app I wrote while with MDG is another.
Comment 1 Daniel Brooks [:db48x] 2008-08-24 05:12:09 PDT
Created attachment 335257 [details] [diff] [review]
451025-1.diff
Comment 2 Daniel Brooks [:db48x] 2008-08-24 05:19:27 PDT
Created attachment 335258 [details] [diff] [review]
451025-2.diff

forgot to update the patch before attaching it
Comment 3 Daniel Brooks [:db48x] 2008-08-26 10:09:46 PDT
Comment on attachment 335258 [details] [diff] [review]
451025-2.diff

patch is incomplete
Comment 4 Daniel Brooks [:db48x] 2008-08-26 11:11:22 PDT
Created attachment 335557 [details] [diff] [review]
451025-3w.diff
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-08-26 12:51:16 PDT
Would be nice to get some tests for this (see e.g. http://mxr.mozilla.org/seamonkey/source/toolkit/content/tests/chrome/test_preferences.xul ).
Comment 6 Daniel Brooks [:db48x] 2008-08-26 16:36:55 PDT
good point, I'll see what I can come up with
Comment 7 Neil Deakin 2008-08-29 10:21:32 PDT
Comment on attachment 335557 [details] [diff] [review]
451025-3w.diff

Don't remove properties that are defined for elements and are listed in the documentation.

Why do we need a separate new element here? Can't we just put all of the functionality in the <preferences> element directly?
Comment 8 Daniel Brooks [:db48x] 2008-08-29 13:01:42 PDT
Created attachment 336099 [details] [diff] [review]
451025-4w.diff
Comment 9 Daniel Brooks [:db48x] 2008-08-29 13:04:09 PDT
(In reply to comment #7)
> (From update of attachment 335557 [details] [diff] [review])
> Don't remove properties that are defined for elements and are listed in the
> documentation.

Done.

> Why do we need a separate new element here? Can't we just put all of the
> functionality in the <preferences> element directly?

Because it needs to listen for change events from the input controls (textboxes, checkboxes, etc) which means it needs to be an ancestor of those controls. Merging the prefmanager tag with the preferences tag would that everyone using these features would need to update their code, moving all of their input controls underneath their preferences tags.
Comment 10 Neil Deakin 2008-09-01 13:53:31 PDT
(In reply to comment #9)
> Because it needs to listen for change events from the input controls
> (textboxes, checkboxes, etc) which means it needs to be an ancestor of those
> controls.

Why can't it just use an event listener?
Comment 11 Daniel Brooks [:db48x] 2008-09-01 16:46:56 PDT
xbl destructors are never called, so it would never get a chance to remove the listener.
Comment 12 Neil Deakin 2008-09-09 08:15:33 PDT
With a few minor errors fixed, the following already works just fine:

<prefpane>
  <preferences>
    <preference id="browser.startup.homepage"
                name="browser.startup.homepage" type="wstring"/>
  </preferences>
  <textbox id="textbox" preference="browser.startup.homepage"/>
</prefpane>

It seems like we can just use <prefpane> here rather than using a new element.
Comment 13 Daniel Brooks [:db48x] 2008-09-09 15:41:27 PDT
No, I can't. The prefpane binding inserts a bunch of UI stuff that are specific to one particular app's preferences window in addition to handling the preferences attribtue. By splitting the prefpane into two elements, (called "prefpane" and "prefmanager") you can get the latter without having to swallow the former.
Comment 14 Neil Deakin 2008-09-09 17:31:10 PDT
I'm not following. I can understand the prefwindow binding, but I don't see anything of note in the prefpane binding. There are a couple of properties and methods that might not be used, but I don't see an issue there.
Comment 15 Daniel Brooks [:db48x] 2008-10-07 22:58:45 PDT
erm. on looking at the file again, I see that you're right. I need to reexamine the symptoms to see exactly how to fix the prefpane binding. In the mean time, however, could I get a review on the change to the preference element? We need that at least for Fennec A1. That's just the second patch.
Comment 16 Daniel Brooks [:db48x] 2008-10-07 23:32:49 PDT
Created attachment 342205 [details] [diff] [review]
451025-2.1.diff

fix a good review commend from Gavin.
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-10-07 23:48:38 PDT
Comment on attachment 342205 [details] [diff] [review]
451025-2.1.diff

>diff -r b222f6e972b3 toolkit/content/widgets/preferences.xml

>+      <property name="type">

>+            return "type" in doc && doc.type

missing semi-colon, and should probably return doc.type || ""; to match the default prefwindow behavior (missing attribute).

>+      <property name="instantApply">

>+            return ("instantApply" in doc && doc.instantApply) || rootBranch.getBoolPref("browser.preferences.instantApply")

I prefer explicit references to |this| (also missing a semi-colon). Shouldn't need the "in" check anymore either.

Should mark these readonly I guess (even though it doesn't seem to affect settability without a setter defined).

r=me with those.
Comment 18 Daniel Brooks [:db48x] 2008-10-09 13:57:42 PDT
Created attachment 342492 [details] [diff] [review]
451025-2.2.diff

patch to be checked in
Comment 19 Daniel Brooks [:db48x] 2008-10-09 14:38:50 PDT
Created attachment 342504 [details] [diff] [review]
451025-2.3.diff

forgot one nit in the last patch
Comment 20 Daniel Brooks [:db48x] 2008-10-09 14:43:24 PDT
Created attachment 342505 [details] [diff] [review]
451025-2.4.diff

/me sighs. perhaps this one will be acceptable?
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-10-09 14:50:48 PDT
Comment on attachment 342505 [details] [diff] [review]
451025-2.4.diff

this is perfectly acceptable, minus the changes to js/src/jsdtoa.cpp! :)
Comment 22 Daniel Brooks [:db48x] 2008-10-09 14:58:40 PDT
lol. I completely forgot to leave those out.
Comment 23 Neil Deakin 2009-06-23 10:55:24 PDT
This should be fixed, no?
Comment 24 Nickolay_Ponomarev 2011-09-28 12:25:18 PDT
*** Bug 293439 has been marked as a duplicate of this bug. ***
Comment 25 Nickolay_Ponomarev 2011-09-28 12:28:22 PDT
This landed on Thu Oct 09 16:16:10 2008 -0700:
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=3fe2918df533
http://hg.mozilla.org/mozilla-central/rev/3fe2918df533

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