Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Embedded WebExtension #11760

Closed
wants to merge 11 commits into from

Conversation

@Hainish
Copy link
Member

commented Aug 8, 2017

This PR, superseding #11639 and resolving #10007, migrates settings from the legacy XPCOM codebase to a new, Embedded WebExtensions codebase. This includes:

  • High-level preferences: Whether HTTPS Everywhere is disabled, whether HTTP requests are allowed, if a counter should be displayed
  • Which rulesets are manually enabled/disabled
  • Any custom rulesets which are housed in HTTPSEverywhereCustomUserRules

In addition, this includes various fixes to the functionality of the WebExtensions code to make it work gracefully in Firefox, cherry-picked from the webextensions branch.

Embedded webextensions will be deprecated as of Firefox 57 as well, so this is merely an intermediate step to enable us to have users' settings migrated.

Additionally, this includes a new options ui to allow users to import a json file with settings. Since this replaces #11639, there will be no way for users to export previous settings yet. This won't be a problem, since Embedded WebExtensions are able to transfer settings automatically via the runtime.sendMessage API.

@Hainish Hainish referenced this pull request Aug 8, 2017

@Hainish Hainish added this to In Progress in Firefox WebExtension Aug 8, 2017

@Hainish Hainish force-pushed the Hainish:embedded-webextension branch from 856656a to d190662 Aug 8, 2017

@Hainish Hainish force-pushed the Hainish:embedded-webextension branch from d190662 to 48d8d3e Aug 8, 2017

@Hainish Hainish changed the title WIP: Embedded webextension Embedded webextension Aug 9, 2017

@Hainish Hainish changed the title Embedded webextension Embedded WebExtension Aug 9, 2017

@Hainish

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2017

In order to maximize the window of time users are given to restart their browsers and have their settings migrated (#11639 (comment)) I'd like to merge this in as soon as possible, and issue a release of the Embedded WebExtension early next week.

@ghost

This comment has been minimized.

Copy link

commented Aug 9, 2017

@Hainish I hope you didn't sacrifice testing to make this come out earlier?

@Hainish

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2017

@koops76 I've been testing all along, but there are bound to be edge cases. Any beta-testers that that want to install this are welcome to, but this combines the work over many months of enumerating bugs in the Firefox WebExtension code and pushing out fixes. What's new in this PR is the migration code as well as the XPCOM wrapper.

@Hainish

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2017

@jsha

This comment has been minimized.

Copy link
Member

commented Aug 9, 2017

In addition, this includes various fixes to the functionality of the WebExtensions code to make it work gracefully in Firefox,

These should be in a separate PR.

@jsha

This comment has been minimized.

Copy link
Member

commented Aug 9, 2017

I've changed the Firefox tests to use Selenium just as the Chrome tests do.

This should also be a separate PR.

@Bisaloo

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2017

@Hainish, I'm sorry, I'm not sure how to test this.

I checked out to your branch and ran bash makexpi.sh which exited just fine but Firefox is telling me "This add-on could not be installed because it appears to be corrupt."

What am I doing wrong?

@ghost

This comment has been minimized.

Copy link

commented Aug 9, 2017

@Bisaloo Probably a signature issue.

@gloomy-ghost

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2017

@Bisaloo You can disable the signature checking by toggling xpinstall.signatures.required, however this option only works for Nightly.

@J0WI

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2017

You can disable the signature checking by toggling xpinstall.signatures.required, however this option only works for Nightly.

You should use an old Nightly Build for this, because they are already removing XPCOM support from current Nightlies (57.0a1).

@jsha
Copy link
Member

left a comment

Some initial comments, haven't reviewed everything yet.


// Send a message to the embedded webextension bootstrap.js to get settings to import
chrome.runtime.sendMessage("import-legacy-data", function(settings){
import_settings(settings);

This comment has been minimized.

Copy link
@jsha

jsha Aug 9, 2017

Member

You can just provide import_settings directly as the callback parameter to sendMessage, right?

This comment has been minimized.

Copy link
@Hainish

Hainish Aug 9, 2017

Author Member

Fixed in #11792

});

/**
* Enable switch planner for specific tab

This comment has been minimized.

Copy link
@jsha

jsha Aug 9, 2017

Member

Incorrect comment

This comment has been minimized.

Copy link
@Hainish

Hainish Aug 9, 2017

Author Member

Fixed in #11792

for(let ruleset of settings.custom_rulesets){
all_rules.addFromXml((new DOMParser()).parseFromString(ruleset, 'text/xml'));
}
storage.set({"legacy_custom_rulesets": settings.custom_rulesets}, item => {

This comment has been minimized.

Copy link
@jsha

jsha Aug 9, 2017

Member

Similarly, no need to define a closure here, just pass resolve as the callback.

Why store this under "legacy_custom_rulesets" rather than store the rulesets in the same way as the other custom rulesets? That way we could just call the appropriate function to load all custom rulesets when we're done, eliminating a possible edge case.

This comment has been minimized.

Copy link
@Hainish

Hainish Aug 9, 2017

Author Member

@jsha the other custom rulesets are specially formatted objects that do not allow specifying, for instance, exclusions. Here is one example:

[{"host":"example.com","redirectTo":"https://example.com/","urlMatcher":"^http://example\\.com/"}]

This was never in XML format, since it is added via the WebExtensions UI. From that format, a RuleSet object is created. This format, notably, does not in its current form support exclusions & securecookies. We'd have to build support for that, as well as code that converts XML to the custom_rule format. I'm not against doing this, but it seems like the job for a future migration.

This comment has been minimized.

Copy link
@Hainish

Hainish Aug 9, 2017

Author Member

Removal of closure resolved in #11792

This comment has been minimized.

Copy link
@jsha

jsha Aug 9, 2017

Member

Sounds reasonable on the custom rulesets vs XML thing.

@Hainish

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2017

PR split into 3 distinct PRs: #11789, #11791, #11792

@Hainish Hainish closed this Aug 9, 2017

@J0WI J0WI moved this from In Progress to Done in Firefox WebExtension Aug 9, 2017

@J0WI J0WI removed this from Done in Firefox WebExtension Aug 9, 2017

@J0WI J0WI added this to Won't fix in Firefox WebExtension Aug 9, 2017

@ghost

This comment has been minimized.

Copy link

commented Aug 11, 2017

@J0WI Move to "Done" please.

@Hainish

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2017

This PR wasn't merged so I'll just remove it from the board

@Hainish Hainish removed this from Won't fix in Firefox WebExtension Aug 11, 2017

@Hainish Hainish deleted the Hainish:embedded-webextension branch Feb 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.