GCLI output should not be forced to be xhtml

RESOLVED FIXED in Firefox 18

Status

()

Firefox
Developer Tools: Console
P3
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwalker, Assigned: saran)

Tracking

unspecified
Firefox 18
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=jwalker])

Attachments

(1 attachment, 2 obsolete attachments)

We're in an iframe, so we can use html without messing up xul
Triage: Filter on the TRIAGE keyword.
Target Milestone: Firefox 17 → Future
Triage
Priority: P2 → P3
Whiteboard: [good first bug][mentor=jwalker]
(Assignee)

Comment 3

5 years ago
hi Joe...I would like to help out with this bug. Could you please guide on how to proceed ? Thanks.
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
(Assignee)

Comment 5

5 years ago
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 ?
If you create a command that does that following:

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

Then I think you'll see the error.
(Assignee)

Comment 7

5 years ago
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 .
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.
(Assignee)

Comment 9

5 years ago
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
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?
(Assignee)

Comment 11

5 years ago
(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; ?
(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 ... }
(Assignee)

Comment 13

5 years ago
thanks for the clarification.After making that modification , how do I test the changes ?
(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.
(Assignee)

Comment 15

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

This time no error...
(Assignee)

Comment 16

5 years ago
Created attachment 666991 [details] [diff] [review]
Flipped the if else part as mentioned
Attachment #666991 - Flags: review?(jwalker)
Assignee: nobody → ksk.3393
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.
Attachment #666991 - Flags: review?(jwalker)
(Assignee)

Comment 18

5 years ago
Created attachment 667062 [details] [diff] [review]
Patch with correct formatting
Attachment #667062 - Flags: review?(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.
Attachment #667062 - Flags: review?(jwalker) → review+
Attachment #666991 - Attachment is obsolete: true
Created attachment 667910 [details] [diff] [review]
v3

This patch properly corrects the formatting, and adds the required attribution headers.
Attachment #667062 - Attachment is obsolete: true
Green on try:
https://tbpl.mozilla.org/?tree=Try&rev=694fce8f252d

Landing in fx-team:
https://tbpl.mozilla.org/?tree=Fx-Team&rev=e5d8ddb1e35f
Whiteboard: [good first bug][mentor=jwalker] → [fixed-in-fx-team][good first bug][mentor=jwalker]
https://hg.mozilla.org/mozilla-central/rev/0c267a2d5f66
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][good first bug][mentor=jwalker] → [good first bug][mentor=jwalker]
Target Milestone: Future → Firefox 18
You need to log in before you can comment on or make changes to this bug.