Closed Bug 1378824 Opened 2 years ago Closed 2 years ago

Stop using sdk/util/uuid in DevTools

Categories

(DevTools :: General, enhancement, P1)

enhancement

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.
It used also in:

  devtools/client/inspector/webpack.config.js
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [nosdk]
Assignee: nobody → sole
Status: NEW → ASSIGNED
Priority: P2 → P1
Target Milestone: --- → Firefox 56
Summary: Stop using sdk/util/uuid → Stop using sdk/util/uuid in DevTools
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 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 :-)
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 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+
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
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)
Flags: needinfo?(sole)
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/b39c8141158d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.