530643 - Multipanels are bad; entab the XBL viewer

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Colby Russell :crussell (no longer Mozilla-ing)]

Multipanels are hard to use, even if you grasp the concept. The XBL viewer should use tabs instead. Bonus: the sections are now keyboard accessible.

Comment 1

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: Does away with multipanels; consistent code style for files I've touched

With my widgets and text size, this requires 334 pixels horizontally for the XBL viewer to fit all tabs, although I wouldn't imagine anyone using it at even that small a width, because you already need much more to read the code comfortably.

Comment 2

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: screenshot of tabbed XBL bindings viewer

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 414102 [details] [diff] [review]
Does away with multipanels; consistent code style for files I've touched

Want to see Neil's input on this before I review.  I like the idea though.

Comment 4

[Alex Vincent [:WeirdAl]]

Comment on attachment 414102 [details] [diff] [review]
Does away with multipanels; consistent code style for files I've touched

I see a lot of unnecessary and irrelevant-to-the-bug-at-hand changes in these patch files.

>-  initialize: function(aPane)
>+  initialize: function XBLBVr_Initialize(aPane)
...

>-  destroy: function()
>+  destroy: function XBLBVr_Destroy()
...

>-    this.mResourceTree.treeBoxObject.view = null;    
>+    this.mResourceTree.treeBoxObject.view = null;
...

>-  addObserver: function(aEvent, aObserver) { this.mObsMan.addObserver(aEvent, aObserver); },
>-  removeObserver: function(aEvent, aObserver) { this.mObsMan.removeObserver(aEvent, aObserver); },
>+  addObserver: function XBLBVr_AddObserver(aEvent, aObserver) {
>+    this.mObsMan.addObserver(aEvent, aObserver);
>+  },
>+
>+  removeObserver: function XBLBVr_RemoveObserver(aEvent, aObserver) {
>+    this.mObsMan.removeObserver(aEvent, aObserver);
>+  },
...

etc.

Please restrict your diffs to the absolute minimum needed, so that we can see what is really changing.

Comment 5

[Alex Vincent [:WeirdAl]]

(I like it too.  I like it a lot.)

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> Please restrict your diffs to the absolute minimum needed, so that we can see
> what is really changing.
But feel free to attach a patch that makes those changes before the actual code changes.  It will make it easier to review.

Comment 7

[[e-mail]]

Comment on attachment 414102 [details] [diff] [review]
Does away with multipanels; consistent code style for files I've touched

I did my best to wade through the boilerplate changes ;-)

Shouldn't all the multipanel goop (CSS, XBL) be removed?

>+  if (viewer) {
>+    viewer.initContent();
>+  }
>+  else {
>+    gInitContent = true;
>+  }
I can't see what's going on here.

>     window.setTimeout(function(me) {
>-      // prepare and attach the content DOM datasource
>-      me.mContentView = XPCU.createInstance(kDOMViewCID, "inIDOMView");
>-      me.mContentView.whatToShow &= ~(NodeFilter.SHOW_TEXT);
>-      me.mContentTree.treeBoxObject.view = me.mContentView;
>+        // prepare and attach the content DOM datasource
>+        me.mContentView = XPCU.createInstance(kDOMViewCID, "inIDOMView");
>+        me.mContentView.whatToShow &= ~(NodeFilter.SHOW_TEXT);
>+        me.mContentTree.treeBoxObject.view = me.mContentView;
> 
>-      me.mContentInit = true;
>-      if (me.mBinding)
>-        me.displayContent();
>-    }, 10, this);
>+        me.mContentInit = true;
>+        if (me.mBinding)
>+          me.displayContent();
>+      }, 10, this);
Not sure what the point of this was.

>+    var tabs = document.getElementById("bxBindingAspects");
Nit: this confused me, because it's the <tabbox>, not the <tabs> element.

>+    if (tabs.selectedTab.disabled) {
>+      var tabEls = document.getElementsByTagName("tab");
Can't you use tabbox.tabs.childNodes? Or are we allowed to use advanceSelectedTab? (Or better still _selectNewTab, but that looks private.)

>+    var ml = document.getElementById("mlBindings");
>     if (!this.mBinding) {
>       ml.setAttribute("disabled", "true");
>-      mps.setAttribute("collapsed", "true");
>     } else {
>       ml.removeAttribute("disabled");
>-      mps.removeAttribute("collapsed");
>     }
Doesn't this now simplify into a simple property assignment?

>+      document.getElementById("tbContent").disabled = (list.length == 0);
>+      this.mContentTree.disabled = (list.length == 0);
Nit: don't need ()s around the condition.

>-    var et = getradio.hasAttribute("selected") ? "get" : setradio.hasAttribute("selected") ? "set" : null;
>+    var et;
>+    if (getradio.hasAttribute("selected"))
>+      et = "get";
>+    else if (setradio.hasAttribute("selected"))
>+      et = "set";
>+    else
>+      et = null;
Not sure that this change helps.

>-      text = aHandler.hasAttribute("action") ? aHandler.getAttribute("action") : null;
>       if (!text)
>         text = this.readDOMText(aHandler);
[Does this look any clearer?
      text = aHandler.getAttribute("action") || this.readDOMText(aHandler);
]

>+    var text = aEl.nodeValue ? aEl.nodeValue : "";
[aEl.nodeValue || ""] (This change looks familiar somehow...)

>-  getBoxObject: function(aTree)
>+  getBoxObject: function XBLBVr_GetBoxObject(aTree)
>   {
>-    return aTree.boxObject.QueryInterface(Components.interfaces.nsITreeBoxObject);
>+    return aTree.boxObject.QueryInterface(
>+             Components.interfaces.nsITreeBoxObject
>+           );
Ironically destroy() just uses .treeBoxObject directly...

>+      <tabpanel id="tpContent" flex="1">
>+        <tree id="olContent" seltype="single" treelines="true" flex="1">
I'm not a fan of useless "tabpanel" elements. Just write
<tree id="olContent" seltype="single" treelines="true">

>+                class="tree-list"
This class no longer (since before Mozilla 1.0) has any effect, and if it was still wanted it would have been replaced with the hideheader and hidecolumpicker attributes. It would be nice if we could ditch the useless CSS rules too.

Comment 8

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: code style cleanup: whitespace, consistent bracing, name functions, use scriptable properties instead of attributes

(In reply to comment #4)
> (From update of attachment 414102 [details] [diff] [review])
> I see a lot of unnecessary and irrelevant-to-the-bug-at-hand changes in these
> patch files.

Right. This includes, e.g., eliminating existing spaces before newlines in touched files, which I was asked to do previously.

Comment 9

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: address last review

(In reply to comment #7)
> (From update of attachment 414102 [details] [diff] [review])
> I did my best to wade through the boilerplate changes ;-)

Can I ask that the previous patch be frozen and any issues with it be included in with further patches, since that one really contains the bulk of the general code style changes? It will be more manageable than juggling two patches touching the same files.

> >+  if (viewer) {
> >+    viewer.initContent();
> >+  }
> >+  else {
> >+    gInitContent = true;
> >+  }
> I can't see what's going on here.

I couldn't either. That was the result of the removal of the following lines:

> -  <multipanelset id="mpsBinding" flex="1">
> -    <multipanel id="bxContent" label="&bindingContent.label;" open="true"
> -                persist="open"
> -                onopen="if (viewer) {viewer.initContent();} else {gInitContent = true;}">
> -      <tree id="olContent" treelines="true" flex="1" class="plain">

It was supposed to be some asynchronous callback to initialize the content, and I really wanted to rip it out and do it in a simpler procedural way, but I couldn't make out the code paths or what it was doing. I was trying not to break it. But it's all gone now.

> >+    if (tabs.selectedTab.disabled) {
> >+      var tabEls = document.getElementsByTagName("tab");
> Can't you use tabbox.tabs.childNodes? Or are we allowed to use
> advanceSelectedTab? (Or better still _selectNewTab, but that looks private.)

advanceSelectedTab steals keyboard focus.

> >-    var et = getradio.hasAttribute("selected") ? "get" : setradio.hasAttribute("selected") ? "set" : null;
> >+    var et;
> >+    if (getradio.hasAttribute("selected"))
> >+      et = "get";
> >+    else if (setradio.hasAttribute("selected"))
> >+      et = "set";
> >+    else
> >+      et = null;
> Not sure that this change helps.

Well, I was really just trying to fit it into 80 columns. But still, nested ternary conditionals?

> >+                class="tree-list"
> This class no longer (since before Mozilla 1.0) has any effect, and if it was
> still wanted it would have been replaced with the hideheader and
> hidecolumpicker attributes. It would be nice if we could ditch the useless CSS
> rules too.

Which ones? The multipanel stuff?

Comment 10

[[e-mail]]

[Note: I haven't actually looked at the new patch yet.]

(In reply to comment #9)
> advanceSelectedTab steals keyboard focus.
Oh, that's annoying.

> > >-    var et = getradio.hasAttribute("selected") ? "get" : setradio.hasAttribute("selected") ? "set" : null;
> > >+    var et;
> > >+    if (getradio.hasAttribute("selected"))
> > >+      et = "get";
> > >+    else if (setradio.hasAttribute("selected"))
> > >+      et = "set";
> > >+    else
> > >+      et = null;
> > Not sure that this change helps.
> Well, I was really just trying to fit it into 80 columns. But still, nested
> ternary conditionals?
What might work (but probably needs more thought than is reasonable at this time of night) is to set value attributes on the raPropG/Setter elements; then as they are chosen they will update the value of the radiogroup appropriately.

> > >+                class="tree-list"
> > This class no longer (since before Mozilla 1.0) has any effect, and if it was
> > still wanted it would have been replaced with the hideheader and
> > hidecolumpicker attributes. It would be nice if we could ditch the useless CSS
> > rules too.
> Which ones? The multipanel stuff?
I'd already mentioned those before; I was talking about the .tree-list stuff.

Comment 11

[[e-mail]]

Comment on attachment 416646 [details] [diff] [review]
code style cleanup: whitespace, consistent bracing, name functions, use scriptable properties instead of attributes

>-  get uid() { return "xblBindings" },
>-  get pane() { return this.mPane },
>+  get uid()
>+  {
>+    return "xblBindings"
>+  },
> 
>-  get subject() { return this.mSubject },
>-  set subject(aObject) 
>+  get pane()
>+  {
>+    return this.mPane
>+  },
>+
>+  get subject()
>+  {
>+    return this.mSubject
>+  },
>+
>+  set subject(aObject)
I'm not too fussed about making these changes, except that it does give you a chance to restore the missing semicolons to the return statements ;-)

>+      document.getElementById("bxContent").disabled = list.length == 0;
I couldn't see a disabled property on a multipanel element.

>+    if (!setradio.hasAttribute("selected") &&
>+        !getradio.hasAttribute("selected")) {
On the other hand radiobuttons do have a selected property, although it's only readonly because you're supposed to use radiogroup methods to set it.

>-  getBoxObject: function(aTree)
>+  getBoxObject: function XBLBVr_GetBoxObject(aTree)
>   {
>-    return aTree.boxObject.QueryInterface(Components.interfaces.nsITreeBoxObject);
>+    return aTree.boxObject.QueryInterface(
>+             Components.interfaces.nsITreeBoxObject
>+           );
>   }
This whole method is a pointless longhand for aTree.treeBoxObject anyway...

>     <multipanel id="bxContent" label="&bindingContent.label;" open="true" persist="open" onopen="if (viewer) {viewer.initContent();} else {gInitContent = true;}">
Strange that the next patch shows this as having been rewrapped ;-)

Comment 12

[[e-mail]]

Comment on attachment 416647 [details] [diff] [review]
address last review

>-  if (gInitContent)
>-    this.initContent();
>+  // prepare and attach the content DOM datasource
>+  this.mContentView = XPCU.createInstance(kDOMViewCID, "inIDOMView");
>+  this.mContentView.whatToShow &= ~(NodeFilter.SHOW_TEXT);
>+  this.mContentTree.treeBoxObject.view = this.mContentView;
[I wonder why the previous code did this on a timeout.]

>+    var list = this.mBinding ?
>+      this.mBinding.getElementsByTagName("content") : null;
>+    this.mContentView.rootNode = list && list.length ? list[0]: null;
Nit: space before : on last line.
Although I wonder whether this could be written
this.mContentView.rootNode =
  this.mBinding && this.mBinding.getElementsByTagName("content").item(0);
[.item(0) is a strict-JS-safe version of [0] for node lists.]

>+    this.mContentTree.disabled = !this.mContentView.rootNode;
>+    document.getElementById("tbContent").disabled =
>+      !this.mContentView.rootNode;
If we're disabling the tab, should we bother disabling the tree?

>     var body = aMethod.getElementsByTagNameNS(kXBLNSURI, "body")[0];
[Interestingly that one uses the NS version. I wonder why the others don't.]

>+    <tabpanels flex="1">
>+      <tree id="olContent" seltype="single" treelines="true" flex="1">
Nit: child nodes of a <tabpanels> don't need flex="1"

>+        <splitter orient="vertical" collapse="after">
>+          <grippy/>
>+        </splitter>
>+        <groupbox flex="1" persist="height">
Did you want to persist the collapsed state too? (I can't remember whether you need to persist the collapsed attribute on the groupbox or the state attribute on the splitter.)

>-        <treechildren id="olbResources"
>-                      alternatingbackground="true"
>-        />
>+        <treechildren id="olbResources"/>
Why remove the alternating background?

Comment 13

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #11)
> >-  getBoxObject: function(aTree)
> >+  getBoxObject: function XBLBVr_GetBoxObject(aTree)
> >   {
> >-    return aTree.boxObject.QueryInterface(Components.interfaces.nsITreeBoxObject);
> >+    return aTree.boxObject.QueryInterface(
> >+             Components.interfaces.nsITreeBoxObject
> >+           );
> >   }
> This whole method is a pointless longhand for aTree.treeBoxObject anyway...

Yeah, it's eliminated in the following patch, since this one was supposed to be code style changes, even to stuff that would be removed.

> >     <multipanel id="bxContent" label="&bindingContent.label;" open="true" persist="open" onopen="if (viewer) {viewer.initContent();} else {gInitContent = true;}">
> Strange that the next patch shows this as having been rewrapped ;-)

Yeah, weird. I'm pretty sure I was working from the state of the repo after the commit, but something went wrong. Maybe I uploaded an older patch. I'll look into it when I start work on this again to address the review.

(In reply to comment #12)
> (From update of attachment 416647 [details] [diff] [review])
...
> >-        <treechildren id="olbResources"
> >-                      alternatingbackground="true"
> >-        />
> >+        <treechildren id="olbResources"/>
> Why remove the alternating background?

Dunno. I thought at the time that this was a now-unsupported attribute, because certainly doesn't work for me. Should it stay?

Comment 14

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: cleanup mkIII

Comment 15

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: real changes mkIII

(In reply to comment #12)
> [I wonder why the previous code did this on a timeout.]
Presumably to avoid race conditions. But here's how the code paths will look now (there are two):
// called from load listener
XBLBindingsViewer_initialize
  XBLBindingsViewer
    // setup tree view
  XBLBVr_Initialize
    notifyViewerReady // inspector.xml
      XBLBindingsViewer set subject
        XBLBVr_DisplayBinding
          // add gDocLoadListener

gDocLoadListener
  XBLBVr_DoDisplayBinding
    XBLBVr_DisplayContent
      // twiddle mContentView

// occurs only when mlBindings's value is null
XBLBindingsViewer_initialize
  XBLBindingsViewer
    // setup tree view
  XBLBVr_Initialize
    notifyViewerReady // inspector.xml
      XBLBindingsViewer set subject
        XBLBVr_DisplayBinding
          XBLBVr_DoDisplayBinding
            XBLBVr_DisplayContent
              //twiddle mContentView


> If we're disabling the tab, should we bother disabling the tree?
In the most reasonable cases, no, but that assumes someone doesn't do something like load a binding that's effectively empty. Also, since up till now, the filter in the viewer registry for the XBL bindings viewer has been relatively dumb, it was possible for the viewer to refer to vanilla elements with no bindings. But that's moot now.

> >     var body = aMethod.getElementsByTagNameNS(kXBLNSURI, "body")[0];
> [Interestingly that one uses the NS version. I wonder why the others don't.]
They probably should, so I set out to fix it. But I ran into problems with getAttributeNS in the tree views (e.g., |handler.getAttributeNS(kXBLNSURI, "event")|), where it's returning an empty string, as if the attributes don't exist in the XBL namespace. Any idea about this?

Comment 16

[[e-mail]]

(In reply to comment #15)
> (In reply to comment #12)
> > >     var body = aMethod.getElementsByTagNameNS(kXBLNSURI, "body")[0];
> > [Interestingly that one uses the NS version. I wonder why the others don't.]
> They probably should, so I set out to fix it. But I ran into problems with
> getAttributeNS in the tree views (e.g., |handler.getAttributeNS(kXBLNSURI,
> "event")|), where it's returning an empty string, as if the attributes don't
> exist in the XBL namespace. Any idea about this?
Unlike elements, most attributes don't exist in the XBL namespace. (The only attribute that does exist in the XBL namespace is xbl:inherits.)

Comment 17

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #16)
> Unlike elements, most attributes don't exist in the XBL namespace. (The only
> attribute that does exist in the XBL namespace is xbl:inherits.)

Ah, this was contrary to my understanding of attributes and namespaces in general.

Comment 18

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: real changes mkIV

lose unnecessary locale DTD imports in xblBindings.xul; to be used with cleanup mkIII

Comment 19

[[e-mail]]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 416647 [details] [diff] [review])
> > >-        <treechildren id="olbResources"
> > >-                      alternatingbackground="true"
> > >-        />
> > >+        <treechildren id="olbResources"/>
> > Why remove the alternating background?
> Dunno. I thought at the time that this was a now-unsupported attribute, because
> certainly doesn't work for me. Should it stay?
It's only currently supported by Pinstripe and Modern.

Comment 20

[[e-mail]]

Comment on attachment 418443 [details] [diff] [review]
real changes mkIV

>+    if (this.mContentView.rootNode)
>+      this.mContentTree.view.selection.select(0);
This selects the rootNode itself, right?

>-    var bx = this.getBoxObject(this.mMethodTree);
>-    bx.view = this.mBinding ? new MethodTreeView(this.mBinding) : null;
>+    this.mMethodTree.view =
>+      this.mBinding ? new MethodTreeView(this.mBinding) : null;
Thus making getBoxObject even more unnecessary :-)

>+    // make sure a valid radio button is selected
>+    if (rgroup.selectedIndex < 0)
>+      rgroup.selectedIndex = 0;
>+    if (rgroup.selectedItem.disabled) {
>+      var other = rgroup.getItemAtIndex((rgroup.selectedIndex + 1) % 2);
>+      if (!other.disabled)
>+        rgroup.selectedItem = other;
Why not set the new selectedIndex directly?

>   readDOMText: function XBLBVr_ReadDOMText(aEl)
>   {
>-    var text = aEl.nodeValue;
>+    if (!aEl)
>+      return "";
When does this happen? (It should never happen in a recursive call, which would make it a waste of time in those cases.)

>+    // remove trailing spaces from all lines
>+    aStr = aStr.replace(/^ +$/gm, "");
This doesn't actually remove trailing spaces, only blank lines.

>+    var indentations = aStr.match(/^ *(?=[^\n])/gm) || [];
>+    var least = 0;
>+    for (var i = 1; i < indentations.length; ++i) {
>+      if (indentations[i].length < indentations[least].length)
>+        least = i;
>+    }
What happened to the first match? What does the (?=[^\n]) give you? Also, mapping and/or sorting and/or reducing might help.

>+    if (indentations.length)
Don't you want to check least here?

>+    if (params.length)
>+      pstr += params[0].getAttribute("name");
>+    for (var i = 1; i < params.length; ++i)
>+      pstr += ", " + params[i].getAttribute("name");
[If pstr was an array then you could just join(", ") it. Not sure what, if anything, you can do to make join work on a NodeList though.]

>+textbox {
>+  font-family: -moz-fixed;
>+}
Isn't that rule rather generic?

And don't forget those alternating backgrounds!

Comment 21

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: real changes mkV

r?neil for locale DTD line change

(In reply to comment #20)
> This selects the rootNode itself, right?
Yup.

> >+    // make sure a valid radio button is selected
> >+    if (rgroup.selectedIndex < 0)
> >+      rgroup.selectedIndex = 0;
> >+    if (rgroup.selectedItem.disabled) {
> >+      var other = rgroup.getItemAtIndex((rgroup.selectedIndex + 1) % 2);
> >+      if (!other.disabled)
> >+        rgroup.selectedItem = other;
> Why not set the new selectedIndex directly?
Okay, but what would I do to ensure |!other.disabled|?

> >   readDOMText: function XBLBVr_ReadDOMText(aEl)
> >   {
> >-    var text = aEl.nodeValue;
> >+    if (!aEl)
> >+      return "";
> When does this happen? (It should never happen in a recursive call, which would
> make it a waste of time in those cases.)



> >+    // remove trailing spaces from all lines
> >+    aStr = aStr.replace(/^ +$/gm, "");
> This doesn't actually remove trailing spaces, only blank lines.
Oops.

> >+    var indentations = aStr.match(/^ *(?=[^\n])/gm) || [];
> >+    var least = 0;
> >+    for (var i = 1; i < indentations.length; ++i) {
> >+      if (indentations[i].length < indentations[least].length)
> >+        least = i;
> >+    }
> What happened to the first match? What does the (?=[^\n]) give you? Also,
> mapping and/or sorting and/or reducing might help.
least is the index of indentations that contains the fewest number of spaces. We start at i = 1 because you can't compare the first element to the one before it, so we start with the second, where indentations[least] will be indentations[0] on the first execution of the loop body.

/^ */gm will match \n\n, which is no good since it will mean our attempts at left justification are foiled (it won't remove any indents), so /^ *(?=[^\n])/ only matches those lines where zero or more spaces are followed by a non-newline character. The (?=[^\n]) ensures that the non-newline character isn't at the tail-end of the strings in the array returned by match. /^ +/gm won't work either; consider the following code (where the 'v' in "var" is actually in column 1):

> var box = document.getElementById("search-box");
> var aEl = box.getElementsByTagName("textbox").item(0);
> if (!aEl) {
>   aEl = document.createElementNS(kXBLNSURI, "textbox";
>   box.appendChild(aEl);
> }

This would produce:

> var box = document.getElementById("search-box");
> var aEl = box.getElementsByTagName("textbox").item(0);
> if (!aEl) {
> aEl = document.createElementNS(kXBLNSURI, "textbox";
> box.appendChild(aEl);
> }

> >+    if (indentations.length)
> Don't you want to check least here?
Er..? Maybe the above explanation clears things up here.

> >+    if (params.length)
> >+      pstr += params[0].getAttribute("name");
> >+    for (var i = 1; i < params.length; ++i)
> >+      pstr += ", " + params[i].getAttribute("name");
> [If pstr was an array then you could just join(", ") it. Not sure what, if
> anything, you can do to make join work on a NodeList though.]
You mean params? Array.prototype.join.call(params, ", ") would work if we wanted to just join the members in params, but we're really interested in those elements' name attributes.

> >+textbox {
> >+  font-family: -moz-fixed;
> >+}
> Isn't that rule rather generic?
What do you mean? The selector? It's in xblBindings.css, and xblBindings.xul is the only user.

Comment 22

[Colby Russell :crussell (no longer Mozilla-ing)]

> >   readDOMText: function XBLBVr_ReadDOMText(aEl)
> >   {
> >-    var text = aEl.nodeValue;
> >+    if (!aEl)
> >+      return "";
> When does this happen? (It should never happen in a recursive call, which would
> make it a waste of time in those cases.)

Forgot to address this. Two out of three uses:
> this.justifySource(this.readDOMText(body));
> text = this.readDOMText(kids.item(0));

Where neither of those are guaranteed to be non-null.

Comment 23

[[e-mail]]

(In reply to comment #21)
>>>+    // make sure a valid radio button is selected
>>>+    if (rgroup.selectedIndex < 0)
>>>+      rgroup.selectedIndex = 0;
>>>+    if (rgroup.selectedItem.disabled) {
>>>+      var other = rgroup.getItemAtIndex((rgroup.selectedIndex + 1) % 2);
>>>+      if (!other.disabled)
>>>+        rgroup.selectedItem = other;
>>Why not set the new selectedIndex directly?
>Okay, but what would I do to ensure |!other.disabled|?
Sorry, I overlooked that, although rgroup.disabled would be true in that case.

[long and useful explanation snipped]
Well, I guess you could test aStr against /^[^ \n]/ to see if it was indented, and then matching against /^ +/ would work, right?

>>>+    if (indentations.length)
>>Don't you want to check least here?
>Er..? Maybe the above explanation clears things up here.
Not quite. I either think the test should be if (indentations[least]), or if you do mean .length then this doesn't change so you might as well test immediately after the match and skip the loop too.

> You mean params? Array.prototype.join.call(params, ", ") would work if we
> wanted to just join the members in params, but we're really interested in those
> elements' name attributes.
Bah, my fault again for reading your patch after midnight local time.

>>>+textbox {
>>>+  font-family: -moz-fixed;
>>>+}
>>Isn't that rule rather generic?
>What do you mean? The selector? It's in xblBindings.css, and xblBindings.xul is
>the only user.
Ah, I forgot that the new version has several textboxes.

Comment 24

[[e-mail]]

(In reply to comment #23)
> [long and useful explanation snipped]
> Well, I guess you could test aStr against /^[^ \n]/ to see if it was indented,
> and then matching against /^ +/ would work, right?
Oops, it's not as easy as I thought; I now like your regexp after all! Now I just need clarification on what that condition does.

Comment 25

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #24)
> Oops, it's not as easy as I thought; I now like your regexp after all! Now I
> just need clarification on what that condition does.

"[T]hat condition" being this indentations.length business? The conditional isn't to make sure that indentations[least] is a non-empty string (i.e., skipping the replace call in the event that the code is already flush with the far left column to begin with), it's to make sure that indentations[least] exists. Array.prototype.match can return null for no matches, and in that case, least will be 0, as initialized; the loop will never execute because it won't pass its initial condition; and executing
> aStr = aStr.replace(RegExp("^" + indentations[least], "gm"), "");
with no check will be in error, since indentations will be of length zero, since a null return from the call to match will set indentations to an empty array:
> var indentations = aStr.match(/^ *(?=[^\n])/gm) || [];
The above line is so that checking against indentations.length in the loop condition and the if condition in question is possible. (Although I am having trouble now envisioning the original scenarios I thought of in which match would return null. Stubbed getters/setters/methods/handlers seems to be the only case, but--to me--that's sufficient enough to worry about doing the check.)

Although, if you'd like the condition to be
> if (indentations.length && indentations[length])
I can do that, too, but that only saves a call to replace in the edge cases where XBL is already left-justified, but it adds an extra condition for the most typical cases. On second thought, those aren't edge cases at all, I guess, since this will also be used for getters and setters defined with the onget and onset attributes, and handlers defined with the action attribute. My initial instinct now is not do that, but instead make sure that justifySource is only called in conjunction with readDomText, eliminating a seemingly useless call when onget/onset/action attributes are used, but then again, what if somebody writes |action="  return this.mTree"|?

Comment 26

[Colby Russell :crussell (no longer Mozilla-ing)]

Also, should justifySource be commented better?

Comment 27

[[e-mail]]

One quick thought:
var indentations = aStr.match(/^ *(?=[^\n])/gm);
if (indentations) {
  indentations = indentations.sort();
  if (indentations[0])
    aStr = aStr.replace(RegExp("^" + indentations[0], "gm"), "");
}

Comment 28

[Shawn Wilsher :sdwilsh]

Comment on attachment 418329 [details] [diff] [review]
cleanup mkIII

nit: application/x-javascript should be just application/javascript

r=sdwilsh with that fixed.

Comment 29

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: mkVI cleanup

r?sdwilsh since a I made some additional changes: indentation for jar.mn and consistent use of namespace URI consts versus string literals

Comment 30

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: mkVI realchanges

Comment 31

[[e-mail]]

Comment on attachment 419963 [details] [diff] [review]
mkVI realchanges

I noticed some extra {}s added to single-line controlled statements, which to me would seem to belong in the previous patch.

>+  // Convert all tabs used for indentation to spaces up to the next tabstop.
>+  // Tabstops are assumed to be 8 columns.
>+  detab: function XBLBVr_Detab(aStr)
It might be possible to simplify this depending on how accurately you want to detab. Let me try some possibilities:
1. aStr = aStr.replace(/ {0,7}[ \t]/g, "        ");
2. while (/^\t/m.test(aStr))
     aStr = aStr.replace(/^(\t*)\t/gm, "$1        ");
3. aStr = aStr.replace(/^\t+/gm, function times8spaces(val) {
  return (val+val+val+val+val+val+val+val).replace(/\t/g, " ")
});
4. aStr = aStr.replace(/^[ \t]*\t/gm, detabify); // XXX write detabify
5. while (/^ *\t/m.test(aStr))
     aStr = aStr.replace(/^((        )*) {0,7}\t/gm, "$1        ");
(Is that last one too clever to be readable?)

>+      // this loop-and-check is necessary, because global replace doesn't work
>+      // with the beginning of line constraint in the RegExp above
Not sure what you mean here, see above.

Comment 32

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: mkVII cleanup [Checkin: Comment 37]

Comment 33

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: mkVII realchanges [Checkin: Comment 38]

(In reply to comment #31)
> (From update of attachment 419963 [details] [diff] [review])
> I noticed some extra {}s added to single-line controlled statements, which to
> me would seem to belong in the previous patch.

I forgot to mention this. It seemed easier to brace the two instances that I
wasn't already touching in the second patch than trying to splice in the
changes and generating a new cleanup patch. But that's what I did here. I
assume sr+ for it.

> >+  // Convert all tabs used for indentation to spaces up to the next tabstop.
> >+  // Tabstops are assumed to be 8 columns.
> >+  detab: function XBLBVr_Detab(aStr)
> It might be possible to simplify this depending on how accurately you want to
> detab. Let me try some possibilities:

Of the first four, I prefer the first for its accuracy, and since the fifth is
similar but more complete (and only messes with the tabs used for
indentation), I'll go with it.

> >+      // this loop-and-check is necessary, because global replace doesn't work
> >+      // with the beginning of line constraint in the RegExp above
> Not sure what you mean here, see above.

I was referring to didReplace:

>+      do {
>+        var didReplace = false;
>+        lines[i] = lines[i].replace(tabr, function XBLBVr_DetabReplace(match)
>+        {
>+          didReplace = true;
...
>+          }
...
>+        });
>+      } while (didReplace);

Using the global flag with my tabr has no effect. I narrowed it down to the
beginning-of-line constraint; using |tabr = / *\t/g| works, but it also has the
potential to mess with tabs not used for indentation. In yours, the while loop accomplishes the same thing as my didReplace workaround.

Comment 34

[[e-mail]]

Comment on attachment 420104 [details] [diff] [review]
mkVII cleanup
[Checkin: Comment 37]

Ironically bugzilla interdiff actually worked for this patch so I would have been able to sr this quickly anyway.

Comment 35

[Shawn Wilsher :sdwilsh]

Comment on attachment 420104 [details] [diff] [review]
mkVII cleanup
[Checkin: Comment 37]

r=sdwilsh

Comment 36

[Shawn Wilsher :sdwilsh]

Comment on attachment 420107 [details] [diff] [review]
mkVII realchanges
[Checkin: Comment 38]

r=sdwilsh

Comment 37

[Serge Gautherie (:sgautherie)]

Comment on attachment 420104 [details] [diff] [review]
mkVII cleanup
[Checkin: Comment 37]


http://hg.mozilla.org/dom-inspector/rev/5236b0db17ba

Comment 38

[Serge Gautherie (:sgautherie)]

Comment on attachment 420107 [details] [diff] [review]
mkVII realchanges
[Checkin: Comment 38]


http://hg.mozilla.org/dom-inspector/rev/190f5438a253

Comment 39

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: remove multipanel bindings and style [Checkin: Comment 41]

I lost the multipanel bindings and style removal requested in comment 7 somewhere between mkVI and mkVII

Comment 40

[Shawn Wilsher :sdwilsh]

Comment on attachment 422141 [details] [diff] [review]
remove multipanel bindings and style
[Checkin: Comment 41]

Dead code goes bye bye :)

r=sdwilsh

Comment 41

[Serge Gautherie (:sgautherie)]

Comment on attachment 422141 [details] [diff] [review]
remove multipanel bindings and style
[Checkin: Comment 41]


http://hg.mozilla.org/dom-inspector/rev/5735ca109135