Closed Bug 1250104 Opened 8 years ago Closed 8 years ago

Import the jsesc library

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: alexis+bugs, Assigned: alexis+bugs)

References

Details

Attachments

(1 file, 8 obsolete files)

Attached patch patch.diff (obsolete) — Splinter Review
jsesc is "A JavaScript library for escaping JavaScript strings while generating the shortest possible valid ASCII-only output.".

This library will be useful to generate a canonical JSON representation of JavaScript objects, that will be used to generate and validate signatures for OneCRL.

I've added this in the "General" component because I'm not sure where it should actually go. Per a discussion with :rnewman, it seems that we would like to create a new "vendor/thirdparty" folder.

As I'm new to m-c dev, I have a few questions:

#1 - Should I create a new top-level directory for this?
#2 - If the answer to #1 is yes, how should this new directory be included in the build process?

I've attached a patch with the library added to `services/common`. I do realize this isn't where we want this to leave, but that would probably let folks have a gist of what's going on here.
Attachment #8721949 - Flags: feedback?(rnewman)
Assignee: nobody → alexis+bugs
Here's the last thread I remember about using third-party libraries.

https://mail.mozilla.org/pipermail/firefox-dev/2015-December/003697.html

It didn't reach an actionable consensus.

Dave, build folks: do you already have something in mind for where to put snapshots of third-party (or first-party but GitHub-canonical) libraries, particularly JS libraries?
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(gps)
Flags: needinfo?(dtownsend)
Blocks: 1250191
Comment on attachment 8721949 [details] [diff] [review]
patch.diff

The code looks fine (modulo being 'webby' JS). We'd need to have some kind of documentation near this that indicates where it comes from, who owns updating it, what to do if there's a bug, etc. -- and probably some tests.
Attachment #8721949 - Flags: feedback?(rnewman) → feedback+
Attached file fx-header (obsolete) —
The way I'm importing this code currently is with a `cat fx-header jsesc.js > jsesc-fx.js`. I attached the `fx-header` to this bug. I just added there some information about how to update it, where it leaves etc.

As for tests, there are tests running in there (https://github.com/mathiasbynens/jsesc/blob/master/tests/tests.js), but I'm not sure how I should integrate them with our tests. Should I take a subset of them and make them run with xpcshell?
Attachment #8722047 - Flags: feedback?(rnewman)
There is no convention on where to put 3rd party JS projects. Various groups have imported code into their own directories, which I see is the approach in this patch. A risk with that is we'll have multiple copies or versions of the same package in the tree and in the distribution.

I would recommend toolkit/modules/third_party or browser/modules/third_party as where to put these packages.
Flags: needinfo?(gps)
Flags: needinfo?(mh+mozilla)
As this looks to be for services I would put something like services/third_party/jsesc/jsesc.js and also add a README alongside explaining what the current version is, where it came from and what the process is for updating it from the source. You may want to add services/third_party to the top-level .eslintignore file.
Flags: needinfo?(dtownsend)
Attached patch jsesc.diff (obsolete) — Splinter Review
Here is the new patch with your comments addressed.

I still need to add some testing, but as I'm unclear as to what should be tested here (it's already tested upstream), I haven't done anything here.

Let me know if I can do anything to improve this patch so it can land :)
Attachment #8721949 - Attachment is obsolete: true
Attachment #8722047 - Attachment is obsolete: true
Attachment #8722047 - Flags: feedback?(rnewman)
Attachment #8723008 - Flags: review?(rnewman)
Comment on attachment 8723008 [details] [diff] [review]
jsesc.diff

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

This seems fine to me for now.

Please set the commit message for this to:

  Bug 1250104 - Import the "jesec" library for escaping JavaScript strings. r=rnewman
Attachment #8723008 - Flags: review?(rnewman) → review+
Attachment #8723512 - Attachment is obsolete: true
Attachment #8723538 - Attachment is obsolete: true
Attachment #8723008 - Attachment is obsolete: true
Attachment #8723543 - Attachment is obsolete: true
Updated with the .eslintignore.
Keywords: checkin-needed
I'll note that these failures did show up in the try push.
Thanks for the feedback, and sorry that you had to back this out.

I don't have any windows machine to debug this. Anyone could take a look (or point me to someone who can)?
Flags: needinfo?(alexis+bugs) → needinfo?(wkocher)
You can probably do most of it just pushing to try and requesting windows xpcshell tests, but you could also request a loaner: https://wiki.mozilla.org/ReleaseEngineering/How_To/Request_a_slave
Flags: needinfo?(wkocher)
Attachment #8735945 - Attachment is obsolete: true
I believe the failing test in threeherder (https://treeherder.mozilla.org/#/jobs?repo=try&revision=39d9d4ed8ca8&selectedJob=18749081) is not related to the changes I'm making here.
Keywords: checkin-needed
Comment on attachment 8723622 [details] [diff] [review]
Import the "jsesc" library for escaping JavaScript strings

(Appears the other patch here is what landed, obsoleting this one.)
Attachment #8723622 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/089d27ec5d7c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.