Closed Bug 750421 Opened 8 years ago Closed 7 years ago

Remove unnecessary nsIBadCertListener2 and nsISSLErrorListener implementations

Categories

(Toolkit :: General, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file, 3 obsolete files)

After bug 682329, there is no default UI for cert errors or SSL errors, so all the nsIBadCertListener2 and nsISSLErrorListener implementations that just "return true" can be removed.
Component: about:memory → General
QA Contact: about.memory → general
I have a patch...somewhere.
Assignee: nobody → bsmith
Attached patch WIP #1 (obsolete) — Splinter Review
Attached patch WIP #2 (obsolete) — Splinter Review
Brian, other than a little bitrot, what's preventing WIP #2 from moving forward? (btw, WIP #1 looks empty...)
Flags: needinfo?(bsmith)
IIRC, some part of the patch causes a test to fail, but I cannot remember which one.

Mostly, it is just a matter of me finding time to clean up the patch. If you want to push this forward, go ahead.
Flags: needinfo?(bsmith)
Attached patch patch (obsolete) — Splinter Review
Well, that try run looked good, so I think this is ready for review.
Attachment #717413 - Attachment is obsolete: true
Attachment #717414 - Attachment is obsolete: true
Comment on attachment 719560 [details] [diff] [review]
patch

mayhemer: please review changes in netwerk/
Attachment #719560 - Flags: review?(honzab.moz)
Comment on attachment 719560 [details] [diff] [review]
patch

dholbert: if you could review changes in content/, that's be great (this isn't urgent, but I'd like to avoid bitrot :)
Attachment #719560 - Flags: review?(dholbert)
Comment on attachment 719560 [details] [diff] [review]
patch

dolske: if you could review changes in toolkit/, that's be great too (again, not super urgent)
Attachment #719560 - Flags: review?(dolske)
(In reply to David Keeler (:keeler) from comment #10) 
> dolske: if you could review changes in toolkit/, that's be great too (again,
> not super urgent)

I'm not really a reviewer for /content/base -- maybe ping a DOM peer, from https://wiki.mozilla.org/Modules/Core#Document_Object_Model ?  (maybe Mounir?)
Comment on attachment 719560 [details] [diff] [review]
patch

Over to Mounir.
Attachment #719560 - Flags: review?(dholbert) → review?(mounir)
Comment on attachment 719560 [details] [diff] [review]
patch

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

r=me for the changes in content/base/.
Do you have someone to review the changes in dom/network/src/TCPSocket.js ?
Attachment #719560 - Flags: review?(mounir) → review+
I don't, actually - if you could, that would be great (and anything else you can review, really).
Attachment #719560 - Flags: review?(dolske) → review+
Comment on attachment 719560 [details] [diff] [review]
patch

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

Remove also a comment reference:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsISpeculativeConnect.idl#25

I presume bug 846581 is going to land simultaneously with this patch, so the following won't fail:
http://hg.mozilla.org/mozilla-central/annotate/0a91da5f5eab/netwerk/test/unit/test_spdy.js#l297

r=honzab for the networking parts
Attachment #719560 - Flags: review?(honzab.moz) → review+
This doesn't remove the nsIBadCertListener2 interface itself, so things like test_spdy.js#l297 should still work.
Thanks for the reviews, everyone, by the way.
(In reply to David Keeler (:keeler) from comment #14)
> I don't, actually - if you could, that would be great (and anything else you
> can review, really).

I wouldn't be comfortable reviewing that. I think you should ask the original author of the code (the chunk in dom/network/) to review your change.
Attached patch patch v1.1Splinter Review
Actually, I don't think the changes in dom/network should be in this patch. That nsIBadCertListener2 implementation does more than the others that are being removed and should be handled separately. This patch is the same as the previous, but without those changes and some unnecessary comment removals. Carrying over r+s. Here's another try run, for good measure: https://tbpl.mozilla.org/?tree=Try&rev=fce7c1137f04
Attachment #719560 - Attachment is obsolete: true
Attachment #721821 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1e52347988ea
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.