59 bytes, text/x-review-board-request
In the current TLS status API, `keaGroupName` and `signatureSchemeName` return the string `"none"` if they are unset. `"none"` is kind of a magic word that doesn't really have any technical meaning, and makes it hard to test for truthiness. https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/addons/SecurityInfo.jsm#140 My proposal: If either `keaGroupName` or `signatureSchemeName` are set to `"none"` by `SSLStatus`, set their value to either `null` or the empty string in what is returned by the API. The first option is more semantically correct, the latter keeps the type the same, but both are easier to test for truthiness.
I'm not sure I'm convinced this is an issue. I'm also unsure about testing this value. If I'm checking for "none", and someone changes the platform to something else...in any case will put up a patch.
Since these are possible uplift candidates for 62, can you please provide some STR and the proper webextension(if required)? So that we can get ahead with testing as per when the bug will be fixed.
I'm still debating whether/how to automate a test on this. It is certainly not something easily testable from QA.
Attachment #8989253 - Flags: review?(lgreco)
Comment on attachment 8989253 [details] Bug 1471959 - leave values undefined if value is none, rpl https://reviewboard.mozilla.org/r/254306/#review261912 To be honest I'm not super thrilled by this, it still seems to me that "normalizing 'none' to null/undefined" could be easily done from the extension code, especially given that the implementation is mostly just returning the strings that the internal implementation is giving us and that we don't have any automated test for the property that we are going to "normalize". Having said that, I completely trust Shane judgment and I'll leave to him the final choice if this is a change that we absolutely want to land.
Attachment #8989253 - Flags: review?(lgreco) → review+
Comment on attachment 8989253 [details] Bug 1471959 - leave values undefined if value is none, rpl https://reviewboard.mozilla.org/r/254306/#review261916 ::: commit-message-6a922:1 (Diff revision 1) > +Bug 1471959 - leave values undefined if value is none, r? rpl Nit, let's mention somewhere in the commit message that with "values" we are referring to the `keaGroupName` and `signatureSchemeName` properties that are part of the webRequest API.
Attachment #8989253 - Attachment is obsolete: true
Comment on attachment 8990291 [details] Bug 1471959 - leave keaGroupName and signatureSchemeName undefined if value is none, https://reviewboard.mozilla.org/r/255324/#review262174
Attachment #8990291 - Flags: review?(lgreco) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/14e28f7332c7 leave keaGroupName and signatureSchemeName undefined if value is none, r=rpl
Comment on attachment 8990291 [details] Bug 1471959 - leave keaGroupName and signatureSchemeName undefined if value is none, Approval Request Comment [Feature/Bug causing the regression]: 1322748 [User impact if declined]: api cleanup before api hits release, would only affect extension devs [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: not yet [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: minor fix [String changes made/needed]: none
Attachment #8990291 - Flags: approval-mozilla-beta?
Some way of testing or verifying would be nice before uplift, even for a minor fix. It seems odd to say, we can't test it, and don't have any automated tests, so let's ship it faster.
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment on attachment 8990291 [details] Bug 1471959 - leave keaGroupName and signatureSchemeName undefined if value is none, Well, let's see how this does on beta 8, since this is a new API, maybe extension authors who are commenting in the meta bug 1322748 will be testing with beta.
Attachment #8990291 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox62: affected → fixed
If manual testing is not needed please set the “qe-verify -“ flag, otherwise please provide some STR and extension if needed, in order to validate the bug. Thanks!
Flags: needinfo?(mixedpuppy) → qe-verify-
These properties weren't documented before, I guess because they weren't in the schema. I've added them to https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/webRequest/SecurityInfo, and marked them optional, which implicitly indicates that they are undefined if not specified, which I think is the truth. I just used the description from the schema: I didn't think it was very helpful but wasn't able to come up with anything better, even after a bit of digging. It seems like they are internal names, rather than names from any standard. But I'd be very happy to accept suggestions for improving this. Anyway, please let me know if this covers it or we need anything else.
This works for me, thank you so much everyone. :)
You need to log in before you can comment on or make changes to this bug.