Change 568545 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/core@master] Add a useskin= cookie for selecting the skin

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

It can be used to set the skin for unregistered readers too if there will be a settings screen or menu for readers.

This probably may not work well with cache infrastructure. See T134592: Allow setting the UI to a language other than English for anonymous users and T149419#2828359

In T243920#5839183, @Bugreporter wrote:

It can be used to set the skin for unregistered readers too if there will be a settings screen or menu for readers.

This probably may not work well with cache infrastructure. See T134592: Allow setting the UI to a language other than English for anonymous users and T149419#2828359

This is not an issue. Changing the language (in the referenced tasks) changes the content, affecting caching.
If this would have an issue with caching, then changing the skin preference would cause the same issue.
Also, using this solution for unregistered users is only a possibility, not the purpose. Setting the cookie is not implemented so far.

Done with CPT review. I'd want SRE and/or Performance-Team to weigh in on whether this would be bad for caching, so I'll leave it to others to do the final review.

In T243920#5856480, @AronManning wrote:

In T243920#5839183, @Bugreporter wrote:

It can be used to set the skin for unregistered readers too if there will be a settings screen or menu for readers.

This probably may not work well with cache infrastructure. See T134592: Allow setting the UI to a language other than English for anonymous users and T149419#2828359

This is not an issue. Changing the language (in the referenced tasks) changes the content, affecting caching.
If this would have an issue with caching, then changing the skin preference would cause the same issue.

This is not true. Skins affect caching too. We at Wikimedia have multiple layers of cache systems. The one this will affect is the CDN final full-rendered page cache (often just called Squid or Varnish, but currently supported by ATS), which is even more critical for site performance for logged-out users than the parser cache.

Also, using this solution for unregistered users is only a possibility, not the purpose. Setting the cookie is not implemented so far.

I don't see much value in this feature for logged-in users.

This is not an issue. Changing the language (in the referenced tasks) changes the content, affecting caching.
If this would have an issue with caching, then changing the skin preference would cause the same issue.

This is not true. Skins affect caching too. We at Wikimedia have multiple layers of cache systems. The one this will affect is the CDN final full-rendered page cache (often just called Squid or Varnish, but currently supported by ATS), which is even more critical for site performance for logged-out users than the parser cache.

Does that mean that the limitations of the infrastructure make it impossible to store any user setting for logged-out users that would affect the generated html, thus only settings used by client-side javascript are possible?

Maybe there is a solution. The info I see on https://www.mediawiki.org/wiki/Manual:Varnish_caching suggests me that cache is bypassed for logged-in users, I assume identified by one of the centralauth_* cookies, or any other session cookie. In the presence of a non-empty 'useskin' cookie the expected behaviour would be the same, as if the user was logged in. If the cookies that don't trigger bypassing the cache are whitelisted, then this cookie won't be on that list, thus resulting in the logged-in behaviour. If the cookies that do trigger the bypass are blacklisted, then 'useskin' can be added to that list. I guess it is the first (whitelist) as that's a significantly shorter list than the blacklist.
It seems to me there is a solution, that possibly needs nothing to be done.

If Varnish is configured with a cookie whitelist, this feature will work as expected: as if the client was a logged-in user.
If it's configured with a blacklist and 'useskin' is not added to it, then it would be -- theoretically -- possible that a client sets this cookie, requests a low-traffic page that's not cached, the page is generated with a skin that is not Vector and cached like that. The next visitor(s) until the cache expires will see a page with an unfamiliar interface. That could become a form of systematic trolling for very bored people affecting low-visibility pages. Easy to avoid tho by adding the cookie to the list.

So the question is: will Varnish serve a cached page if this cookie is set, and will it save a page generated with this cookie?

Also, using this solution for unregistered users is only a possibility, not the purpose. Setting the cookie is not implemented so far.

I don't see much value in this feature for logged-in users.

Yes, it's not for logged-in users, but useful for testing while logged in for us. By making sure that Varnish bypasses the cache when this cookie is not empty (which might be the default behaviour), this can be used even by logged-out users and a simple skin chooser can be added. If that feature would be implemented and become popular, caching the result pages per skin should also be considered.
This is not the target of this patch though. At this stage, it's only for developers.

In T243920#5857412, @AronManning wrote:

This is not an issue. Changing the language (in the referenced tasks) changes the content, affecting caching.
If this would have an issue with caching, then changing the skin preference would cause the same issue.

This is not true. Skins affect caching too. We at Wikimedia have multiple layers of cache systems. The one this will affect is the CDN final full-rendered page cache (often just called Squid or Varnish, but currently supported by ATS), which is even more critical for site performance for logged-out users than the parser cache.

Does that mean that the limitations of the infrastructure make it impossible to store any user setting for logged-out users that would affect the generated html, thus only settings used by client-side javascript are possible?

Yes. This is intentional, and has saved our donors tens of millions of dollars worth of hardware.

Note also that our performance and performance perception requirements preclude JS-executed preferences which result in a reflow, which changing the skin post-paint defintely would.

[Snip]

So the question is: will Varnish serve a cached page if this cookie is set, and will it save a page generated with this cookie?

Yes, it is definitely true that we could add this cookie to the allow list for varnish cache splitting (it won't automatically). But that's a potentially huge cost driver, of a scale that would need to go through a huge amount of management review (not sure about exact costs this would incur, but above a certain threshold it has to go through the Board).

Also, using this solution for unregistered users is only a possibility, not the purpose. Setting the cookie is not implemented so far.

I don't see much value in this feature for logged-in users.

Yes, it's not for logged-in users, but useful for testing while logged in for us. By making sure that Varnish bypasses the cache when this cookie is not empty (which might be the default behaviour), this can be used even by logged-out users and a simple skin chooser can be added. If that feature would be implemented and become popular, caching the result pages per skin should also be considered.
This is not the target of this patch though. At this stage, it's only for developers.

I'm sorry, but the amount of work involved just "for developers" is not justified. Even if this is just a toy patch for developers, any new cookie needs to go through Legal.

In T243920#5857823, @Jdforrester-WMF wrote:

Yes, it is definitely true that we could add this cookie to the allow list for varnish cache splitting (it won't automatically). But that's a potentially huge cost driver, of a scale that would need to go through a huge amount of management review (not sure about exact costs this would incur, but above a certain threshold it has to go through the Board).

We aren't talking about turning this into a proper user feature, in which case the readers choosing not-Vector as their skin might be more than the logged-in users in the same timeframe, that would be noticable in server load.
Without a proper UI to enable this, the users could be in the order of a dozen or less, thus insignificant. So, I don't see performance concerns with this.

I'm sorry, but the amount of work involved just "for developers" is not justified. Even if this is just a toy patch for developers, any new cookie needs to go through Legal.

Legal too? I'm interested, how that works for other matters too. As we don't set this cookie, I don't think there would be any problem with it. If they give a green light, the only thing to do would be to add it to the allow list and merge the patch.

Without a proper UI to enable this, the users could be in the order of a dozen or less, thus insignificant. So, I don't see performance concerns with this.

What's stopping a browser extension from discovering this is possible, setting the cookie and then the extension becoming widely adopted? That then becomes a feature.

Legal too?

Yes. All cookies are documented on https://foundation.wikimedia.org/wiki/Cookie_statement and have gone through legal review.

I don't think there would be any problem with it. If they give a green light, the only thing to do would be to add it to the allow list and merge the patch.

You are making some assumptions here about what this involves. Our legal team is very busy. :)

It can be used to set the skin for unregistered readers too if there will be a settings screen or menu for readers.

Essentially what you are proposing is to vary HTML returned by the server based on cookie. This comes up often in all sorts of form. Such a change requires a good understanding of our caching layer.

Recently we considered this for A/B testing (which I'd say is more advantageous than "testing purposes" - at our scale that's not a strong enough reason when other options exist!). T233609 is the most recent of these that I'm aware of it and it has lots of useful reasons to why we wouldn't do this. I'd recommend a read through.

In general, noticing this change and some other patches you've posted that I consider non-trivial - as a rule of thumb I recommend talking through problems on Phabricator and getting a thumbs up from at least one person in writing before writing any such patch. There's a wealth of knowledge in our community which will happily be shared if you ask the right questions before writing patches and it can save everyone a lot of time! You can also mark your patches as WIP in both the commit message and using the WIP button in Gerrit to indicate to people you are looking for discussion not code review. Talk is cheap!

In T243920#5858829, @Jdlrobson wrote:

Without a proper UI to enable this, the users could be in the order of a dozen or less, thus insignificant. So, I don't see performance concerns with this.

What's stopping a browser extension from discovering this is possible, setting the cookie and then the extension becoming widely adopted? That then becomes a feature.

That's a reasonable speculation. I wonder how many readers and requests it takes to be noticeable performance-wise. There are - I don't know exactly - how many thousand logged-in users connecting every day. This to be noticeable about that many readers should use that feature. Maybe thousands or hundreds of thousands of requests every day? I don't see that as a likely possibility, at least not in a short timeframe, for ex. a year. If that would be noticeable it would be a clear request for a proper feature, that should be implemented efficiently on that scale. That would be the reader-oriented approach. While it is a faint possibility, I find it very unlikely that so many unregistered readers would find such an extension and use it. People are used to the Vector skin.

You are making some assumptions here about what this involves. Our legal team is very busy. :)

My thinking is very solution-oriented and this cookie obviously has no legal concerns compared to say the GeoIP we send every time. Unless proven otherwise I assume no unnecessary bureaucratic struggles.

Recently we considered this for A/B testing (which I'd say is more advantageous than "testing purposes" - at our scale that's not a strong enough reason when other options exist!). T233609 is the most recent of these that I'm aware of it and it has lots of useful reasons to why we wouldn't do this. I'd recommend a read through.

Thanks, will do.

You can also mark your patches as WIP in both the commit message and using the WIP button in Gerrit to indicate to people you are looking for discussion not code review. Talk is cheap!

That was my intent with the patches. I was unaware of the local meaning of WIP, I'll mark these now.

@Demian: Why has this suddenly become high priority?

In T243920#6603986, @Aklapper wrote:

@Demian: Why has this suddenly become high priority?

There was no engagement for 8 months. Without this links cannot be navigated while working on the non-default skin / skinversion. Typing the extra URL param turns a 1 second action into 10-20. Major bottleneck in Developer Productivity.
Please help progress this ticket by adding appropriate project tags and finding developers who can participate.