Page MenuHomePhabricator

Remove settings.js module from mobile.startup
Closed, ResolvedPublic1 Estimated Story Points


mobile.startup adds a module 'mobile.startup/settings' which depends on and adds cookie support.
This is not needed any more given that localStorage is so widely supported.
We don't appear to use the cookie fallback for anything inside MobileFrontend except choosing the default editor (which is strange as VisualEditor is unlikely to be supported on browsers which don't have localStorage)

Note all ResourceLoader enabled clients have localStorage:

So this fallback only assists users who have exhausted localStorage.
I think it is better to use the generic storage library in core.

Possible side effects of this change:

  • ZeroBanner uses this module. Make sure to not remove the module before swapping out the code in such extensions.
  • It may not be possible to disable a zero banner
  • VisualEditor may load as the default editor rather than the wikitext editor or vice versa

Both are unlikely and low risk on the majority of modern clients.

Event Timeline

Jdlrobson raised the priority of this task from Low to Medium.Apr 14 2017, 12:28 AM
Jdlrobson updated the task description. (Show Details)
Jdlrobson set the point value for this task to 1.

I'm not sure why we still ship this code. Since all browsers we run JS on support localStorage I suggest the priority reflect this.

By ^ this isn't mobile.settings any more but mobile.startup/settings instead. If I find some spare time I'll try to dig into this, but everybody else feel free to claim if interested.

Jdlrobson renamed this task from Remove mobile.settings module to Remove settings.js module from mobile.startup.Apr 24 2017, 7:45 PM
Jdlrobson updated the task description. (Show Details)

Confirming that we're OK with working on the task given the side effects mentioned in the description? Since the two projects affected are not owned by Reading Web, we should at least ping their corresponding owners.

Yes. The zero issue is not a problem. I think it's enough here to ping the VisualEditor team on code review for the patchset. Neither should be an issue given all clients who run JS have localStorage enabled.

Change 359219 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/MobileFrontend@master] Use for saving settings

(looks like that comment related to an older form of this task).

Change 359223 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/ZeroBanner@master] Zero should use

Jdlrobson added a subscriber: Krinkle. has had some review from @Krinkle, I also noticed while reviewing that the description for task wasn't quite accurate. will need to be a dependency.

Change 359223 merged by jenkins-bot:
[mediawiki/extensions/ZeroBanner@master] Zero should use

Change 359219 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Use for saving settings