Using this hook comes with a big performance penalty, since the variables are injected into the <head> and load before even the article content. We should switch to another hook, or a ResourceLoader data module. Our variable names alone add up to 128 bytes:
"wgPopupsReferencePreviews":!0, "wgPopupsReferencePreviewsBeta":!0, "wgPopupsConflictsWithNavPopupGadget":!1, "wgPopupsConflictsWithRefTooltipsGadget":!0,
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| Use flags to represent settings | mediawiki/extensions/Popups | master | +73 -24 | |
| Add tests for bitmask code | mediawiki/extensions/Popups | master | +131 -1 |
@Krinkle I'm struggling to find a better way to provide these variables. The complication is that they all depend on user preferences, so:
Is there any other way to provide user context to a ResourceLoader module?
Change 670245 had a related patch set uploaded (by WMDE-Fisch; owner: WMDE-Fisch):
[mediawiki/extensions/Popups@master] [WIP][POC] Use flags to represent settings
Change 670802 had a related patch set uploaded (by WMDE-Fisch; owner: WMDE-Fisch):
[mediawiki/extensions/Popups@master] Add tests for getConfigBitmaskFromUser
User preferences are already exported on all page views as part of user.options dependency (private, inlined module in head) and its mw.user.options interface.
From a quick look, it seems it is computed based on site config and a user pref. The site config doesn't need to be checked client-side afaik since the JS code being loaded already implies that at least the site config enablement is on. If there's some amount of code that needs to load regardless of pref, then checking the pref client-side is trivial without these variables.
Is it feasible to move the logic to the server and instead control the enablement by virtue of the module being added to OutputPage based on its result? What else do these values do on the client side?
From a quick look, it seems it is computed based on site config and a user pref. The site config doesn't need to be checked client-side afaik since the JS code being loaded already implies that at least the site config enablement is on. If there's some amount of code that needs to load regardless of pref, then checking the pref client-side is trivial without these variables.
Thanks, this would be a reasonable solution. We'll try a patch along these lines. In the meantime, I think the patch for review is a good step forward, it reduces though doesn't eliminate the <head> overhead.
Is it feasible to move the logic to the server and instead control the enablement by virtue of the module being added to OutputPage based on its result? What else do these values do on the client side?
Unfortunately, we're providing two different features with the same code base, and they will be independently controllable. The wgPopupsReferencePreviews, wgPopupsConflictsWithNavPopupGadget, and wgPopupsConflictsWithRefTooltipsGadget variables are involved in this logic. The code base is also rolled up into a single module by the compilation step. There are currently client-side settings for anonymous users which can override default settings, so in theory the server doesn't have enough information to disable either feature. We could change this, but it's probably not worthwhile to do this deeper refactoring, especially since the likely implementation (one common module, two small modules injected only if each feature is present) come with their own critical-path overhead.
The wgPopupsReferencePreviewsBeta setting modifies some presentation, and rendering is done by the client so probably must stay this way.
Thanks for the ideas, it looks like the user.options thread is worth a bit more follow-up!
Change 670245 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Use flags to represent settings
Change 670802 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Add tests for bitmask code
The remaining question for this task is whether we want to look at explicitly querying user options.