750421 - Remove unnecessary nsIBadCertListener2 and nsISSLErrorListener implementations

Status: RESOLVED FIXED

(Toolkit :: General, enhancement)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I have a patch...somewhere.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: WIP #1

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: WIP #2

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Brian, other than a little bitrot, what's preventing WIP #2 from moving forward? (btw, WIP #1 looks empty...)

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Ok - I did some cleanup and pushed it to try: https://tbpl.mozilla.org/?tree=Try&rev=6b764d9a2518

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

Well, that try run looked good, so I think this is ready for review.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 719560 [details] [diff] [review]
patch

mayhemer: please review changes in netwerk/

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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 :)

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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)

Comment 11

[Daniel Holbert [:dholbert]]

(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 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 719560 [details] [diff] [review]
patch

Over to Mounir.

Comment 13

[Mounir Lamouri (: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 ?

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I don't, actually - if you could, that would be great (and anything else you can review, really).

Comment 15

[Honza Bambas (:mayhemer)]

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

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

This doesn't remove the nsIBadCertListener2 interface itself, so things like test_spdy.js#l297 should still work.

Comment 17

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Thanks for the reviews, everyone, by the way.

Comment 18

[Mounir Lamouri (:mounir)]

(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.

Comment 19

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v1.1

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

Comment 20

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e52347988ea

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1e52347988ea

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Followups to unbreak Thunderbird and Seamonkey.
https://hg.mozilla.org/comm-central/rev/5110d56fbd96
https://hg.mozilla.org/comm-central/rev/8fb887a279ff