Last Comment Bug 72494 - DOM Inspector needs save option
: DOM Inspector needs save option
Status: RESOLVED FIXED
:
Product: Other Applications
Classification: Client Software
Component: DOM Inspector (show other bugs)
: Trunk
: All All
: P4 enhancement (vote)
: Future
Assigned To: Shawn Wilsher :sdwilsh
:
Mentors:
: 141341 (view as bug list)
Depends on: 155749
Blocks:
  Show dependency treegraph
 
Reported: 2001-03-19 10:19 PST by karnaze (gone)
Modified: 2007-07-06 00:41 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1.0 (8.95 KB, patch)
2006-09-21 11:02 PDT, Shawn Wilsher :sdwilsh
timeless: review-
Details | Diff | Splinter Review
v1.1 (6.96 KB, patch)
2006-10-26 14:30 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v2.0 (7.29 KB, patch)
2006-11-15 22:51 PST, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v2.1 (5.74 KB, patch)
2006-11-16 18:35 PST, Shawn Wilsher :sdwilsh
sdwilsh: review-
Details | Diff | Splinter Review
v2.2 (9.32 KB, patch)
2007-06-10 00:25 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v2.3 (9.88 KB, patch)
2007-06-10 12:25 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v2.4 (11.14 KB, patch)
2007-06-13 11:19 PDT, Shawn Wilsher :sdwilsh
timeless: review+
neil: superreview+
Details | Diff | Splinter Review

Description karnaze (gone) 2001-03-19 10:19:45 PST
The DOM Inspector would be a very useful tool for developing reduced test cases 
if it had a file/save option. I was able to delete dom objects in the upper 
pane and see the results in the lower pane, but couldn't save the resulting 
markup.
Comment 1 Alex Vincent [:WeirdAl] 2002-11-06 18:45:25 PST
Sounds like it's possible, given the DOM Level 3 proposed methods for load/save 
that Mozilla implements.
Comment 2 timeless 2002-11-07 05:14:58 PST
you could use dom inspector's copy as xml feature and write that to a file
Comment 3 Christopher Aillon (sabbatical, not receiving bugmail) 2002-11-11 01:40:18 PST
*** Bug 141341 has been marked as a duplicate of this bug. ***
Comment 4 Christopher Aillon (sabbatical, not receiving bugmail) 2003-09-05 03:22:11 PDT
Mass re-assigning bugs to dom.inspector@extensions.bugs
Comment 5 Sylvain Viart 2004-07-25 20:58:20 PDT
The save option would be great to save modified stylesheet too.

This feature could be linked to Bug 121182 (copy form DOM inspector).
Comment 6 Jose Pina Coelho 2005-10-05 08:10:21 PDT
A save feature would also be essential to be able to document problems with
extensions.  The ammount of data in there is just too big to report by hand.
Comment 7 Shawn Wilsher :sdwilsh 2006-07-09 08:51:52 PDT
Could this be accomplished with the nsIDOMSerializer (http://www.xulplanet.com/references/xpcomref/ifaces/nsIDOMSerializer.html)?

If it is that simple (well, relatively), I'd be happy to take this bug.
Comment 8 Shawn Wilsher :sdwilsh 2006-07-20 06:45:34 PDT
(In reply to comment #2)
> you could use dom inspector's copy as xml feature and write that to a file
> 

I've been playing with it, and it doesn't get the DOCTYPE because it doesn't actually copy anything if you select the #document node, but I imagine it uses XMLSerializer, so I think this is very doable.  Do we want this option under File->Save, or what?
Comment 9 Shawn Wilsher :sdwilsh 2006-09-21 11:02:16 PDT
Created attachment 239532 [details] [diff] [review]
v1.0

I think this will do, but I'm pretty sure it's not yet up to par.  Timeless, I'd like some feedback if you could.
Comment 10 Alex Vincent [:WeirdAl] 2006-09-21 11:17:20 PDT
Comment on attachment 239532 [details] [diff] [review]
v1.0

>Index: resources/content/inspector.js
>+      var serializer = Components.classes[kSerializerCID]
>+                                 .createInstance(nsIDOMSerializer);
>+      serializer.serializeToStream(this.mDocPanel.subject, foStream, null);

What are you going to do for HTML documents which (with the exception of XHTML) aren't XML?  Does this output a different format?
Comment 11 Shawn Wilsher :sdwilsh 2006-09-21 11:31:57 PDT
(In reply to comment #10)
> (From update of attachment 239532 [details] [diff] [review] [edit])
> >Index: resources/content/inspector.js
> >+      var serializer = Components.classes[kSerializerCID]
> >+                                 .createInstance(nsIDOMSerializer);
> >+      serializer.serializeToStream(this.mDocPanel.subject, foStream, null);
> 
> What are you going to do for HTML documents which (with the exception of XHTML)
> aren't XML?  Does this output a different format?

Well, I think that it would be a valid DOM Document because it would have to go through the parser in the browser.  Currently, my test page has been http://www.mozilla.org/projects/minefield/, which has a content type of text/html, so it seems to work.  Have any pages in particular you'd like me to test?
Comment 12 timeless 2006-10-24 07:08:27 PDT
Comment on attachment 239532 [details] [diff] [review]
v1.0

tabs! bad!
please name your constants and don't mess up your indentation.

I'm not fond of depending on view source, what if some silly package decides not to have view source (or worse, just puts it in some other palce)?
Comment 13 Shawn Wilsher :sdwilsh 2006-10-24 07:13:32 PDT
(In reply to comment #12)
> (From update of attachment 239532 [details] [diff] [review] [edit])
> tabs! bad!
> please name your constants and don't mess up your indentation.

I'm not sure how those tabs got in there seeing how my editor is supposed to convert tabs to spaces...

> I'm not fond of depending on view source, what if some silly package decides
> not to have view source (or worse, just puts it in some other palce)?

That entity should be somewhere more mainstream then - I'll try to grab it from somewhere else (I'm trying to not add another string to any of our DTD's)
Comment 14 Shawn Wilsher :sdwilsh 2006-10-26 14:30:48 PDT
Created attachment 243707 [details] [diff] [review]
v1.1

Ok, so it looks like I will have to add some strings to one of the dtd files and a properties file :(

This covers everything in your previous comments (although I'm not sure what you meant by "name your constants") and things in our irc conversation a few weeks ago.

Specifically, this should work on a |data:text/html,<br>| by returning |<html><head></head><body><br /></body></html>|

I'm not so sure this is "done" though, so looking for feedback.
Comment 15 Christian :Biesinger (don't email me, ping me on IRC) 2006-10-27 04:49:23 PDT
Comment on attachment 243707 [details] [diff] [review]
v1.1

+        // HTML Document, so lets make it more like HTML and less like XML
+        doc = doc.replace(/<(script|body|head)(.*?)\/>/i, "<$1$2></$1>");
+        doc = doc.replace(/\/>/g, " />");

ew... why not use nsIDocumentEncoder instead?
Comment 16 Shawn Wilsher :sdwilsh 2006-10-27 07:59:22 PDT
Comment on attachment 243707 [details] [diff] [review]
v1.1

(In reply to comment #15)
> ew... why not use nsIDocumentEncoder instead?

Mostly because I didn't know it existed.  Looking at it now (didn't find anything on XULPlanet though - had to go to lxr), it seems like the right thing to use!
Comment 17 Shawn Wilsher :sdwilsh 2006-11-15 08:52:10 PST
(In reply to comment #15)
> (From update of attachment 243707 [details] [diff] [review] [edit])
> +        // HTML Document, so lets make it more like HTML and less like XML
> +        doc = doc.replace(/<(script|body|head)(.*?)\/>/i, "<$1$2></$1>");
> +        doc = doc.replace(/\/>/g, " />");
> 
> ew... why not use nsIDocumentEncoder instead?
> 

So, I've been having issues with this.  I think that using nsIDocumentEncoder only for text/html and using XMLSerializer for everything else is a good idea.
Comment 18 Shawn Wilsher :sdwilsh 2006-11-15 22:51:23 PST
Created attachment 245739 [details] [diff] [review]
v2.0

Ok, this uses the document encoder for text/html documents, and the serializer for everything else.  I'm having an issue that I don't know why I'm getting though.  When trying out this document:
http://www.mozilla.org/projects/minefield/
I will get this error when trying to save:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80500001 [nsIDocumentEncoder.encodeToStream]"  nsresult: "0x80500001 (<unknown>)"  location: "JS frame :: chrome://inspector/content/inspector.js :: save :: line 366"  data: no]

As far as I can tell, I'm passing it the document.  Any suggestions?
Comment 19 Shawn Wilsher :sdwilsh 2006-11-16 18:35:04 PST
Created attachment 245809 [details] [diff] [review]
v2.1

This is everything short of adding the entities to the locale.  Thanks for the help on irc timeless.
Comment 20 Shawn Wilsher :sdwilsh 2006-11-30 07:07:26 PST
Comment on attachment 245809 [details] [diff] [review]
v2.1

r- from timeless on irc
Comment 21 Christian :Biesinger (don't email me, ping me on IRC) 2007-03-16 05:54:14 PDT
(In reply to comment #17)
> So, I've been having issues with this.  I think that using nsIDocumentEncoder
> only for text/html and using XMLSerializer for everything else is a good idea.

What were the issues there?
Comment 22 Shawn Wilsher :sdwilsh 2007-03-16 06:02:51 PDT
(In reply to comment #21)
> (In reply to comment #17)
> > So, I've been having issues with this.  I think that using nsIDocumentEncoder
> > only for text/html and using XMLSerializer for everything else is a good idea.
> 
> What were the issues there?

This bug fell off my radar :(

I think I actually worked through the issues, I just needed to fix what timeless had discussed with me on irc.  (Now recalling that is going to be the fun part).  Maybe I'll get a patch up this weekend since this was one of the things I wanted to make for Firefox 3
Comment 23 Shawn Wilsher :sdwilsh 2007-06-10 00:25:53 PDT
Created attachment 267844 [details] [diff] [review]
v2.2

yey!  A patch!
Comment 24 neil@parkwaycc.co.uk 2007-06-10 09:26:48 PDT
Comment on attachment 267844 [details] [diff] [review]
v2.2

>+    picker.appendFilters(picker.filterHTML | picker.filterXML |
>+                         picker.filterXUL);
I'm wondering whether we should onlly show HTML or XUL as the default where appropriate, plus append XML as a fallback or for other MIME types.

>+    var file = picker.file;
Hardly worth a separate variable.

>+		var fos = Components.classes[kFOStreamCID]
>+                        .createInstance(nsIFileOutputStream);
Nit: tabs

>+    fos.init(file, 0x02 | 0x08 | 0x20, 0664, 0); // write, create, truncate
Do we want to do this before we create the encoder/serializer?
I was surprised to see 0664 documented as the default mode, as when I studied C 20 years ago we were taught to use 0666 and let the umask do the rest.

>+      // first we try to use the document encoder for that content type.  If
>+      // that fails, we move on to the xml serializer.

>+        // Someone can build without the serializer, so we have to see if we
>+        // can actually get it or not (just like copyXML function, but here we
>+        // do not want to fake it).
How can these fail? If it's only due to the lack of the component, then do an in check for the component, and allow real exceptions to be exceptional.
Comment 25 Shawn Wilsher :sdwilsh 2007-06-10 12:03:50 PDT
(In reply to comment #24)
> I'm wondering whether we should onlly show HTML or XUL as the default where
> appropriate, plus append XML as a fallback or for other MIME types.

I know there is a group that uses the DOMi to inspect XML files (myself included), so I think we should leave it in there.

> >+    var file = picker.file;
> Hardly worth a separate variable.

If was for line wrapping, so I've now but the flags in their own variable.

> Do we want to do this before we create the encoder/serializer?

I've refactored that a bit and I think you'll be happier with it the way it is.

> I was surprised to see 0664 documented as the default mode, as when I studied C
> 20 years ago we were taught to use 0666 and let the umask do the rest.

Do you want me to use 0666 instead?  I was just following the docs...

New patch shortly
Comment 26 Shawn Wilsher :sdwilsh 2007-06-10 12:25:19 PDT
Created attachment 267886 [details] [diff] [review]
v2.3

Addresses comments
Comment 27 neil@parkwaycc.co.uk 2007-06-11 01:36:51 PDT
Comment on attachment 267886 [details] [diff] [review]
v2.3

>+    const flags = 0x02 | 0x08 | 0x20; // write, create, truncate
Nit: you can use -1 for the same effect, which saves tying.

>+      fos.init(picker.file, flags, 0664, 0); 
Please change this to 0666. (Perhaps the idl needs updating?)

>+      } catch (e) {
>+        // Let's let people know that there is an error.  We do this in a try
>+        // catch block because we still want to close the file stream.
>+        Components.utils.reportError(e);
Nit: removing the catch block will achieve this; if an exception is thrown then the finally block will get executed, and assuming the close succeeds the exception will then get logged on the Error Console in the normal way.

What do you do if there's no encoder or serializer?
Comment 28 Shawn Wilsher :sdwilsh 2007-06-11 09:20:33 PDT
(In reply to comment #27)
> What do you do if there's no encoder or serializer?

Fail miserably, of course.  I'm open to suggestions, but I'm not really sure of what else to do.
Comment 29 neil@parkwaycc.co.uk 2007-06-11 16:16:24 PDT
Is it feasible to disable the save option if there's no encoder or serializer?
Comment 30 Shawn Wilsher :sdwilsh 2007-06-13 11:19:25 PDT
Created attachment 268253 [details] [diff] [review]
v2.4

> Nit: you can use -1 for the same effect, which saves tying.
While it has the same affect, I think the code is easier to understand the way it is now, so I'd prefer to keep it as is if possible.

This disables the command if it cannot actually save the document.
Comment 31 neil@parkwaycc.co.uk 2007-06-13 13:44:46 PDT
Comment on attachment 268253 [details] [diff] [review]
v2.4

I guess this is OK, but I'm surprised there isn't some other way to update global commands.
Comment 32 Shawn Wilsher :sdwilsh 2007-06-13 17:13:31 PDT
Checking in extensions/inspector/resources/content/commandOverlay.xul;
new revision: 1.10; previous revision: 1.9
Checking in extensions/inspector/resources/content/inspector.js;
new revision: 1.36; previous revision: 1.35
Checking in extensions/inspector/resources/content/popupOverlay.xul;
new revision: 1.18; previous revision: 1.17
Checking in extensions/inspector/resources/content/inspector.xul;
new revision: 1.17; previous revision: 1.16
Checking in extensions/inspector/resources/locale/en-US/inspector.dtd;
new revision: 1.10; previous revision: 1.9
Checking in extensions/inspector/resources/locale/pl/inspector.dtd;
new revision: 1.5; previous revision: 1.4

Note You need to log in before you can comment on or make changes to this bug.