Opened 8 years ago

Closed 8 years ago

#4060 closed enhancement (fixed)

Add a global enable / disable to the Toolbar Menu

Reported by: pde Owned by: pde
Priority: Low Milestone:
Component: HTTPS Everywhere/EFF-HTTPS Everywhere Version:
Severity: Keywords:
Cc: jacobske87@…, schoen, mikeperry Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Semantically, this is similar to, but not quite the same as, the "disable all" and "reset to defaults" options in the ruleset window.

Patches welcome ;)

Child Tickets

Change History (9)

comment:1 Changed 8 years ago by jacobske87

Cc: jacobske87@… added

Started work on this - https://github.com/kevinjacobs/HTTPS-Everywhere/commit/b0bc2a4b5c271dab37a314814e636a0db7b74848

The code does work, but before I do any more with this approach.. Is this roughly what you had in mind, or can you foresee any problems with this method? I'm not sure if there's anything else that would be affected by just removing the listeners (and obviously I'll need to set a boolPref so it persists through restarts).

Let me know and I'll get it finished up - thanks.

comment:2 Changed 8 years ago by pde

Cc: schoen added
Status: newaccepted

Hi Kevin,

This is a great start.

The main things I'd want before merging are:

  • Give the user some modal indication of whether the extension is enabled or not (the best way, I think, is to have the text switch between "Disable HTTPS Everywhere" and "Enable HTTPS Everywhere"
  • Reload the page (perhaps using the same call we make for reloads when toggling a ruleset)
  • Grey out or hide all the ruleset related entries in the toolbar menu, since those are confusing if the extension is actually disabled at the moment.

It might also be best to move this option to the bottom of the menu.

comment:3 Changed 8 years ago by jacobske87

Great - I'll try to get it finished up this weekend. Thanks!

comment:4 Changed 8 years ago by jacobske87

Status: acceptedneeds_review

I have this finished - here's the commit: https://github.com/kevinjacobs/HTTPS-Everywhere/commit/252d061ba9052b21102c6913a9bd1c18c4d9a16b

I've been testing it out this evening and it seems to work great. Let me know if you need anything else changed before merging.

One thing to note is that to pull localized strings from a .js file, they have to be in a separate /locale/*.properties file (rather than the .dtd). I added one for each locale, but they're all in English right now.

comment:5 Changed 8 years ago by pde

So I tried to merge this with master (the results are in the global-enable-disable branch of my remote).  It doesn't seem to be working: I still find, for instance, that google.com is remapped to https://encrypted.google.com.

I haven't looked at the code super closely but I suspect you aren't toggling our nsIContentPolicy->shouldLoad() callback.  You might find this summary of the places we hook useful: https://bugzilla.mozilla.org/show_bug.cgi?id=677643#c26, but note that we also have some callbacks when cookies are set, in order to enforce the <securecookie> directives in rulesets -- we should double check those too.

comment:6 Changed 8 years ago by jacobske87

Thanks for the link - it looks like I was missing the other callbacks.

I updated the code here: https://github.com/kevinjacobs/HTTPS-Everywhere/commit/491c39903cb855bbb9f98a584ab6f548534c4cee and it now works for the previously problematic sites.

I feel like this is a really hackish (bad) way to do this - Is there an easier way to toggle the other callbacks (I'm not even seeing addEventListener/addObservers for them?), or just use the if/else statements? I'm open to suggestions.

Lastly - When I toggle enable/disable, I'm only having it reload the current tab. Are you okay with this? I feel like reloading all tabs is a little too aggressive (e.g. background Youtube tabs would start replaying, any unsubmitted form data could be lost, etc).

Thanks for the feedback - I'm still learning the codebase.

comment:7 in reply to:  6 Changed 8 years ago by pde

Cc: mikeperry added

Replying to jacobske87:

I feel like this is a really hackish (bad) way to do this - Is there an easier way to toggle the other callbacks (I'm not even seeing addEventListener/addObservers for them?), or just use the if/else statements? I'm open to suggestions.

We can use if/else if all else fails, but let's try to do it right first.

We aren't making the addCategoryManager registration call shown here: https://developer.mozilla.org/En/NsIContentPolicy . I think that's because XPCOMUtils is doing it for us from the _xpcom_categories attribute of the prototype (see https://developer.mozilla.org/en/JavaScript_code_modules/XPCOMUtils.jsm).

But you could certainly try calling this guy to undo the registration that XPCOMUtils made for us: https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsICategoryManager#deleteCategoryEntry()

comment:8 Changed 8 years ago by jacobske87

Hey Mike, thanks for the info.

I updated my code here: https://github.com/kevinjacobs/HTTPS-Everywhere/commit/166547df9bf916bd8e6633b59313fccbbf3e5e73

It seems to work great now (and got rid of those if/else blocks from the last commit). Firefox error log is clean, and by Wireshark logs, everything seems to be re-enabling normally.

Let me know how it looks.

comment:9 Changed 8 years ago by pde

Resolution: fixed
Status: needs_reviewclosed

This looks solid enough to test on our development branch users now :). Merged!

Note: See TracTickets for help on using tickets.