768399 - GCLI output should not be forced to be xhtml

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Joe Walker [:jwalker]]

We're in an iframe, so we can use html without messing up xul

Comment 1

[Joe Walker [:jwalker]]

Triage: Filter on the TRIAGE keyword.

Comment 2

[Joe Walker [:jwalker]]

Triage

Comment 3

[saran]

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]]

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]

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]]

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]

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]]

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]

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]]

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]

(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]]

(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]

thanks for the clarification.After making that modification , how do I test the changes ?

Comment 14

[Joe Walker [:jwalker]]

(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]

Hi Joe... I tried the command now... I get :
First
Second

This time no error...

Comment 16

[saran]

Attachment: Flipped the if else part as mentioned

Comment 17

[Joe Walker [:jwalker]]

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]

Attachment: Patch with correct formatting

Comment 19

[Joe Walker [:jwalker]]

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]]

Attachment: v3

This patch properly corrects the formatting, and adds the required attribution headers.

Comment 21

[Joe Walker [:jwalker]]

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 (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/0c267a2d5f66