Last Comment Bug 768399 - GCLI output should not be forced to be xhtml
: GCLI output should not be forced to be xhtml
Status: RESOLVED FIXED
[good first bug][mentor=jwalker]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: unspecified
: All All
: P3 normal (vote)
: Firefox 18
Assigned To: saran
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-26 04:37 PDT by Joe Walker [:jwalker] (needinfo me or ping on irc)
Modified: 2012-10-08 07:39 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Flipped the if else part as mentioned (2.00 KB, patch)
2012-10-02 07:49 PDT, saran
no flags Details | Diff | Review
Patch with correct formatting (1.58 KB, patch)
2012-10-02 11:01 PDT, saran
jwalker: review+
Details | Diff | Review
v3 (811 bytes, patch)
2012-10-04 05:14 PDT, Joe Walker [:jwalker] (needinfo me or ping on irc)
no flags Details | Diff | Review

Description Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-06-26 04:37:56 PDT
We're in an iframe, so we can use html without messing up xul
Comment 1 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-08-27 05:11:31 PDT
Triage: Filter on the TRIAGE keyword.
Comment 2 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-08-28 02:28:48 PDT
Triage
Comment 3 saran 2012-09-05 15:51:21 PDT
hi Joe...I would like to help out with this bug. Could you please guide on how to proceed ? Thanks.
Comment 4 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-09-06 07:27:46 PDT
To diagnose, create a command [1] that outputs invalid XHTML, see where the error is, I suspect util.setContents() [2]
We'll probably need to alter the source document to be HTML5 rather than XHTML5 [3] in addition to fixing setContents() (if that is the source of the errors)

[1] See https://developer.mozilla.org/en-US/docs/Tools/GCLI#Extending_the_Command_Line
[2] https://github.com/joewalker/gcli/blob/master/lib/gcli/util.js#L346
[3] https://hg.mozilla.org/integration/fx-team/file/tip/browser/devtools/commandline/commandlineoutput.xhtml
Comment 5 saran 2012-09-30 21:32:23 PDT
Hi joe.. while creating the command for [1] , in the exec() I am not sure how to access the XHTML . Could you help me with that ?
Comment 6 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-10-01 04:47:35 PDT
If you create a command that does that following:

  exec: function() {
    return "<p>First<p>Second";
  }

Then I think you'll see the error.
Comment 7 saran 2012-10-01 06:01:01 PDT
This was the command defn: 
Components.utils.import("resource:///modules/devtools/gcli.jsm");

gcli.addCommand({
  name: 'invalidtest',
  exec: function() {

    return "<p>First<p>Second";

  }
});
When i run it i get the following exception.


[Exception... "An invalid or illegal string was specified"  code: "12" nsresult: "0x8053000c (SyntaxError)"  location: "resource:///modules/devtools/gcli.jsm Line: 3018"]

When I looked at http://mxr.mozilla.org/comm-central/source/mozilla/browser/devtools/commandline/gcli.jsm 
I couldnt exactly find out what the error was due to .
Comment 8 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-10-01 08:53:54 PDT
That link is to comm-central, a better link is http://mxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/gcli.jsm#3018

However that's still not enough to pinpoint the line of the error. One of 2 things needs to happen. I guess you're not using Nightly? So the source is likely to be out of date. You can solve this by hunting around in MXR for the matching version of that file, or by repeating the experiment using Nightly. I recommend the latter.
Comment 9 saran 2012-10-01 10:55:36 PDT
Yes .Thanks for pointing out. I was using FF 16 beta.. When I tried it in Nightly , this is what I got :
[Exception... "An invalid or illegal string was specified"  code: "12" nsresult: "0x8053000c (SyntaxError)"  location: "resource:///modules/devtools/gcli.jsm Line: 3068"]

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/gcli.jsm#3068
Comment 10 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-10-01 11:16:55 PDT
So the contextual fragment thing isn't going to work with html I guess. It certainly makes sense to use innerHtml if it's available rather than as a default. We'd probably need to test for innerHtml using ('innerHtml' in elem) to avoid calling a potentially expensive getter.

Or we could look at the clause at the top of the function, perhaps that can be persuaded to work?
Comment 11 saran 2012-10-02 01:40:36 PDT
(In reply to Joe Walker [:jwalker] from comment #10)
> So the contextual fragment thing isn't going to work with html I guess. It
> certainly makes sense to use innerHtml if it's available rather than as a
> default. We'd probably need to test for innerHtml using ('innerHtml' in
> elem) to avoid calling a potentially expensive getter.
> 
> Or we could look at the clause at the top of the function, perhaps that can
> be persuaded to work?
I am sorry Joe , but I don't follow you on this . Are you suggesting that when in xml mode we do something else instead of var child = range.createContextualFragment(contents).firstChild; ?
Comment 12 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-10-02 02:28:16 PDT
(In reply to saran from comment #11)
> (In reply to Joe Walker [:jwalker] from comment #10)
> > So the contextual fragment thing isn't going to work with html I guess. It
> > certainly makes sense to use innerHtml if it's available rather than as a
> > default. We'd probably need to test for innerHtml using ('innerHtml' in
> > elem) to avoid calling a potentially expensive getter.
> > 
> > Or we could look at the clause at the top of the function, perhaps that can
> > be persuaded to work?
> I am sorry Joe , but I don't follow you on this . Are you suggesting that
> when in xml mode we do something else instead of var child =
> range.createContextualFragment(contents).firstChild; ?

I'm suggesting trying flipping the sense of the if statement so we try to use innerHtml before trying the createContextualFragment route.

i.e. Not:

if (xmlMode) { ... createContextualFragment ... }
else { ... innerHtml ... }

But instead:

if ("innerHtml" in elem) { ... innerHtml ... }
else { ... createContextualFragment ... }
Comment 13 saran 2012-10-02 02:52:42 PDT
thanks for the clarification.After making that modification , how do I test the changes ?
Comment 14 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-10-02 03:18:21 PDT
(In reply to saran from comment #13)
> thanks for the clarification.After making that modification , how do I test
> the changes ?

Obviously your command is a good place to start. Does that work?

The next step would be to run the test suite to make we've not broken anything.
Comment 15 saran 2012-10-02 07:28:16 PDT
Hi Joe... I tried the command now... I get :
First
Second

This time no error...
Comment 16 saran 2012-10-02 07:49:55 PDT
Created attachment 666991 [details] [diff] [review]
Flipped the if else part as mentioned
Comment 17 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-10-02 10:38:13 PDT
Comment on attachment 666991 [details] [diff] [review]
Flipped the if else part as mentioned

Review of attachment 666991 [details] [diff] [review]:
-----------------------------------------------------------------

Please could you do some simple formatting before we commit this? We generally use 2 space tabs, and try to avoid trailing spaces?
Thanks.
Comment 18 saran 2012-10-02 11:01:40 PDT
Created attachment 667062 [details] [diff] [review]
Patch with correct formatting
Comment 19 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-10-02 11:10:21 PDT
Comment on attachment 667062 [details] [diff] [review]
Patch with correct formatting

Review of attachment 667062 [details] [diff] [review]:
-----------------------------------------------------------------

There are still several spaces at the ends of lines, which I can remove as I integrate this if you'd like.

Most editors have some way to display spaces at the ends of lines, and using them to remove trailing spaces helps people to focus on the actual changes to a file to be able to ignore changes in whitespace.

Thanks for the patch.
Comment 20 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-10-04 05:14:03 PDT
Created attachment 667910 [details] [diff] [review]
v3

This patch properly corrects the formatting, and adds the required attribution headers.
Comment 21 Joe Walker [:jwalker] (needinfo me or ping on irc) 2012-10-05 09:51:51 PDT
Green on try:
https://tbpl.mozilla.org/?tree=Try&rev=694fce8f252d

Landing in fx-team:
https://tbpl.mozilla.org/?tree=Fx-Team&rev=e5d8ddb1e35f
Comment 22 Panos Astithas [:past] 2012-10-08 07:39:21 PDT
https://hg.mozilla.org/mozilla-central/rev/0c267a2d5f66

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