Closed Bug 1116269 Opened 5 years ago Closed 5 years ago

Add WebCrypto to GlobalProperties for import

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: mt, Assigned: mt)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(9 files, 1 obsolete file)

I'm looking to implement bug 975144, there is a good chance that the sandboxed code will need to run some crypto functions.  WebCrypto is nicely self-contained, so this should be straightforward to expose.
Attached file MozReview Request: bz://1116269/mt (obsolete) —
Attachment #8542372 - Flags: review?(rlb)
Attachment #8542372 - Flags: review?(peterv)
/r/1791 - Bug 1116269 - Crypto/SubtleCrypto only depend on nsIGlobalObject
/r/1793 - Bug 1116269 - Add 'crypto' to sandbox global properties

Pull down these commits:

hg pull review -r cde3bd84767b1c25c885fbc02ad88814325b2054
Comment on attachment 8542372 [details]
MozReview Request: bz://1116269/mt

Gabor is probably the best alternative xpconnect reviewer these days.

This looks good at first glance, but I didn't have time to audit things very carefully (especially the crypto-side changes), so bouncing to gabor.
Attachment #8542372 - Flags: review?(peterv) → review?(gkrizsanits)
https://reviewboard.mozilla.org/r/1789/#review1213

::: js/xpconnect/src/Sandbox.cpp
(Diff revision 1)
> +    return JS_DefineProperty(cx, obj, "crypto", wrapped, JSPROP_ENUMERATE);

Please be consistent with naming here. Either call it WebCrypto or crypto everywhere (not sure which is the name of the ctor on a regular DOM window but you should use the same here too)

Patches look great but I'm missing a simple test at least from it. Could you please add one? (I would do an xpcshell test under js/xpconnect)
Comment on attachment 8542372 [details]
MozReview Request: bz://1116269/mt

I've did the review on review board, please flag me back once you added the test and fixed the naming issue I mentioned there.
Attachment #8542372 - Flags: review?(gkrizsanits)
https://reviewboard.mozilla.org/r/1789/#review1215

> Please be consistent with naming here. Either call it WebCrypto or crypto everywhere (not sure which is the name of the ctor on a regular DOM window but you should use the same here too)

The name of the API is WebCrypto; the name of the property on window is "crypto".  I'm trying to ensure that the property is named consistently.  I can't call the flag "Crypto", since that conflicts with the name of the Crypto class that this instantiates.  Do you have any suggestions?

I don't know how to link to files in reviewboard (I'm only just learning still), but I guess that you missed [test_crypto.js](https://reviewboard.mozilla.org/r/1789/diff/7).
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> I've did the review on review board, please flag me back once you added the
> test and fixed the naming issue I mentioned there.

It looks like reviewboard is adding comments to bugzilla (I'm only just learning how this works myself).  It doesn't quote properly though.
Attachment #8542372 - Flags: review?(gkrizsanits)
(In reply to Martin Thomson [:mt] from comment #7)
> It looks like reviewboard is adding comments to bugzilla (I'm only just
> learning how this works myself).  It doesn't quote properly though.

Right, I'm totally new to this too...

(In reply to Martin Thomson [:mt] from comment #6)
> The name of the API is WebCrypto; the name of the property on window is
> "crypto".  I'm trying to ensure that the property is named consistently.  I
> can't call the flag "Crypto", since that conflicts with the name of the
> Crypto class that this instantiates.  Do you have any suggestions?

The flag should not have capital C, just simply "crypto" as the property on the global. And then it shouldn't conflict with the class name, or am I missing something? The idea is that those bool flags (atob/File/Blob/...) named exactly as the properties will appear on the global (all lower case in this case). Not a regular coding style, but quite useful there, so I wanted to preserve it... And sorry about the test, probably I just looked it over as I'm not too familiar with this review-board yet. It looks great!
Attachment #8542372 - Flags: review?(gkrizsanits) → review+
Attachment #8542372 - Flags: review+ → review?(peterv)
/r/1791 - Bug 1116269 - Crypto/SubtleCrypto only depend on nsIGlobalObject, r=rbarnes
/r/1793 - Bug 1116269 - Add 'crypto' to sandbox global properties, r=gabor

Pull down these commits:

hg pull review -r 6c2ea4e0ab880c38c90828ce42eca784b2791a77
Attachment #8542372 - Flags: review?(peterv)
/r/1791 - Bug 1116269 - Crypto/SubtleCrypto only depend on nsIGlobalObject, r=rbarnes
/r/1793 - Bug 1116269 - Add 'crypto' to sandbox global properties, r=gabor

Pull down these commits:

hg pull review -r 6c2ea4e0ab880c38c90828ce42eca784b2791a77
/r/1791 - Bug 1116269 - Crypto/SubtleCrypto only depend on nsIGlobalObject, r=rbarnes
/r/1793 - Bug 1116269 - Add 'crypto' to sandbox global properties, r=gabor
/r/1865 - Bug 975144 - WebIDL changes for RTC identity sandbox
/r/1867 - Bug 975144 - Implementation of RTC identity DOM component
/r/1869 - Bug 975144 - Rework RTC identity to use JS sandbox
/r/1871 - Bug 975144 - Updating test IdP for new API
/r/1873 - Bug 975144 - Updating RTC identity tests
/r/1875 - Bug 975144 - Adding rtcIdentityProvider to sandbox global scope
/r/1877 - Bug 975144 - Tests for rtcIdentityProvider property

Pull down these commits:

hg pull review -r 8c84952758cc934e83e42129b5e57938f3642e5d
Blocks: 975144
OK, Richard, please disregard reviewboard being stupid.  Can you look at https://reviewboard.mozilla.org/r/1791/ please?
Flags: needinfo?(rlb)
Flags: needinfo?(rlb)
Attachment #8542372 - Flags: review?(rlb) → review+
https://hg.mozilla.org/mozilla-central/rev/b206ea15cb59
https://hg.mozilla.org/mozilla-central/rev/da03d6aff37d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
One new Sandbox global property: crypto
Keywords: dev-doc-needed
Attachment #8542372 - Attachment is obsolete: true
Attachment #8618986 - Flags: review+
Attachment #8618987 - Flags: review+
Attachment #8618988 - Flags: review+
Attachment #8618989 - Flags: review+
Attachment #8618990 - Flags: review+
Attachment #8618991 - Flags: review+
Attachment #8618992 - Flags: review+
Attachment #8618993 - Flags: review+
Attachment #8618994 - Flags: review+
Attached file MozReview Request:
Attached file MozReview Request:
Attached file MozReview Request:
Attached file MozReview Request:
Attached file MozReview Request:
Attached file MozReview Request:
Attached file MozReview Request:
You need to log in before you can comment on or make changes to this bug.