273202 - Support searching by XPath expressions in DOM inspector

Status: RESOLVED INCOMPLETE

(Other Applications :: DOM Inspector, enhancement)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

From: Alan Donovan <adonovan AT csail.mit.edu>

I just added some code to the DOM Inspector to provide rudimentary
support for XPath expressions.  If you apply the attached patch, you
will notice an additional XPath toolbar appears, enabling the
following operations:

1. Searching for elements in the tree widget by XPath expression.

   Type an XPath expression into the toolbar, and click
   "Evaluate".  This will report the value of the expression (if
   scalar), or the number of nodes matching, otherwise.
 
   Clicking on "Next Match" cycles through the matching nodes in
   order, revealing the node in the tree widget, and flashing the node
   in the browser window.

2. Suggestion of a simple XPath expression that matches a given node.

   By choosing "Suggest XPath for Element" from a node's popup menu, a
   naive path expression will appear in the toolbar.  (I indend to
   refine this in the future to provide more sophisticated XPath
   expressions that incorporate just enough attribute information to
   make the query uniquely identify the selected node.)
   
I hope the utility of these features is obvious.  My longer term goal
is to build an IDE that permits users to specify interesting parts of
web pages in a point-and-click fashion, and helps them craft XPath
expressions to describe those parts in a robust manner, allowing them
to be applied against future variations of the page.  Thus the user
can do "screen scraping" easily and robustly, and in effect "impose" a
Web Service abstraction onto an arbitrary web site.

Feel free to get in touch (or forward this message) if this direction
is of interest to you or others.

cheers
alan

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch from Alan Donovan

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

as for #2, note bug 208500

Comment 3

[Alex Vincent [:WeirdAl]]

Comment on attachment 167894 [details] [diff] [review]
Patch from Alan Donovan

A few preliminary nits:

>Index: extensions/inspector/resources/content/inspector.js
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/inspector/resources/content/inspector.js,v
>retrieving revision 1.21
>diff -u -w -b -B -r1.21 inspector.js
>--- extensions/inspector/resources/content/inspector.js	23 Nov 2004 23:00:20 -0000	1.21
>+++ extensions/inspector/resources/content/inspector.js	5 Dec 2004 02:39:15 -0000
>@@ -164,6 +164,11 @@
>             aEvent.subject && "location" in aEvent.subject) {
>           this.locationText = aEvent.subject.location; // display document url
>         }
>+        this.mDocPanel.viewer.addObserver("suggestXPath", this, false);
>+        break;
>+      case "suggestXPath":

We really need more lines of context to figure out where this is.

>+  XPathNextMatch : function() {
>+      if(this.mDocPanel.viewer == null) {
>+	  alert("No document is being inspected.");

Alerts have bad vibes.	This function should never be called if the viewer is
null.  Ideally, disable the command element that would call this function.

>+	  return;
>+      }
>+      this.mDocPanel.viewer.XPathNextMatch();

I really don't like having the same function name in inspector.js and in the
DOM Nodes viewer.  Call it a hunch (that you don't have to agree with or
conform to...)

>+  },
>+
>   ////////////////////////////////////////////////////////////////////////////
>   //// UI Labels getters and setters
> 
>Index: extensions/inspector/resources/content/inspector.xul
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/inspector/resources/content/inspector.xul,v
>retrieving revision 1.9
>diff -u -w -b -B -r1.9 inspector.xul
>--- extensions/inspector/resources/content/inspector.xul	18 May 2003 12:15:00 -0000	1.9
>+++ extensions/inspector/resources/content/inspector.xul	5 Dec 2004 02:39:15 -0000
>@@ -80,6 +80,8 @@
>   <toolbox id="tbxInsToolbox">
>     <menubar id="mbrInspectorMain"/>
>     <toolbar id="tbInspectorPrimary"/>
>+    <!-- [adonovan] -->

Please remove that in a final patch.  It really doesn't help, and a credit line
would better go in the file license.

>Index: extensions/inspector/resources/content/toolboxOverlay.xul
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/inspector/resources/content/toolboxOverlay.xul,v
>retrieving revision 1.12
>diff -u -w -b -B -r1.12 toolboxOverlay.xul
>--- extensions/inspector/resources/content/toolboxOverlay.xul	12 Jun 2002 00:25:28 -0000	1.12
>+++ extensions/inspector/resources/content/toolboxOverlay.xul	5 Dec 2004 02:39:15 -0000
>@@ -60,4 +60,31 @@
> 
>   </toolbar>
> 
>+  <--- !!! THIS FILE DOES NOT SEEM TO BE USED!!! --->

It's used.  See inspector.xul#45 in LXR.

>+
>+  <!-- [adonovan (moz/ext)] -->

Again, better to credit in the license.

>+    <hbox id="bxXPathBar" flex="1">
>+
>+      <hbox id="bxXPathBarContainer" flex="1">
>+        <hbox align="center" flex="1">
>+          <textbox id="tfXPathBar" flex="1">
>+          </textbox>
>+        </hbox>
>+
>+      </hbox>

We can reduce this code a bit.	(1) Why do you need exactly one hbox within
another?  (2) The textbox element is really empty, so you could go <textbox
id="tfXPathBar" flex="1"/>

>Index: extensions/inspector/resources/content/viewers/dom/dom.js
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/inspector/resources/content/viewers/dom/dom.js,v
>retrieving revision 1.33
>diff -u -w -b -B -r1.33 dom.js
>--- extensions/inspector/resources/content/viewers/dom/dom.js	21 Apr 2004 15:09:27 -0000	1.33
>+++ extensions/inspector/resources/content/viewers/dom/dom.js	5 Dec 2004 02:39:15 -0000
>@@ -96,6 +96,8 @@
>   mFindType: null,
>   mFindWalker: null,
>   mSelecting: false,
>+  mXPathMatchNodes: new Array(),

Can we use [] instead of new Array()?

>+  mLastMatchIndex: -1,
>   
>   ////////////////////////////////////////////////////////////////////////////
>   //// interface inIViewer
>@@ -261,6 +263,112 @@
>     return this.selectedNode ? (this.selectedNode.nodeType == Node.ELEMENT_NODE) : false;

Components.interfaces.nsIDOMNode.  You might want to include this line
somewhere near the top of this file if it's not already there:

const nsIDOMNode = Components.interfaces.nsIDOMNode;

>+      alert("No more matches.");

Again, bad vibes.

>+  // [adonovan (moz/ext)]

Again, credit in license.

>+  XPathEval: function(xpath)
>+  {
>+    var nsResolver =
>+    this.mSubject.createNSResolver(this.mSubject.documentElement);
>+    var contextNode = this.mSubject; // root of web XML
>+    var result = null;
>+    try {
>+      result = this.mSubject.evaluate(xpath, contextNode, nsResolver, 0, null);
>+    } catch (e) {
>+      alert("Invalid XPath expression:\n"+xpath);

dump() or throw an exception, please.  (I'm not sure which is better.)	

It might also be wise, if it's not too much trouble, to use on-the-fly
validation of the XPath expression (that is, in the textbox giving us the xpath
variable's value, set an input event listener or something like that to check
the expression.  That way we can disable the command if it's no good.)

>+      return;
>+    }
>+
>+    this.mXPathMatchNodes = new Array();
>+    this.mLastMatchIndex = -1;
>+
>+    if(result.resultType == XPathResult.NUMBER_TYPE) {
>+      alert("XPath Number: "+result.numberValue);
>+      return;
>+    } else if(result.resultType == XPathResult.STRING_TYPE) {
>+      alert("XPath String: "+result.stringValue);
>+      return;
>+    } else if(result.resultType == XPathResult.BOOLEAN_TYPE) {
>+      alert("XPath Boolean: "+result.booleanValue);
>+      return;

An else after a return isn't necessary.

>+      for(var ii = 0; ii < result.snapshotLength; ++ii) {

Space between for and (, please.  Also, is there any particular reason we're
using "ii" as the variable name?  (Just a nit, not a serious objection.)

>+      alert("Can't yet handle XPath result type "+result.resultType);

...

You know, there's another big problem with these alert messages.   They're not
localized in any sense.

caillon: we should really think about this.  Maybe a XUL section in DOM
Inspector's overall window for error messages?	That would mean we could use
localized DTDs, which are a little easier to work with than string bundles.

>+    alert(msg);

:)

>+  },
>+
>+  cmdSuggestXPath: function()
>+  {
>+    var node = this.selectedNode;
>+    if (node == null) {

if (!node) perhaps.

>+      alert("No node selected.");

:)

>+  // Produces a simple XPath expression containing a list of tags.
>+  // XXX todo: design clever heuristics that make the XPath expressions
>+  // unambiguous, by incorporating a minimal number of relevant element
>+  // attributes.

Great!	We need more documentation in our source code.	I would personally
prefer /*...*/ commenting, but this is conformant with the file's coding style,
so that's fine.

>+  XPathHeuristic: function(node) {
>+    var xpath = null;
>+    while(node.nodeType != Node.DOCUMENT_NODE) {

nsIDOMNode again

>+	xpath = segment+"/"+xpath;

Space that out a little, please.  Other nits on the rest of the patch I've
already mentioned above.

Comment 4

[Alan Donovan]

Hi Alex, thanks for the very prompt and careful scrutiny of my code!  It's my
first foray into both Mozilla and Javascript, which might explain some of the
style issues.  I'll try to fix things up a little today and resubmit a patch.  
Also, I'll RTFM and use -u8pN next time...  sorry.

Q. where you suggest "dump() or throw an exception, please": this is not an
internal error, but a user-input error, so I believe an alert is appropriate. 
Perhaps I missed your point?

alan

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

note that chrome code should not use alert() but use the prompt service
(nsIPromptService), which allows specifying a title for the alert. localizable
texts would also be nice...

Comment 6

[Alex Vincent [:WeirdAl]]

(In reply to comment #4)
> Hi Alex, thanks for the very prompt and careful scrutiny of my code!  It's my
> first foray into both Mozilla and Javascript, which might explain some of the
> style issues.  I'll try to fix things up a little today and resubmit a patch.  
> Also, I'll RTFM and use -u8pN next time...  sorry.

No biggie.  caillon, timeless and bz are the real reviewers of DOM Inspector
code.  I'm not an official reviewer unless caillon (module owner) says so, and
there are probably a few things I missed.  :)

> Q. where you suggest "dump() or throw an exception, please": this is not an
> internal error, but a user-input error, so I believe an alert is appropriate. 
> Perhaps I missed your point?

You did, partially.  We don't want to do alerts in DOM Inspector code, ever.  In
fact, we have at least one bug on file to get rid of prompt calls.

There needs to be some other way of notifying the user that they made a mistake,
or otherwise we need to disable the command element as I suggested earlier, to
make sure the user can't call the command in the first place.

If we're going to notify the user of their mistake, there needs to be a more
friendly approach to it than an alert.  Hence my comment:

"caillon: we should really think about this.  Maybe a XUL section in DOM
Inspector's overall window for error messages?	That would mean we could use
localized DTDs, which are a little easier to work with than string bundles."

To date, Inspector's never needed a section for notifying about user errors.  If
I were to do it using current design, I'd add a <xul:description/> below the two
panels, and tie it into the inspector.xml binding, so that any panel or the main
window could set a message for the user.

If you must notify the user of their mistake(s), then you should probably file a
bug asking for a general notification widget that can be used by any part of DOM
Inspector.  I'd come up with a quick patch (with docs), you'd mark this bug as
blocked by that bug, and then we get someone to review that patch first.

I wouldn't advise writing your patch to depend on another patch that hasn't
gotten r+sr, though... ;) An r- or sr- means we have to either change the patch
(and thus the API) or scrap it altogether.

Oh, and welcome aboard.  :D

Comment 7

[Shawn Wilsher :sdwilsh]

We have a patch that just needs fixing up.  It'd be really nice to get this in for 1.9

Comment 8

[AndrewM]

This shows up on the search for "Unassigned M9 Blockers" (http://tinyurl.com/2unjnx), but it is not marked blocking1.9. Should it be?

Comment 9

[Shawn Wilsher :sdwilsh]

No - this is hardly a blocker.

Comment 10

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

I'm limiting the scope of this bug, since we have three open bugs on file for these features.  For anybody who comes along and wants to work on part 2, do so as part of bug 425264.

Comment 12

[Asa Dotzler [:asa]]

I cannot see any reason why release drivers would track this issue for Firefox 7. It's an ancient bug and I have no idea what this nomination hopes to accomplish so I'm minusing absent any compelling argument for release tracking.

Comment 13

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

Whoa, sorry, no idea what happened there.  The only thing I can think of is that when changing the summary, an errant form control got focused and hilarity ensued.

Comment 14

[Firefox Bug Husbandry Bot]

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

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

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