956567 - [prettify] Pretty print in the debugger produces invalid js

Status: VERIFIED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Victor Porof [:vporof][:vp]]

STR:

1. Go to http://todomvc.com/labs/dependency-examples/enyo_backbone/
2. Open debugger
3. Make sure app.js is selected and pretty printed

Line 4144 looks like this:

h.innerHTML = '  <link/><table></table><a href='/a'>a</a><input type='checkbox'/>',

Let me break that down for you:

foo.bar = "str" /a "str" checkbox "str"

This makes Parser.jsm sad when searching for functions and doing other smartsy stuff. It's also invalid javascript, but that's a technicality.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Victor Porof [:vp] from comment #0)
> It's also invalid javascript, but that's a technicality.

lol

Comment 2

[Brion Vibber]

This looks like the same as bug 964103.

Comment 4

[Brion Vibber]

Attachment: prettyprint-singlequote-escape.patch

Patch to escape single quotes in strings in the debugger's pretty printer. Resolves round-tripping issues with strings like 'Can\'t' or "Can't"; both now turn into correct 'Can'\t' instead of broken 'Can't'.

Comment 5

[Brion Vibber]

Comment on attachment 8374048 [details] [diff] [review]
prettyprint-singlequote-escape.patch

Please feel free to bump review around, I'm a bit unclear on the protocol here. :) Thanks!

Comment 6

[Brion Vibber]

And here's a pull req on the separate pretty-fast repo on github: https://github.com/mozilla/pretty-fast/pull/3

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Copying comment from github for posterity:

"""
Thanks for taking this on Brion.

1. Tests are failing. At a glance it looks like pretty fast is giving the correct output but the test needs to be updated. Make sure that the tests are passing (via npm test on the command line) before updating this pull request.

2. We need a test that specifically tests that we escape single quotes properly. I think actually updating the test failing above would be perfect. Embarrassingly enough, I think that test is asserting the incorrect behavior that your patch is fixing! Woops!
"""

Comment 8

[Brion Vibber]

Attachment: prettyprint-singlequote-escape2.patch

Updated patch which also updates the test case for the fix.

Added a second commit to pull req version with this fix as well: https://github.com/mozilla/pretty-fast/pull/3

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8374801 [details] [diff] [review]
prettyprint-singlequote-escape2.patch

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

Thanks!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/b7240f73df3f

Brion, please make sure that future patches follow the guidelines below. Makes life easier for those landing on your behalf :)
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 11

[Brion Vibber]

Thanks! *bookmarked & .hgrc edited*

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b7240f73df3f