Closed
Bug 1250104
Opened 8 years ago
Closed 8 years ago
Import the jsesc library
Categories
(Toolkit :: General, defect)
Toolkit
General
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: alexis+bugs, Assigned: alexis+bugs)
References
Details
Attachments
(1 file, 8 obsolete files)
13.70 KB,
patch
|
Details | Diff | 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 | ||
Updated•8 years ago
|
Assignee: nobody → alexis+bugs
Comment 1•8 years ago
|
||
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)
Comment 2•8 years ago
|
||
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+
Assignee | ||
Comment 3•8 years ago
|
||
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)
Comment 4•8 years ago
|
||
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)
Updated•8 years ago
|
Flags: needinfo?(mh+mozilla)
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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+
Assignee | ||
Comment 8•8 years ago
|
||
Assignee | ||
Comment 9•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=318e57e49dcd
Assignee | ||
Comment 10•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8723512 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6aad9789363
Assignee | ||
Comment 12•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8723538 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8723008 -
Attachment is obsolete: true
Assignee | ||
Comment 13•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8723543 -
Attachment is obsolete: true
Assignee | ||
Comment 14•8 years ago
|
||
Updated with the .eslintignore.
Assignee | ||
Comment 15•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8250212ac5e8
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 16•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/26bbd4cee816
Keywords: checkin-needed
The test this added is failing like https://treeherder.mozilla.org/logviewer.html#?job_id=7602831&repo=fx-team I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/bab8e722f1aa
Flags: needinfo?(alexis+bugs)
I'll note that these failures did show up in the try push.
Assignee | ||
Comment 19•8 years ago
|
||
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)
Assignee | ||
Comment 21•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f29e9079358
Assignee | ||
Comment 22•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd2d488ff157
Assignee | ||
Comment 23•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a28d9eef04a1
Assignee | ||
Comment 24•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a90a3d9e8e63
Assignee | ||
Comment 25•8 years ago
|
||
Assignee | ||
Comment 26•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8735945 -
Attachment is obsolete: true
Assignee | ||
Comment 27•8 years ago
|
||
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 28•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/089d27ec5d7c
Keywords: checkin-needed
Comment 29•8 years ago
|
||
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
Comment 30•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/089d27ec5d7c
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•