Page MenuHomePhabricator

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

Description

mobile.startup adds a module 'mobile.startup/settings' which depends on mediawiki.storage 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:
https://github.com/wikimedia/mediawiki/blob/master/resources/src/startup.js#L59

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 mw.storage for saving settings

https://gerrit.wikimedia.org/r/359219

(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 mediawiki.storage

https://gerrit.wikimedia.org/r/359223

Jdlrobson added a subscriber: Krinkle.

https://gerrit.wikimedia.org/r/359219 has had some review from @Krinkle, I also noticed while reviewing that the description for task wasn't quite accurate. https://gerrit.wikimedia.org/r/359223 will need to be a dependency.

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

https://gerrit.wikimedia.org/r/359223

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

https://gerrit.wikimedia.org/r/359219