Remove unimplemented method in nsCrypto

RESOLVED FIXED in mozilla31

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: allstars, Assigned: anujagarwal464)

Tracking

({dev-doc-needed, site-compat})

Trunk
mozilla31
dev-doc-needed, site-compat
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

In Bug 883741, comment 61 BZ Suggested we'd remove those unimplemented methods in nsCrypto.
(including popChallengeResponse, random, and disableRightClick).
(Assignee)

Comment 1

4 years ago
I would like to work on this bug, could you update the description.
Flags: needinfo?(allstars.chh)
I haven't started working on this bug, feel free to take it.
Assignee: allstars.chh → nobody
Flags: needinfo?(allstars.chh)
(Assignee)

Comment 3

4 years ago
Created attachment 8407450 [details] [diff] [review]
bug897359.diff
Assignee: nobody → anujagarwal464
Attachment #8407450 - Flags: feedback?(allstars.chh)
Comment on attachment 8407450 [details] [diff] [review]
bug897359.diff

Review of attachment 8407450 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I am not a peer in either DOM nor Security.
Can you find someone else to review your patch?

Thanks
Attachment #8407450 - Flags: feedback?(allstars.chh)
(Assignee)

Updated

4 years ago
Attachment #8407450 - Flags: review?(bzbarsky)
Comment on attachment 8407450 [details] [diff] [review]
bug897359.diff

Thank you for working on this!

Have you compiled this patch?  Have you run the tests in dom/tests/mochitest/crypto ?  I'm guessing the answer is "no", for both of those.....  Please do that and fix the issues it catches?
Attachment #8407450 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

4 years ago
Comment on attachment 8407450 [details] [diff] [review]
bug897359.diff

Build was successful and all tests in dom/tests/mochitest/crypto passed.


(In reply to Boris Zbarsky [:bz] from comment #5)
> Comment on attachment 8407450 [details] [diff] [review]
> bug897359.diff
> 
> Thank you for working on this!
> 
> Have you compiled this patch?  Have you run the tests in
> dom/tests/mochitest/crypto ?  I'm guessing the answer is "no", for both of
> those.....  Please do that and fix the issues it catches?
Attachment #8407450 - Flags: review?(bzbarsky)
Comment on attachment 8407450 [details] [diff] [review]
bug897359.diff

Oh, I see, because these are in the superclass.  So this is fine as far as it goes.  r=me on this part.

But what I was actually asking for in bug 883741 is to remove these pointless methods completely.  Not just from nsCrypto, but also from Crypto and from Crypto.webidl.
Attachment #8407450 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 8

4 years ago
Created attachment 8408399 [details] [diff] [review]
bug897359.diff incomplete

When I compile with this patch I'm getting following error 

error C3668: 'nsCrypto::PopChallengeResponse' : method with override specifier 'override' did not override any base class methods
Attachment #8408399 - Flags: feedback?(bzbarsky)
Comment on attachment 8408399 [details] [diff] [review]
bug897359.diff incomplete

That's odd, since you're removing nsCrypto::PopChallengeResponse, no?
Attachment #8408399 - Flags: feedback?(bzbarsky) → feedback+
https://tbpl.mozilla.org/?tree=Try&rev=84a9a3ae8068 for that last patch.

The reason for the compile failure in comment 8 was doing a local build in dom/, which didn't export the modified header.
(Assignee)

Comment 11

4 years ago
Created attachment 8408770 [details] [diff] [review]
bug897359.diff -final

Try build was successful.
Attachment #8408399 - Attachment is obsolete: true
Attachment #8408770 - Flags: review?(bzbarsky)
Comment on attachment 8408770 [details] [diff] [review]
bug897359.diff -final

r=me
Attachment #8408770 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

4 years ago
Attachment #8407450 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e5bffd7d60c5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31

Updated

4 years ago
Keywords: dev-doc-needed, site-compat
OS: Linux → All
Hardware: x86_64 → All
You need to log in before you can comment on or make changes to this bug.