360898 - Option to hide processing instructions

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, enhancement)

Description

[[e-mail]]

Processing instructions (and there can be quite a few for documents with many overlays) now appear in XUL documents, but I think they're just clutter ;-)

Comment 1

[Shawn Wilsher :sdwilsh]

Do we want to add a new preference - something like "inspector.dom.showProcessingInstructions"?

Comment 2

[[e-mail]]

Sounds good to me.

P.S. Did you mean to assign it to yourself? ;-)

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

I can't actually test this since my computer doesn't want to build for whatever reason, but I figured I'd throw this out there.  I think it will accomplish the task at hand, but I'm not sure :(

Comment 4

[Alex Vincent [:WeirdAl]]

Comment on attachment 245785 [details] [diff] [review]
v1.0

>Index: layout/inspector/public/inIDOMView.idl
> interface inIDOMView : nsISupports

Rev the IID.

Comment 5

[[e-mail]]

Hey, I wonder whether we can do this by tweaking the whatToShow attribute?

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #5)
> Hey, I wonder whether we can do this by tweaking the whatToShow attribute?

I suppose we could do that if we did something like:
NS_IMETHODIMP
inDOMView::SetShowProcessingInstructions(PRBool aShowProcessingInstructions)
{
  if (mShowProcessingInstructions == aShowProcessingInstructions)
    return NS_OK;
  mShowProcessingInstructions = aShowProcessingInstructions;
  if (mShowProcessingInstructions) {
    mWhatToShow = mWhatToShow | nsIDOMNodeFilter::SHOW_PROCESSING_INSTRUCTION;
  } else {
    mWhatToShow = mWhatToShow ^ nsIDOMNodeFilter::SHOW_PROCESSING_INSTRUCTION;
  }
  return NS_OK;
}

Someone check me on that logic though.

Comment 7

[[e-mail]]

Or you could move the bit-twiddling logic into the JS front end.

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

This works quite nicely (I forgot I could build on my mac).

Neil - I didn't move the bit twiddling to the JS because I felt that if anyone else ever wanted to use this in something else, I'd rather it work for them instead of the fiddling with it in their JS.  Doing it this way keeps the toggleProcessingInstructions/setProcessingInstructions inline with other existing toggleSomething/setSomething functions in dom.js.

This removes outdated locales from the Makefile and adds the proper entities to the dom.dtd file.  Let me know if I should use a different access key.

Comment 9

[[e-mail]]

Comment on attachment 245808 [details] [diff] [review]
v1.1

>+    this.setProcessingInstructions(PrefUtils.getPref("inspector.dom.showProcessingInstructions"));
Execpt it's a two-arg function :-P

>+  mShowProcessingInstructions(PR_TRUE),
>   mWhatToShow(nsIDOMNodeFilter::SHOW_ALL)
I don't like this because the two could get out of sync.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> (From update of attachment 245808 [details] [diff] [review] [edit])
> >+    this.setProcessingInstructions(PrefUtils.getPref("inspector.dom.showProcessingInstructions"));
> Execpt it's a two-arg function :-P

whoops...

> >+  mShowProcessingInstructions(PR_TRUE),
> >   mWhatToShow(nsIDOMNodeFilter::SHOW_ALL)
> I don't like this because the two could get out of sync.

Good point.  I'll have a revised patch this evening.  Looks like I'll be playing with whatToShow in the JS.

Comment 11

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

Alright, this doesn't change the component anymore and does all of it's magic in the JS file.  I cleaned up some formatting of code (added javadoc headers and changed anonymous functions to named functions) when I was nearby it.

I also forgot to include the changes to the default preferences file for the DOM Inspector.

Comment 12

[[e-mail]]

Comment on attachment 245871 [details] [diff] [review]
v1.2

>-  this.mDOMView.whatToShow &= ~(NodeFilter.SHOW_ATTRIBUTE); // hide attribute nodes
>+  // hide attribute nodes
>+  this.mDOMView.whatToShow &= ~(NodeFilter.SHOW_ATTRIBUTE);
>+  if (PrefUtils.getPref("inspector.dom.showProcessingInstructions")) {
>+    this.mDOMView.whatToShow |= NodeFilter.SHOW_PROCESSING_INSTRUCTION;
>+  } else {
>+    this.mDOMView.whatToShow &= ~(NodeFilter.SHOW_PROCESSING_INSTRUCTION);
>+  }
Nit: all options are on by default, so you only need to turn it off if the pref is unset. I don't know why the existing code uses ~(NodeFilter.SHOW_ATTRIBUTE); I would just use ~NodeFilter.SHOW_ATTRIBUTE;

>+    var value = !PrefUtils.getPref("inspector.dom.showProcessingInstructions");
Actually to be consistent with the other settings you should use
!(this.mDOMView.whatToShow & NodeFilter.SHOW_PROCESSING_INSTRUCTION)

>+    var curVal = PrefUtils.getPref("inspector.dom.showProcessingInstructions");
>+    // If the value isn't changing, don't run
>+    if (curVal == aValue) return;
This might not be optimal, see the bug I'm about to file.

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> Nit: all options are on by default, so you only need to turn it off if the pref
> is unset. I don't know why the existing code uses ~(NodeFilter.SHOW_ATTRIBUTE);
> I would just use ~NodeFilter.SHOW_ATTRIBUTE;

Fixed.

> Actually to be consistent with the other settings you should use
> !(this.mDOMView.whatToShow & NodeFilter.SHOW_PROCESSING_INSTRUCTION)

Fixed.

> This might not be optimal, see the bug I'm about to file.

I believe I handle it correctly though (https://bugzilla.mozilla.org/attachment.cgi?id=245871&action=diff#mozilla/extensions/inspector/resources/content/viewers/dom/dom.js_sec5), but I do agree that the existing code for other things didn't work right.

The main reason why that line is there is because rebuild is expensive, and if we don't have to rebuild, I don't want to.  This is only an issue when loading because I set whatToShow in the DOMView constructor, and then I call setProcessingInstructions in the initialize function to set-up the menu items.  I essentially copied this idea from setWhitespaceNodes, but they used a lot more comments to explain it.  I can do that if need be.

Comment 14

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

So, I lied about being sure that it would work right with the observer.  So, I had to make a code change that isn't big, but I figure it should be reviewed.

The observer works nicely (I tested it while playing in about:config).

Comment 15

[[e-mail]]

Comment on attachment 245924 [details] [diff] [review]
v1.3

>+                                                                aRebuild)
I don't understand why you reintroduced this parameter.

>+    var curVal = PrefUtils.getPref("inspector.dom.showProcessingInstructions");
Unused?

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> I don't understand why you reintroduced this parameter.

We don't rebuild on initialize because it will already have the correct value from the DOMView constructor.  We do however need to call the function to set the check status of the command attribute.  Every other call will rebuild, however.

> Unused?

Yes, that shouldn't be there.  Will be removed in final patch.

Comment 17

[[e-mail]]

Comment on attachment 245924 [details] [diff] [review]
v1.3

>+  if (!PrefUtils.getPref("inspector.dom.showProcessingInstructions")) {
>+    this.mDOMView.whatToShow &= ~NodeFilter.SHOW_PROCESSING_INSTRUCTION;
>+  }
Except you have custom initialization code...

Comment 18

[Shawn Wilsher :sdwilsh]

(In reply to comment #17)
> Except you have custom initialization code...

I see I need a better explanation.  In the constructor for DOMViewer I do have code that initializes the state of whatToShow.  This happens before the initial build of the tree, which as far as I can tell happens in the second to last line of the constructor (http://lxr.mozilla.org/seamonkey/source/extensions/inspector/resources/content/viewers/dom/dom.js#86).  However, this doesn't do anything with the menu items and their checked status.  This is why initialize calls setProcessingInstructions with the rebuild parameter set to false.  The tree has already been built correctly, but the menuitem's checked state hasn't been updated yet.

The problem with v1.2's code was that I was attempting to prevent rebuild if the value passed to the function was the same as the preference.  However, this wouldn't work correctly if the preference was changed elsewhere and the observer called the function.

I suppose I could drop rebuild and just always rebuild the tree, but then we are doing a rebuild when we don't need to (and arguably shouldn't) when initialize calls this.setProcessingInstructions.

Comment 19

[[e-mail]]

(In reply to comment #18)
>However, this doesn't do anything with the menu items and their checked
>status.  This is why initialize calls setProcessingInstructions with the
>rebuild parameter set to false.  The tree has already been built correctly, but
>the menuitem's checked state hasn't been updated yet.
So if you have to call setProcessingInstructions can you remove that custom initialization?

Comment 20

[Shawn Wilsher :sdwilsh]

(In reply to comment #19)
> So if you have to call setProcessingInstructions can you remove that custom
> initialization?

I could, but then we would build the tree in the constructor (DOMViewer), and then rebuild the tree in setProcessingInstructions.  I feel that building the tree twice would be a bad thing, but if that is the direction you want me to take, I can do that.

Comment 21

[[e-mail]]

Sorry, I meant leave the rebuild flag in, but don't initialise the tree twice [first with the custom code and second with setProcessingInstructions(false)].

Comment 22

[[e-mail]]

Comment on attachment 245924 [details] [diff] [review]
v1.3

As I read it,
>+  if (!PrefUtils.getPref("inspector.dom.showProcessingInstructions")) {
>+    this.mDOMView.whatToShow &= ~NodeFilter.SHOW_PROCESSING_INSTRUCTION;
>+  }
is made superfluous by
>+    this.setProcessingInstructions(PrefUtils.getPref("inspector.dom.showProcessingInstructions"), false);
which you need anyway to update the menuitem.

Comment 23

[Shawn Wilsher :sdwilsh]

(In reply to comment #22)
> As I read it,
> >+  if (!PrefUtils.getPref("inspector.dom.showProcessingInstructions")) {
> >+    this.mDOMView.whatToShow &= ~NodeFilter.SHOW_PROCESSING_INSTRUCTION;
> >+  }
> is made superfluous by
> >+    this.setProcessingInstructions(PrefUtils.getPref("inspector.dom.showProcessingInstructions"), false);
> which you need anyway to update the menuitem.

This is true.  However, this builds the tree, correct?
>    this.mDOMTree.treeBoxObject.view = this.mDOMView;
Unless we want to rebuild we need the code in question.  Would you be happier if I did something like this?
>+  setProcessingInstructions: function setProcessingInstructions(aValue,
>+                                                                aRebuild)
>+  {
>+    this.mPanel.panelset.setCommandAttribute("cmd:toggleProcessingInstructions",
>+                                             "checked", aValue);
>+    if (!aRebuild) return;
>+    var curVal = PrefUtils.getPref("inspector.dom.showProcessingInstructions");
>+
>+    if (aValue) {
>+      this.mDOMView.whatToShow |= NodeFilter.SHOW_PROCESSING_INSTRUCTION;
>+    } else {
>+      this.mDOMView.whatToShow &= ~NodeFilter.SHOW_PROCESSING_INSTRUCTION;
>+    }
>+    if ( curValue != aValue) {
>+      PrefUtils.setPref("inspector.dom.showProcessingInstructions", aValue);
>+    }
>+
>+    this.rebuild();
>+  },

Explanation:
If we don't want to rebuild, we return before setting this.mDOMViewer.whatToShow againe.  We will only return when initialize is called since it is the only time rebuild is false.  Otherwise, we rebuild because this.mDOMViewer.whatToShow should be changing.

The whole point of this is to prevent a rebuild on the initial loading of the viewer.

Comment 24

[[e-mail]]

Actually the initial rebuild happens somewhere inside notifyViewerReady so I therefore put it to you that we're not actually ready when we notify ;-)

Comment 25

[Shawn Wilsher :sdwilsh]

(In reply to comment #24)
> Actually the initial rebuild happens somewhere inside notifyViewerReady so I
> therefore put it to you that we're not actually ready when we notify ;-) 

Well, if the tree is built in notifyViewerReady, aren't we ready then?  Or are you saying that aPane.notifyViewerReady(this); should be the last thing called in initialize?

Comment 26

[[e-mail]]

(In reply to comment #25)
>Or are you saying that aPane.notifyViewerReady(this); should be the last thing
>called in initialize?
Sounds good to me.

Comment 27

[Shawn Wilsher :sdwilsh]

Attachment: v1.4

I believe this addresses all the issues brought up.

Comment 28

[[e-mail]]

Comment on attachment 246242 [details] [diff] [review]
v1.4

>-  this.mDOMView.whatToShow &= ~(NodeFilter.SHOW_ATTRIBUTE); // hide attribute nodes
>+  // hide attribute nodes
>+  this.mDOMView.whatToShow &= ~NodeFilter.SHOW_ATTRIBUTE;
>+  if (!PrefUtils.getPref("inspector.dom.showProcessingInstructions")) {
>+    this.mDOMView.whatToShow &= ~NodeFilter.SHOW_PROCESSING_INSTRUCTION;
>+  }
So, my idea is that by changing the code below to match the other cases i.e. if (aRebuild) this.rebuild(); then you don't need the change above.
>+    if (!aRebuild) return;
>+
>+    if (aValue) {
>+      this.mDOMView.whatToShow |= NodeFilter.SHOW_PROCESSING_INSTRUCTION;
>+    } else {
>+      this.mDOMView.whatToShow &= ~NodeFilter.SHOW_PROCESSING_INSTRUCTION;
>+    }
>+    
>+    var curVal = PrefUtils.getPref("inspector.dom.showProcessingInstructions");
>+    if (curVal != aValue) {
>+      PrefUtils.setPref("inspector.dom.showProcessingInstructions", aValue);
>+    }
>+
>+    this.rebuild();
Also I don't think you need the curVal check, and watch for trailing spaces.

Comment 29

[Shawn Wilsher :sdwilsh]

Attachment: v1.5

Ok, so as far as I'm seeing, this works.

I still have the currValue check, and that's because when we change the pref, the observer will be called, which calls this function, which would change the pref, which would call the observer....yey for recursion.

I think I addressed all of your comments as well.

Comment 30

[[e-mail]]

Comment on attachment 248012 [details] [diff] [review]
v1.5

sr=me with these fixed:

>-  this.mDOMView.whatToShow &= ~(NodeFilter.SHOW_ATTRIBUTE); // hide attribute nodes
>+  // hide attribute nodes
>+  this.mDOMView.whatToShow &= ~NodeFilter.SHOW_ATTRIBUTE;
Don't bother with this change any more.

>+    var curVal = PrefUtils.getPref("inspector.dom.showProcessingInstructions");
>+    if (curVal != aValue) {
>+      PrefUtils.setPref("inspector.dom.showProcessingInstructions", aValue);
>+    }
Strangely enough all the other prefs manage without it ;-) - for details see pref_ValueChanged in prefapi.cpp - but as I mentioned in bug 361146 you could alternatively make ToggleProcessingInstructions set the pref directly.

>+  <!ENTITY cmdToggleProcessingInstructions.label "Show Processing Instructions">
>+  <!ENTITY cmdToggleProcessingInstructions.accesskey "p">
Nit: should be uppercase P to match (access key code prefers a case-sensitive match first, then falls back to a case-insensitive match).

Comment 31

[Shawn Wilsher :sdwilsh]

Attachment: v1.6

Comment 32

[Daniel Brooks [:db48x]]

Comment on attachment 248037 [details] [diff] [review]
v1.6

>+      <menuitem id="item:toggleProcessingInstructions"
>+    <menuitem id="item:toggleProcessingInstructions"

These two have the same id. You could actually get rid of the id altogether, since nothing ever needs to refer to this element by it's id.

My only other comment is that you ought to avoid the setShowPI() -> onPrefChanged() -> setShowPI() recursion. It's not fatal because it stops after one iteration, because the second setShowPI() will set the pref to the value the pref already has so onPrefChanged() won't be called again, but it is wasteful. Make setShowPI() set the pref, and make onPrefChanged() call rebuild(). You're probably just following the pattern set by the rest of the code, but that doesn't make it right.

r=db48x with those changes.

Comment 33

[[e-mail]]

(In reply to comment #32)
>(From update of attachment 248037 [details] [diff] [review])
>>+      <menuitem id="item:toggleProcessingInstructions"
>>+    <menuitem id="item:toggleProcessingInstructions"
>These two have the same id.
I think these menuitems get overlaid into in different documents.

>My only other comment is that you ought to avoid the setShowPI() ->
>onPrefChanged() -> setShowPI() recursion.
I meant to file a bug on that (for the other prefs) - see comment 30.

Comment 34

[Daniel Brooks [:db48x]]

(In reply to comment #33)
> (In reply to comment #32)
> >(From update of attachment 248037 [details] [diff] [review] [edit])
> >>+      <menuitem id="item:toggleProcessingInstructions"
> >>+    <menuitem id="item:toggleProcessingInstructions"
> >These two have the same id.

> I think these menuitems get overlaid into in different documents.

They're still in the same XML document (the overlay file) and therefore shouldn't have the same id. And anyway, nothing ever uses the id, so it's pointless. That said, it's just a nit and I really don't care much.

> 
> >My only other comment is that you ought to avoid the setShowPI() ->
> >onPrefChanged() -> setShowPI() recursion.

> I meant to file a bug on that (for the other prefs) - see comment 30.
 
cc me on that bug, if you would.

Comment 35

[Shawn Wilsher :sdwilsh]

(In reply to comment #32)
> (From update of attachment 248037 [details] [diff] [review] [edit])
> >+      <menuitem id="item:toggleProcessingInstructions"
> >+    <menuitem id="item:toggleProcessingInstructions"
> 
> These two have the same id. You could actually get rid of the id altogether,
> since nothing ever needs to refer to this element by it's id.
> 
> My only other comment is that you ought to avoid the setShowPI() ->
> onPrefChanged() -> setShowPI() recursion. It's not fatal because it stops after
> one iteration, because the second setShowPI() will set the pref to the value
> the pref already has so onPrefChanged() won't be called again, but it is
> wasteful. Make setShowPI() set the pref, and make onPrefChanged() call
> rebuild(). You're probably just following the pattern set by the rest of the
> code, but that doesn't make it right.
> 
> r=db48x with those changes.

Would you be OK with both of those issues being fixed in separate bugs since the whole file really needs to be changed for both?

Comment 36

[Daniel Brooks [:db48x]]

(In reply to comment #35)
> (In reply to comment #32)
> > r=db48x with those changes.
> 
> Would you be OK with both of those issues being fixed in separate bugs since
> the whole file really needs to be changed for both?
> 

Yes

Comment 37

[Shawn Wilsher :sdwilsh]

(In reply to comment #32)
> My only other comment is that you ought to avoid the setShowPI() ->
> onPrefChanged() -> setShowPI() recursion. It's not fatal because it stops after
> one iteration, because the second setShowPI() will set the pref to the value
> the pref already has so onPrefChanged() won't be called again, but it is
> wasteful. Make setShowPI() set the pref, and make onPrefChanged() call
> rebuild(). You're probably just following the pattern set by the rest of the
> code, but that doesn't make it right.

Before I file the bug, I want to check - that fix doesn't actually stop the wasteful call to set the pref, and you just want to drop the rebuild parameter and put that in onPrefChanged, correct?

Comment 38

[[e-mail]]

(In reply to comment #37)
>Before I file the bug, I want to check - that fix doesn't actually stop the
>wasteful call to set the pref
Old code:
menuitem -> command -> toggler -> setter -> pref -> observer -> setter -> pref
New code:
menuitem -> command -> toggler -> pref -> observer -> setter

I haven't thought about the rebuilding.

Comment 39

[Shawn Wilsher :sdwilsh]

r=db48x, sr=neil

Comment 40

[:Gavin Sharp [email: [e-mail]]]

mozilla/extensions/inspector/resources/Makefile.in 	1.31
mozilla/extensions/inspector/resources/content/prefs/inspector.js 	1.14
mozilla/extensions/inspector/resources/content/viewers/dom/commandOverlay.xul 1.9
mozilla/extensions/inspector/resources/content/viewers/dom/dom.js 	1.46
mozilla/extensions/inspector/resources/content/viewers/dom/popupOverlay.xul 1.9
mozilla/extensions/inspector/resources/locale/en-US/viewers/dom.dtd 	1.10