331926 - Remove redundant and unnecessary code in dom.js - cmdInspectBrowser and cmdInspectBrowserIsValid

Status: RESOLVED INCOMPLETE

(Other Applications :: DOM Inspector, defect)

Description

[Jason Barnabe (np)]

cmdInspectBrowser and cmdInspectBrowserIsValid handle whether to show the "Inspect Contained Document" context menu item in the DOM Nodes panel. Right now, both functions are checking the node's name, sometimes checking the namespace, sometimes not. Instead of doing that, all the code really has to do is check for the existance of node.contentDocument.

Comment 1

[Jason Barnabe (np)]

Attachment: patch v1

Comment 2

[timeless]

Comment on attachment 216482 [details] [diff] [review]
patch v1

>Index: extensions/inspector/resources/content/viewers/dom/dom.js
>@@ -304,42 +304,24 @@ DOMViewer.prototype = 

>   cmdInspectBrowserIsValid: function()
>   {
>     var node = viewer.selectedNode;
>+    if (!node || node.nodeType != Node.ELEMENT_NODE)
>+      return false;

that's kind of nice.

but this introduces a strictwarning/reference to undefined property.

you could do "contentDocument" in node i suppose... that might be scalable...

>+    return !!node.contentDocument;

Comment 3

[Jason Barnabe (np)]

If you're referring to...

JavaScript strict warning: chrome://inspector/content/inspector.xml, line 455: reference to undefined property event.originalTarget.nodeType

That existed before this patch.

If you're referring to !!node.contentDocument causing a warning, it doesn't.

Comment 4

[Jason Barnabe (np)]

On the matter of whether it *should* give a warning, I filed bug 332299, which was marked WONTFIX.

Comment 5

[timeless]

Comment on attachment 216482 [details] [diff] [review]
patch v1

oh well

Comment 6

[[e-mail]]

Comment on attachment 216482 [details] [diff] [review]
patch v1

Since DOM Inspector can't use XPC native wrappers by default, this change now makes it slightly more open to abuse; any chance you can address the issue?

Comment 7

[Jason Barnabe (np)]

(In reply to comment #6)
> (From update of attachment 216482 [details] [diff] [review] [edit])
> Since DOM Inspector can't use XPC native wrappers by default, this change now
> makes it slightly more open to abuse; any chance you can address the issue?

How would I do that? By adding a node instanceof nsIDOMNode check to cmdInspectBrowserIsValid?

Comment 8

[[e-mail]]

(In reply to comment #7)
>How would I do that?
Well there are two problems.
The first is that a web page can write anElement.contentDocument = something; and with your code DOM Inspector will try to inspect it, whereas with the original code it would only work with a well-known tag in a random namespace (in the correct namespace the property is readonly). Thus I don't think you should try to simplify the validity check; if anything you should check more strictly.
There is a variant which is is harder to solve, and after some testing might not even be realistically possible, because of the way xul and xbl work^H^H^H^Hsuck.

Comment 9

[Jason Barnabe (np)]

(In reply to comment #8)
> (in the correct namespace the property is readonly).
So why don't we restrict it to namespaces where contentDocument is readonly? (HTML and XUL, I assume.)

>Thus I don't think you should try to simplify the validity check; if anything you should check more strictly.
Strict and simple aren't mutually exclusive.

Comment 10

[Shawn Wilsher :sdwilsh]

Couldn't we create an XPCNativeWrapper to use for the check?

Comment 11

[[e-mail]]

(In reply to comment #10)
>Couldn't we create an XPCNativeWrapper to use for the check?
That doesn't work on XUL elements (because XPCNativeWrapper bypasses XBL).

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> That doesn't work on XUL elements (because XPCNativeWrapper bypasses XBL).

That seems to scream "security hole" to me if someone is using remote XUL.

Comment 13

[Javi Rueda :javirid]

cmdInspectBrowserIsValid function was removed from code in http://hg.mozilla.org/dom-inspector/rev/257b43aa4180

Comment 14

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

Note that the logic was moved to isCommandEnabled.

Comment 15

[Firefox Bug Husbandry Bot]

Bulk close. This component is no longer supported or maintained.

https://bugzilla.mozilla.org/show_bug.cgi?id=1499023

Comment 16

[Firefox Bug Husbandry Bot]

Bulk close. This component is no longer supported or maintained.

https://bugzilla.mozilla.org/show_bug.cgi?id=1499023