399031 - Convert xpfe prefpane to toolkit version

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Shawn Wilsher :sdwilsh]

Spun-off from Bug 322245 Comment 8.

Whoever does this needs to make sure they use accesskeys for things!

Comment 1

[Robert Kaiser]

This depends on the first step of bug 394522, which is creation of the main new toolkit prefwindow itself for SeaMonkey. We have a WIP patch there and even a first pre-review, this should be available very soon. Once that part has landed, this panel can be reworked, with the accesskeys from bug 322245 integrated, of course.

Comment 2

[Bruno 'Aqualon' Escherl]

Attachment: Port the DOMI prefpane v1

First thanks KaiRo for the ground work. What did I have to do to make it work:

- outsource the JS code to its own file
- load the scripts in the script attribute of prefpane
- call enableBlinkPrefs() in onpaneload, calling it in Startup() is too early, because the preferences aren't initalized at that stage
- I also removed the flex="1" from both groupboxes to prevent the boxes from using all vertical space

Note: the patch from bug 421832 is needed for the DOMI prefpane to appear in the preferences window.

Comment 3

[Bruno 'Aqualon' Escherl]

Comment on attachment 309287 [details] [diff] [review]
Port the DOMI prefpane v1

oops, forgot to include the access keys, so removing review request. I'll post a new patch tomorrow.

Comment 4

[Bruno 'Aqualon' Escherl]

Attachment: Port the DOMI prefpane v2

Now with all accesskeys from the patch in bug 322245.

The onpaneload call was wrong, so I removed it.
The install button is working now, I had to insert a Startup() function into pref-sidebar.js that calls the initialization.

The DOMI sidebar isn't working, but I think that's an issue for another bug (if there isn't one already, didn't look).

Comment 5

[[e-mail]]

Comment on attachment 309401 [details] [diff] [review]
Port the DOMI prefpane v2

>@@ -0,0 +1,16 @@
No licence block?

>+function enableBlinkPrefs(aTruth)
>+{
>+  var els = [
>+    "lbElBorderColor", "cprElBorderColor",
>+    "lbElBorderWidth", "txfElBorderWidth",
>+    "lbElDuration", "txfElDuration",
>+    "lbElSpeed", "txfElSpeed",
>+    "cbElInvert"];
>+
>+  for (var i = 0; i < els.length; ++i) {
>+    if (aTruth)
>+      document.getElementById(els[i]).removeAttribute("disabled");
>+    else
>+      document.getElementById(els[i]).setAttribute("disabled", true);
>+  }
>+}
I guess this was a problem with the old code, rather than a problem with your patch, but a) it doesn't disable when you open the window b) it enables locked items.

>+        <checkbox id="cbElOn" label="&blinkSelectedElement.label;"
>+                  oncommand="enableBlinkPrefs(this.checked)"
This doesn't refresh properly if you change the pref from about:config while the preferences window is open; IIRC to fix that put <preference id="inspector.blink.on" ... onchange="enableBlinkPrefs(this.value);"/>

Comment 6

[Shawn Wilsher :sdwilsh]

Just a comment based on Neil's review comments (I hope to get to your review within a week):
s/var/let/

There's actually a performance benefit from using let over var, so for new code I'd like to see the use of let.

Comment 7

[Bruno 'Aqualon' Escherl]

Attachment: Port the DOMI prefpane v3

Next version of the patch, includes fixes for Neil's comments and contains so much `let´ even Scheme would be proud of ;)

Comment 8

[Ed Lee :Mardak]

Comment on attachment 309701 [details] [diff] [review]
Port the DOMI prefpane v3

>+++ extensions/inspector/resources/content/prefs/pref-inspector.js	
>+function enableBlinkPrefs(aTruth)
>+{
>+  let els = [
>+    {
>+      "lbElBorderColor": "cprElBorderColor"
>+    },
>+    {
>+      "lbElBorderWidth": "txfElBorderWidth"
>+    },
..
>+  ];
>+
>+  for (let [, entry] in Iterator(els))
>+    for (let [label, control] in Iterator(entry))
Do you think that in the future, you'll want multiple pairs of label/control for each entry? If not, you could get the same effect by doing this:

let els = {
  lbElBorderColor: "cprElBorderColor",
  lbElBorderWidth: "txtElBorderWidth",
..
};
for (let [label, control] in Iterator(entry)) ...

(FYI, you could have used "for each (let entry in els)")

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 309701 [details] [diff] [review]
Port the DOMI prefpane v3

>Index: extensions/inspector/resources/content/prefs/pref-inspector.js
>+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
can we get the vim modeline here too?  some files in storage have the right example

>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
Really?

>+  let elts = {
A comment explaining this would be really nice

>+  for (let [, entry] in Iterator(els))
why not |for each (let entry in els)|?
nit: also, everywhere else in inspector we have the brace on the same line as the for statement.  I'm rather in favor of keeping that.

>+      var controlElem = document.getElementById(control);
s/var/let/

>+      if (aTruth && !isPrefLocked(controlElem))
>+      {
nit: same thing with bracing here

>+        if(label)
nit: |if (label)|

>+      }
>+      else
>+      {
nit: like browser and toolkit style rules please
}
else {

>+  if(!elem.hasAttribute("preference"))
nit: space after if

>Index: extensions/inspector/resources/content/prefs/pref-inspector.xul
>+<overlay xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>+  <prefpane id="inspector_pane"
nit: I'm much more of a fan of hyphens instead of underscores in id's

>+    <preferences id="inspector_preferences">
ditto

>+      <!-- preference id="inspector.hooks.navigator"
>+                  name="inspector.hooks.navigator"
>+                  type="bool"/ -->
why is this commented out?

Also not sure if we need the RDFU.js file for this - but not a big deal either way.

r=sdwilsh with comments addressed

Comment 10

[Bruno 'Aqualon' Escherl]

(In reply to comment #9)
> (From update of attachment 309701 [details] [diff] [review])
> >Index: extensions/inspector/resources/content/prefs/pref-inspector.xul
> >+  <prefpane id="inspector_pane"
> nit: I'm much more of a fan of hyphens instead of underscores in id's
In SeaMonkey, we prefer the underscore for the pref ids. KaiRo wanted to talk about it on IRC.

> >+      <!-- preference id="inspector.hooks.navigator"
> >+                  name="inspector.hooks.navigator"
> >+                  type="bool"/ -->
> why is this commented out?
That's the case since end of 2001. I'm not sure if the feature ever worked.

> Also not sure if we need the RDFU.js file for this - but not a big deal either
> way.
That's needed in pref-sidebar.js 

I'll post a patch with all other changes soon, that's ready for checkin (subject to the clarification of the hyphen vs. underscore issue).

Comment 11

[Bruno 'Aqualon' Escherl]

Attachment: Port the DOMI prefpane v4

Comment 12

[Shawn Wilsher :sdwilsh]

I don't care enough about the hyphen/underscore issue since this is a seamonkey only file.

Comment 13

[Shawn Wilsher :sdwilsh]

I probably won't be able to get to the review on this until after April 15.  Let me know if you need it sooner and I'll try to squeeze it in.

Comment 14

[Bruno 'Aqualon' Escherl]

No problem, bug 421832 has to be fixed anyway before that patch is useful.

Comment 15

[Shawn Wilsher :sdwilsh]

Comment on attachment 310362 [details] [diff] [review]
Port the DOMI prefpane v4

Sorry this took so long.  r=sdwilsh, but I'd like Neil to take one more look.

Comment 16

[Bruno 'Aqualon' Escherl]

Attachment: Port the DOMI prefpane v5

Taking over r+ from sdwilsh.

Patch uses Startup() function instead of onpaneload.

Note: it only works, when the pref-inspector.js is declared as last file in the script attribute of the prefpane. In any other case Startup() wasn't called when opening the prefwindow.

Comment 17

[[e-mail]]

Comment on attachment 324972 [details] [diff] [review]
Port the DOMI prefpane v5

>+function Startup()
>+{
>+  enableBlinkPrefs(document.getElementById("inspector.blink.on").value);
>+}
...
>+function Startup()
>+{
>+  SidebarPrefs_initialize();
>+}
This won't work, you can only have one Startup()! Just put both lines in it.

>+            script="chrome://inspector/content/prefs/pref-sidebar.js
>+                    chrome://inspector/content/extensions/wsm-colorpicker.js
>+                    chrome://inspector/content/jsutil/xpcom/XPCU.js
>+                    chrome://inspector/content/jsutil/rdf/RDFU.js
>+                    chrome://inspector/content/prefs/pref-inspector.js">
You can put these back into the original order now :-)

>+      <preference id="inspector.blink.on"
>+                  name="inspector.blink.on"
>+                  type="bool"
>+                  onchange="enableBlinkPrefs(this.value);"/>
...
>+        <checkbox id="cbElOn" label="&blinkSelectedElement.label;"
>+                  oncommand="enableBlinkPrefs(this.checked)"
>+                  preference="inspector.blink.on"
>+                  accesskey="&blinkSelectedElement.accesskey;"/>
You don't need this oncommand any more.

Comment 18

[Bruno 'Aqualon' Escherl]

Attachment: Port the DOMI prefpane v6

Having two Startup() functions doesn't work, so I moved the functionality from the Startup() in pref-sidebar.js into pref-inspector.js' Startup().

Comment 19

[Bruno 'Aqualon' Escherl]

Comment on attachment 324985 [details] [diff] [review]
Port the DOMI prefpane v6

I overlooked that Neil already gave sr+ and since I addressed his comments from IRC, the patch is ready for checkin.

Comment 20

[[e-mail]]

> >+            script="chrome://inspector/content/prefs/pref-sidebar.js
> >+                    chrome://inspector/content/extensions/wsm-colorpicker.js
> >+                    chrome://inspector/content/jsutil/xpcom/XPCU.js
> >+                    chrome://inspector/content/jsutil/rdf/RDFU.js
> >+                    chrome://inspector/content/prefs/pref-inspector.js">
> You can put these back into the original order now :-)
Apart from this one ;-)

Comment 21

[Bruno 'Aqualon' Escherl]

Attachment: Port the DOMI prefpane v7 (ready for checkin)

(In reply to comment #20)
> Apart from this one ;-)
yeah...

Comment 22

[Robert Kaiser]

checked into both mozilla-central and cvs trunk, leaving to Bruno to mark fixed.

Comment 23

[Ian Neal]

Comment on attachment 325086 [details] [diff] [review]
Port the DOMI prefpane v7 (ready for checkin)

>Index: extensions/inspector/resources/content/prefs/pref-inspector.xul
>===================================================================
>-              <label id="lbElBorderWidth" value="&borderWidth.label;" control="txfElBorderWidth"/>
>+              <label id="lbElBorderWidth" control="txfElBorderWidth"
>+                     value="&borderWidth.label;"
>+                     accesskey="&borderWidth.accesskey;"/>
>               <hbox align="center">
>                 <textbox id="txfElBorderWidth" style="width: 4em"
>-                  preftype="int" prefstring="inspector.blink.border-width" prefattribute="value"/>
>+                         preference="inspector.blink.border-width"/>
>                 <label value="&px.label;"/>
You might want to use aria-labelledby, MarcoZ could advise. Similarly for the other ones.

Comment 24

[Shawn Wilsher :sdwilsh]

(In reply to comment #23)
> You might want to use aria-labelledby, MarcoZ could advise. Similarly for the
> other ones.
New bug please.

Resolving this bug - and it made 2.0.1.