Closed Bug 897359 Opened 11 years ago Closed 10 years ago

Remove unimplemented method in nsCrypto

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: allstars.chh, Assigned: anujagarwal464)

Details

(Keywords: dev-doc-needed, site-compat)

Attachments

(1 file, 2 obsolete files)

In Bug 883741, comment 61 BZ Suggested we'd remove those unimplemented methods in nsCrypto.
(including popChallengeResponse, random, and disableRightClick).
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)
Attached patch bug897359.diff (obsolete) — Splinter Review
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)
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)
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+
Attached patch bug897359.diff incomplete (obsolete) — Splinter Review
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.
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+
Attachment #8407450 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e5bffd7d60c5
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
OS: Linux → All
Hardware: x86_64 → All
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.