307255 - DOM Inspector "blink selected element" wrong on startup if "show whitespace nodes" is false

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[dolphinling]

DOM Inspector "blink selected element" is no longer the default, and doesn't
stick between times you open it. This is an immensely annoying regression.

Hmm... as I write this I notice that inspector.blink.on = true, and flipping it
twice turns it on. So it looks like it's not checking the pref each time I start
it up.

Regression range coming.

Comment 1

[dolphinling]

Er, nevermind. It works with a clean profile.

But the pref is correct, so why... ARGH.

Comment 2

[dolphinling]

AHA! The "show whitespace nodes" pref is wrongly interacting with "blink
selected element". Basically, if you have "show whitespace nodes"=false, "blink
selected element" will be false on startup, regardless of what its actual value is.

Reopening, changing summary, etc. And I will find that regression range.

Comment 3

[dolphinling]

Regressed between 2005-08-19-06 and 2005-08-20-06 on trunk, I assume the same
time for branch. I don't see anything that looks like a likely culprit,
unfortunately.

Comment 4

[Andrew Schultz]

I tracked this down to bug 303981

With builds with the bug, I get this:
[Exception... "'[JavaScript Error: "this.mDOMView.selection has no properties"
{file: "chrome://inspector/content/viewers/dom/dom.js" line: 630}]' when calling
method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021
(NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]

Comment 5

[dolphinling]

Can this block 1.8b5? It's a regression that's been tracked down to the checkin
that regressed it, and for web developers with certain configurations, it has
the potential to be a real pain.

Comment 6

[Asa Dotzler [:asa]]

If we get a fix for this that's low risk and has appropriate reviews, we may
consider this but I don't think we're going to block on this.

Comment 7

[dolphinling]

I believe I have figured out the cause of this bug, though it's entirely
possible I'm dead wrong, since I really have no idea what I'm doing.

In dom.js
http://lxr.mozilla.org/mozilla1.8/source/extensions/inspector/resources/content/viewers/dom/dom.js#121

initialize does this:

this.setAnonContent(PrefUtils.getPref("inspector.dom.showAnon"), false);
this.setWhitespaceNodes(PrefUtils.getPref("inspector.dom.showWhitespaceNodes"));
this.setFlashSelected(PrefUtils.getPref("inspector.blink.on"));

But if setWhitespaceNodes finds it doesn't need to change anything...
http://lxr.mozilla.org/mozilla1.8/source/extensions/inspector/resources/content/viewers/dom/dom.js#217

...it returns, and initialize never _gets_ to setFlashSelected.

So this could be fixed either by moving setFlashSelected above
setWhitespaceNodes, or by changing setWhitespaceNodes so it doesn't return
unnecessarily.

Comment 8

[dolphinling]

Attachment: patch

This patch uses the first method, since the second is beyond me.

Comment 9

[[e-mail]]

So, this one eventually annoyed me sufficiently to look into it. Both
dolphinling and ajschult have a grasp at different parts of the problem. What's
happening is that ajschult's exception (which appears to be genuine, and not
caused by bug 303981) is causing setFlashSelected to be skipped.

By comparison setAnonContent does not throw an exception, as it uses an aReload
parameter. It seems to me that setWhitespaceNodes should use the same system.

Comment 10

[[e-mail]]

OK, so this is really a regression from bug 221619.

Comment 11

[[e-mail]]

Attachment: Get the selection from the tree view

This is easier than fiddling around with QI.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 199929 [details] [diff] [review]
Get the selection from the tree view

r=bzbarsky, dammit.  This was pissing me off too.

Neil, do you want to land this?  Fwiw, I think we should get this in on branch.

Comment 13

[[e-mail]]

Fix checked in to the trunk.

Comment 14

[Mike Schroepfer]

Comment on attachment 199929 [details] [diff] [review]
Get the selection from the tree view

Please land on 1.8 and 1.8.1 branches. Thanks!

Comment 15

[Boris Zbarsky [:bzbarsky]]

*** Committing to MOZILLA_1_8_BRANCH... 
new revision: 1.35.4.1; previous revision: 1.35
*** Committing extensions/inspector/resources/content/viewers/dom/dom.js on MOZILLA_1_8_0_BRANCH... 
new revision: 1.35.12.1; previous revision: 1.35