Closed
Bug 1378824
Opened 7 years ago
Closed 7 years ago
Stop using sdk/util/uuid in DevTools
Categories
(DevTools :: General, enhancement, P1)
DevTools
General
Tracking
(firefox56 fixed)
RESOLVED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: zer0, Assigned: sole)
References
Details
(Whiteboard: [nosdk])
Attachments
(1 file)
Used in: devtools/client/shared/redux/middleware/promise.js As part of the work to remove usage of the Addon SDK. Details to follow as we triage.
Reporter | ||
Comment 1•7 years ago
|
||
It used also in: devtools/client/inspector/webpack.config.js
Updated•7 years ago
|
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [nosdk]
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → sole
Updated•7 years ago
|
Status: NEW → ASSIGNED
Priority: P2 → P1
Target Milestone: --- → Firefox 56
Assignee | ||
Updated•7 years ago
|
Summary: Stop using sdk/util/uuid → Stop using sdk/util/uuid in DevTools
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8889419 [details] Bug 1378824 - Stop using sdk/util/uuid in DevTools. https://reviewboard.mozilla.org/r/160458/#review165716 Thanks! I'd like us to clearly separate parsing and generating UUIDs rather than having everything in the same method. ::: devtools/client/inspector/webpack.config.js:108 (Diff revision 1) > "devtools": path.join(__dirname, "../../"), > "gcli": path.join(__dirname, "../../shared/gcli/source/lib/gcli"), > "method": path.join(__dirname, "../../../addon-sdk/source/lib/method"), > "modules/libpref/init/all": > path.join(__dirname, "../../../modules/libpref/init/all.js"), > - "sdk/util/uuid": > + "devtools/shared/uuid": The inspector in launchpad is broken at the moment, but since the exported interface is different between sdk/util/uuid and devtools/shared/uuid, the sham should be updated to have the same API. In devtools/client/inspector/webpack/uuid-sham.js module.exports = { uuid } => module.exports = uuid Can you either fix it here or file a follow up? ::: devtools/shared/uuid.js:17 (Diff revision 1) > + * Returns `uuid`. If `id` is passed then it's parsed to `uuid` and returned > + * if not then new one is generated. > + * > + * Replacement for `uuid` API from "sdk/util/uuid". > + */ > +module.exports = function uuid(id) { We never pass an id to this method, therefore the ability to "parse a uuid" is not used by devtools. Even if we need the parseUUID in the future, I would prefer to have it as a different method rather than having a method with a behavior that varies depending on the signature.
Attachment #8889419 -
Flags: review?(jdescottes)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8889419 [details] Bug 1378824 - Stop using sdk/util/uuid in DevTools. https://reviewboard.mozilla.org/r/160458/#review166702 New version following your insightful comments, Julian! Now everything is way more explicit. Let me know how does it look :-)
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8889419 [details] Bug 1378824 - Stop using sdk/util/uuid in DevTools. https://reviewboard.mozilla.org/r/160458/#review165716 > The inspector in launchpad is broken at the moment, but since the exported interface is different between sdk/util/uuid and devtools/shared/uuid, the sham should be updated to have the same API. > > In devtools/client/inspector/webpack/uuid-sham.js > module.exports = { uuid } => module.exports = uuid > > Can you either fix it here or file a follow up? Ah I hadn't realised that bit! I have made both use the same interface. Let me know if that looks better now :-) > We never pass an id to this method, therefore the ability to "parse a uuid" is not used by devtools. > > Even if we need the parseUUID in the future, I would prefer to have it as a different method rather than having a method with a behavior that varies depending on the signature. This makes total sense! I also had an "hmm" moment when I copied the code. I have turned this into a function that only generates UUIDs.
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8889419 [details] Bug 1378824 - Stop using sdk/util/uuid in DevTools. https://reviewboard.mozilla.org/r/160458/#review166852 Looks good to me, thanks for cleaning this up! For the record we could not inline `const { generateUUID } = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);` in devtools/client/shared/redux/middleware/promise.js because we try to avoid using chrome code in client. ::: devtools/shared/generate-uuid.js:16 (Diff revision 2) > +/* exports.generateUUID = function () { > + return generateUUID(); > +}; > +*/ nit: remove commented out code
Attachment #8889419 -
Flags: review?(jdescottes) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c0f919689e48 Stop using sdk/util/uuid in DevTools. r=jdescottes
Keywords: checkin-needed
Comment 10•7 years ago
|
||
Backed out for failing eslint at devtools/client/inspector/webpack.config.js:166:3 with: Newline required at end of file but not found: https://hg.mozilla.org/integration/autoland/rev/b11ddbf7ee37420ede861eb2dd928624721877d1 Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c0f919689e48052b2a9815e4b756f944bb2a7125&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=118497323&repo=autoland > TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/inspector/webpack.config.js:166:3 | Newline required at end of file but not found. (eol-last)
Flags: needinfo?(sole)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(sole)
Keywords: checkin-needed
Comment 12•7 years ago
|
||
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/b39c8141158d Stop using sdk/util/uuid in DevTools. r=jdescottes
Keywords: checkin-needed
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b39c8141158d
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•