Use storage.sync APIs to sync preferences accross browser using Firefox Sync. #31

Natim · 2018-02-01T09:56:56Z

Fixes #30

…ox Sync. Fixes stoically#30

Natim · 2018-02-01T09:58:56Z

In case you were wondering it is safe to use storage.sync even if users don't have sync activated. Also the backend is SQLite rather than a JSON file in the profile.

stoically · 2018-02-01T10:19:29Z

Hi, thanks for the PR.

Syncing everything is not possible since it would also sync stuff that's only valid in the local browser. So that would need to still be stored in the local storage.

Also, how would switching from local to sync affect existing installations? Is some sort of migration needed or does it just work?

Natim · 2018-02-01T10:36:25Z

Syncing everything is not possible

Thank you for noticing that. I am going to split the storage and only store preferences inside the sync backend then.

how would switching from local to sync affect existing installations? Is some sort of migration needed or does it just work?

I handle the migration by checking the local storage in case the sync storage is empty.

stoically · 2018-02-01T10:49:02Z

I handle the migration by checking the local storage in case the sync storage is empty.

Ah, now I see how the migration is done, cool.

I am going to split the storage and only store preferences inside the sync backend then.

I'd also say that only storing preferences in sync is the way to go, yeah.

@stoically

@stoically review.

stoically · 2018-02-01T11:27:28Z

I understand that babel stage2 is probably/unfortunately needed to support the spread operator with webpack - but I really don't want to ship the whole codebase converted to stage2 just to have a slightly cleaner default preference detection/merge in the src-file. So if you could please change that accordingly and remove babel stage2 again.

Other than that it looks good to me. Will test it manually to make sure everything works correct.

Natim · 2018-02-01T11:31:19Z

I really don't want to ship the whole codebase converted to stage2 just to have a slightly cleaner default preference detection/merge in the src-file.

Yes I quite agree, do you know if there is another way for webpack not to raise a syntax error with the spread operator since Firefox handles it?

Natim · 2018-02-01T11:33:30Z

According to acornjs/acorn@5aa2e73 it should be in the next release of webpack.

Natim · 2018-02-01T11:37:04Z

Would stage3 work for you?

stoically · 2018-02-01T11:42:50Z

Adding the babel-plugin-transform-object-rest-spread package and replacing presets:[ 'stage-2' ] with plugins: ['transform-object-rest-spread'] should do the trick until it lands in webpack.

Natim · 2018-02-01T11:50:57Z

All it does is:

this.preferences = _extends({}, this.preferencesDefault, this.preferences);

stoically · 2018-02-01T12:02:37Z

src/background/storage.js

      // set preferences defaults if not present
-      if (!this.local.preferences) {
+      if (!Object.keys(this.preferences).length) {


This will always be true for a fresh installation if I'm not mistaken. Since this is the migration path it should additionally check whether this.local.preferences exists.

Actually that's what is done the next line, when we create this.preferences we make sure to override default preferences with the local ones.

If you install the Add-on the first time after this change is published, this.local.preferences will be undefined and the migration happening is not needed. It will only happen once; but still - it should be a migration-path, not something that runs on every first time install.

Ok, I see now - it actually does two things. For one it sets the preferences to default on first time install - which is wanted behavior. And additionally it migrates this.local.preferences if available. Could you please check explicitly for this.local.preferences inside this if and only do the migration if it exists? Thanks.

I can but it doesn't cost anything to do it if it doesn't exists :)

stoically · 2018-02-01T12:10:47Z

src/background/storage.js

@@ -35,33 +36,30 @@ class Storage {
    try {
      let storagePersistNeeded = false;
      this.local = await browser.storage.local.get();
+      this.preferences = await browser.storage.sync.get();


Personally I like explicit over implicit, so I'd actually prefer this.sync here - which would as a side-effect make it possible that we could store something else besides preferences into sync without having to implement a migration-path again.

But since I'm not sure what else could be stored to sync, I'd also be OK with leaving it as it is.

stoically · 2018-02-01T12:51:05Z

src/background/storage.js

      }

+      // Make sure to default missing preferences with default value
+      this.preferences = {...this.preferencesDefault, ...this.preferences};
+
      if (storagePersistNeeded) {


When a new preferencesDefault key is added this will no longer automatically set to true and thus the new preference default will not be available on the options page since the options page accesses storage directly - tho still local and not sync.

Good find 🥇 should we use storage there too or switch to sync?

I'm not exactly sure what you mean - could you please elaborate?

If you with "use storage" meant to require/import the storage.js in the options page - then I'd say that's not that easy and I don't see the point of doing that since you could only reuse the code, but not access the actual background storage from the options page, afaik. Changing it to sync sounds good to me.

I mean using the storage.js interface to save the preferences.

It is a bit sad to let the preference page know about the underlying storage but I understand it might not be trivial to do it.

If you would want to abstract it away, the way to go would imho be sending a message to the background to request the storage. The same is already done when saving the preferences.

Yes that's what I had in mind, should I give it a try?

If you like to, sure.

stoically · 2018-02-01T14:13:00Z

Something that just came to my mind. I've never used Firefox Sync - is it possible to deactivate for particular Add-ons? Like if someone has Sync configured but wants different preferences for an Add-on on a particular machine - would that be possible or would the Add-on have to implement that itself?

Natim · 2018-02-01T14:14:19Z

You can deactivate sync for add-ons with Firefox Sync but if you don't it will always sync the preferences stored in storage.sync

stoically · 2018-02-01T14:22:55Z

I wonder how other Add-ons handle that - do they have a setting to disable syncing or do they just say: "if you enable Firefox Sync for Add-ons, your preferences will be synced, that's it"? :D

Natim · 2018-02-01T14:34:32Z

Yes I guess the later :)

stoically · 2018-02-01T14:38:37Z

Well, just checked uBlock Origin and it has an Enable cloud storage support preference which is deactivated by default. So I guess it depends.

Actually, having it as an opt-in preference sounds really good to me. What do you think?

Natim · 2018-02-01T14:43:36Z

IMHO I would rather go for the opt-out one, but I guess it depends what take we have on the default preferences 😂

Natim · 2018-02-01T14:54:28Z

I think it doesn't make sense to have a setting and not only because I am lazy implementing it but also because once you've configured your website rules and click rules there is no point not syncing them if you use the same sync account with add-on synchronization between them.

stoically · 2018-02-01T15:01:50Z

The thing with opt-out is, that if you have Firefox Sync enabled and install the Add-on, it instantly syncs storage.sync - and if you then actually opt-out the Add-on can't really just delete everything in storage.sync because how do you know what got stored by your or by other machines? That would get complicated to keep a state. So you might end up with data in the cloud that you didn't want to have there in the first place. On the other hand you could argue that if you activated Firefox Sync including Add-ons, you probably want exactly that and I agree with you that being able to sync preferences (like website rules) is really useful.

Personally I'm on the data-protection end of the spectrum though and thus would want to avoid handing out data without explicit permission. Which is actually one of the reasons I made this Add-on with "Automatic Mode" enabled per default in the first place (don't want to hand out my cookies and localstorage to anyone if it's not an explicit permanent container :D).

Natim · 2018-02-01T15:20:43Z

I'm on the data-protection end of the spectrum

The data saved in the cloud is encrypted

stoically · 2018-02-01T15:38:15Z

The data saved in the cloud is encrypted

Yeah, and if what I've read is correct your sync gets wiped if you change your password because it's needed to use the key to decrypt. Which is a strong signal that Mozilla takes security and privacy seriously - one of the reasons why I use Firefox.

But "data-protection" also means giving users a choice. Even the strongest encryption might one day be broken - so if I want to keep data safe, I might want to decide that particular data should never leave my machine. Which is why I don't use Firefox Sync.

Of course - Temporary Containers preferences aren't that important - but for me it's also a matter of principle.

Natim · 2018-02-01T15:39:56Z

Yes I understand, then maybe we should fix this as wontfix.

stoically · 2018-02-01T15:48:25Z

My standpoint is just that I don't want to sync by default - having a preference to opt-in sync is absolutely something I want to add. If you don't feel like putting more work into this PR because of that, that's absolutely OK. When I find the time to work on this feature I'm going to build on top what you've committed so far - if that's not OK for you, just let me know.

Thanks for getting involved and sorry that it didn't went like you expected it to.

Natim · 2018-02-01T15:54:50Z

I totally understand, don't worry about it. I thought I would be able to contribute it but I spent way too much time on it today, so I would rather close this than let it rot forever.

You've been really helpful and thank your for your nice feedback, it's been a nice experience working with you on that PR today.

I think if I were to work on it again I would have started from scratch the other way around:

Adding a preferences in the panel
Then changing the storage backend to sync only if the preference is true.

In a way it is simpler than this PR alltogether :)

stoically · 2018-02-01T16:00:11Z

Happy to hear that! Yeah, interesting and insightful PR for me too. Maybe until next time. :)

Use storage.sync APIs to sync preferences accross browser using Firef…

0a4d23a

…ox Sync. Fixes stoically#30

Rémy HUBSCHER added 2 commits February 1, 2018 11:01

Use browser.storage.sync stub in tests.

ed7491b

Update browser.storage mock.

464301c

Rémy HUBSCHER added 2 commits February 1, 2018 11:56

Only store preferences in storage.sync.

6510fb0

@stoically review.

Update webpack config

1385c49

Update webpack config

77acdba

stoically requested changes Feb 1, 2018

View reviewed changes

stoically reviewed Feb 1, 2018

View reviewed changes

stoically requested changes Feb 1, 2018

View reviewed changes

Natim closed this Feb 1, 2018

Natim deleted the migrate-to-storage-sync branch February 1, 2018 15:40

Use storage.sync APIs to sync preferences accross browser using Firefox Sync. #31

Use storage.sync APIs to sync preferences accross browser using Firefox Sync. #31

Conversation

Natim commented Feb 1, 2018

Natim commented Feb 1, 2018

stoically commented Feb 1, 2018

Natim commented Feb 1, 2018

stoically commented Feb 1, 2018

stoically commented Feb 1, 2018 • edited Loading

Natim commented Feb 1, 2018

Natim commented Feb 1, 2018

Natim commented Feb 1, 2018

stoically commented Feb 1, 2018 • edited Loading

Natim commented Feb 1, 2018

stoically Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoically Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoically Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

stoically Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoically Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoically commented Feb 1, 2018

Natim commented Feb 1, 2018 • edited Loading

stoically commented Feb 1, 2018 • edited Loading

Natim commented Feb 1, 2018 • edited Loading

stoically commented Feb 1, 2018 • edited Loading

Natim commented Feb 1, 2018

Natim commented Feb 1, 2018

stoically commented Feb 1, 2018 • edited Loading

Natim commented Feb 1, 2018 • edited Loading

stoically commented Feb 1, 2018

Natim commented Feb 1, 2018

stoically commented Feb 1, 2018

Natim commented Feb 1, 2018

stoically commented Feb 1, 2018

stoically commented Feb 1, 2018 •

edited

Loading

stoically commented Feb 1, 2018 •

edited

Loading

stoically Feb 1, 2018 •

edited

Loading

stoically Feb 1, 2018 •

edited

Loading

stoically Feb 1, 2018 •

edited

Loading

stoically Feb 1, 2018 •

edited

Loading

stoically Feb 1, 2018 •

edited

Loading

Natim commented Feb 1, 2018 •

edited

Loading

stoically commented Feb 1, 2018 •

edited

Loading

Natim commented Feb 1, 2018 •

edited

Loading

stoically commented Feb 1, 2018 •

edited

Loading

stoically commented Feb 1, 2018 •

edited

Loading

Natim commented Feb 1, 2018 •

edited

Loading