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

Change AppCache from browser whitelist to blacklist #2241

Closed
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
2 participants
@Cangit
Contributor

Cangit commented Jun 19, 2014

Implemented changes talked about in #2235
In addition to changing AppCache from whitelist to blacklist I also added the discussed change of having Firefox default to 'on', this will break backwards compatibility!

  • What browsers AppCache is enabled in is now using a blacklist system rather than whitelist system.
  • AppCache is now on by default in all browsers.
  • You can use the same config parameters as before to disable AppCache in your browsers of choice.
  • Config option parameter 'browsers' is deprecated, this option was previously undocumented(!)

Pros:

  • Easier to maintain a blacklist than a whitelist. (Example: Opera was unsupported, and therefore off. But it can handle AppCache.)
  • All on by default is more consistent.
  • Less lines, improves code readability.
  • ... ?

Cons:

  • Current developers who rely on Firefox being off by default will have to add 'firefox: false' in config
  • ... ?
@glasser

This comment has been minimized.

Member

glasser commented Jun 27, 2014

Thanks for doing this. But if FF really is still popping up a dialog, then the blacklist probably should default to containing FF. Can you adjust? (Plus, that way it's not that backwards incompatible.)

I think the "browsers" option (which overrides all defaults) was intentional, but probably should be documented instead of removed.

@Cangit

This comment has been minimized.

Contributor

Cangit commented Jul 14, 2014

@glasser sorry for being absent on this, its summer after all ;-)

I have done some testing and I can't produce the pop-up dialog on FireFox 30 anymore, I don't know when they decided to change their policy and follow all the other browsers on this, but it looks like they have finally changed.

Regarding the 'browsers' parameter I was in the process of adding it back in now, but I had to stop as it really don't make any sense for me. If the parameter would override the defaults and add your passed in arguments as defaults instead it would be perfectly viable for me. However, thats not what this parameter does. This parameters only purpose is to override the defaults (defaults are now that all browsers are on) and then turn what-ever browsers you pass in to be on, so basically it wont achieve anything. If you could pass in browsers that you wanted to disable and turn off, it would make sense, but the parameter will only allow the browsers you pass in to be set on, however every browser are now already on(!). This function made much more sense when the default was that some browser were off and if a browser was not defined in the hardcoded 'knownBrowsers' array(now deprecated) you would need a function like this to add them in. I will argue it would still make sense if it allowed you to pass on browsers you wanted to turn off, but it wont.

So I'm really struggling to see any use of this command, the only edge case I can find is if you first turn some browsers off using

Meteor.AppCache.config({
  chrome: false,
  firefox: false
});

And then instead of turning them back on using the currently documented way

Meteor.AppCache.config({
  chrome: true,
  firefox: true
});

You do it with the browsers command like so:

Meteor.AppCache.config({
  browsers: { chrome, firefox }
});

A to me (personal preference I guess) a less readable way, and its hard to realize that I actually overwrite all previous set configuration. If its desirable to have this parameter, could a name change be considered? Example: call it 'browsersReset', without needing any parameters, as the parameters really don't do anything now regardless.

Do I make any sense at all? :-S

TL;DR: It looks to me that the 'browsers' parameter was intended as a way to bypass the 'knownBrowsers' array, that array is now deprecated so the 'browsers' parameter makes less sense. FireFox no longer gives you a pop-up (Hurray!)

@glasser

This comment has been minimized.

Member

glasser commented Jul 17, 2014

Good catch re firefox. Looks like: https://bugzilla.mozilla.org/show_bug.cgi?id=892488

So it definitely makes sense to make the default be "all browsers".

That said, I don't see a reason to explicitly break people who've used the browsers flag. We make users rewrite their apps on upgrade enough; let's avoid this particular case.

Also, since we're not completely getting rid of the config feature, we should keep around the paragraph which lists the browser names.

@Cangit

This comment has been minimized.

Contributor

Cangit commented Jul 17, 2014

@glasser alright, I have re-added the 'browsers' option and the paragraph which lists the browser names.

@glasser

This comment has been minimized.

Member

glasser commented Jul 18, 2014

Thanks, merged!

@glasser glasser closed this Jul 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment