Thank you, I will schedule this as a Q1 goal for our team.

Q1 should be fine as long as the performance review is not a blocker for production deployment. Our goal is to have the API Portal, which this skin is a requirement for, live by the end of Q4 this FY.

That's fine, as long as you take ownership of emergency fixes that might have to happen if performance degrades in a major way when your feature is deployed. That includes full undeployment if major issues aren't fixed rapidly.

The goal of performance reviews, and their appropriate scheduling: https://www.mediawiki.org/wiki/Wikimedia_Performance_Team/Performance_Review is to avoid putting you in a position of failure where a feature deployment causes an incident and has to be undone.

In the future, if you want faster performance review, you must request them before a quarter involving a feature deployment starts. A skeleton task is fine, we simply have to take it into account in our own capacity planning.

Understood. Thank you.

As requested by @CCicalese_WMF, we have updated our test system to the latest versions of Chameleon, Bootstrap and SCSS. Please feel free to use the site for your purposes.

See https://apigateway.wmf.labs.hallowelt.biz/wiki/Special:Version

• Skin:Chameleon: 3.0.0-alpha (29848fb) 02:56, 12 May 2020
• Extension:Bootstrap: 4.2 (1bf5723) 22:46, 9 May 2020
• Lib: mediawiki/scss: 2.0.0

A quick incomplete review with some questions to help get on our way:

1. Will the wiki use the "wikimediaapiportal" skin (as the test instance does now) or "chamelon" skin (indicated in the task)?
2. The visual style that this portal tries to resemble is known as wikimedia-ui-base (design.wm.o). These colors and proportions are available as a LESS file that you could embed and use directly. By doing that instead of using Bootstrap and Sass/SCSS you'd likely reduce maintenance cost and overally complexy by 90%, as all as significantly reduce the amount of code to review for security and performance, and naturally improve both backend and frontend perf in the process. I noticed various bugs, performance and code quality issues in SCSS extension, but I'll leave it at that for now pending result of question 1.
3. Is there a representative example page that show-cases the kind of content that this will host? Or will it mainly be standard wikitext constructs with no custom JS? (Or will that be done as part of another perf review later, separate from the skin?).

In T252462#6131830, @Krinkle wrote:

1. Will the wiki use the "wikimediaapiportal" skin (as the test instance does now) or "chamelon" skin (indicated in the task)?

It'd certainly be useful to know this... And if the "wikimediaapiportal" skin is also due to be deployed "soon"... Where as the performance review and security review tasks for that?

Thank you for your initial assessment, @Krinkle. Responses:

1. The API Portal wiki will use the Chameleon skin enhanced by additional customization from WikimediaApiPortal. There was some discussion about whether WikimediaApiPortal should be classified as a skin or an extension. It does not stand by itself as a skin, but its function is purely related to skinning. Development of WikimediaApiPortal should be complete by May 22 at which point it will be submitted for security review and can be included in this performance review.

2. The use of Chameleon is only partially about its ability to be customized in colors and font sizes, etc., to match the Wikimedia Design Style Guide. It is also about the general look, menu placement, responsiveness, ability for content customization, and other related look and feel aspects. These are delivered by Chameleon and Bootstrap. Chameleon is a skin frequently used in non-Wikimedia wikis built upon a well-established Bootstrap framework, and as such is quite stable. Of course, if there are issues with the SCSS library, fixes should be contributed to remediate them.

3. A prototype was created and serves as a model for the current development. There is also a documentation plan on mw.o. In particular, see the Prototype section near the bottom of that plan to see links directly to some representative pages in the prototype.

Hi @CCicalese_WMF - I just wanted to send a quick reminder that, per our SOP, we do need a minimum of 30 days prior to a desired deployment date in order to properly resource and perform a Readiness Review.

Let me know if I can be of service or if you have any questions.

@Jcross Did you mean for that comment to be on T246949? That ticket is for the security readiness review. This ticket is for the performance review.

I've been wondering for while where the actual code is for this skin, given that it is not in any of the three repos linked in the task, nor in the skins/WikimediaApiPortal repo (which is empty at time of writing). Based on Special:Version on the demo wiki I see that the wiki runs on an draft commit still in Gerrit (link).

Assuming the request is not for @Reedy or myself to perform the first code review, please let us know when this code is peer-reviewed, merged, and considered stable and ready for review by us.

@Krinkle, the code to be reviewed is listed above in the task description in the Which code to review section (the Chameleon skin, the Bootstrap extension, and the SCSS library). All of the code is currently hosted on github, although there is a task for figure out the best way to mirror the code from github to gerrit. Note that this task does not refer to WikimediaApiPortal, which is not yet ready for review. A separate task will be filed for that at the appropriate time.

I've submitted technical details in the form of a commit message and diff at:

• https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/WikimediaApiPortal/+/602200/
• https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/WikimediaApiPortal/+/602202/

Frontend

The frontend of the skin seems well-designed and well-implemented by the contractors. Both visually, interactively and in terms of LESS/JS code quality. It is responsive as well, and this seems to follow good practices. I will try to write something more detailed later in the review process, once a task for the frontend skin is filed.

Backend

Chameleon HTML templates

The backend is in worse condition I'm afraid. I believe this to be primarily due to the current state of the Chameleon codebase making it infeasible for a skin to be developed in a stable or performant manner. I estimate its code would take months to rewrite and bring into a decent shape that is deployable and able to keep up with MediaWiki master's development cycle. Noting that the Skin system in particular is being overhauled by Reading as we speak, and CPT is also actively refactoring MW core and removing or changing undocumented APIs.

Much of Chameleon duplicates core business logic, or relies on unstable/deprecated mechanisms. Continuing to use it would I believe be very costly to WikimediaApiPortal. In addition it would also impose significant limits on other WMF developments. This because it would mean its codebase must remain compatible with MW master and WMF production on a weekly basis. This would be challenging to do, not in the least due to being two upstreams away from us, and being developed on GitHub.

If MediaWiki did not already have an HTML template engine, perhaps Chameleon's PHP-XML hybrid layout system could be seen as a strategic investment for WMF. However, MediaWiki has adopted Mustache as its HTML template engine. It is worth noting that Mustache requires little to no knowledge of PHP, which was a significant reason for why it was favoured by developers at WMF and the wider technical community. Chameleon on the other hand embraces PHP fully. I think it's important that the result of outsourced work is something we can still debug, contribute to, or even maintain ourselves in the future.

Chameleon also utilizes cross-directional chatter to MW core classes from within individual component code (thus opposite to strategic direction of T140664).

The silver lining is that the verbose nature of Chameleon's layout system, means its instantiation in WikimediaApiPortal is essentially a full description of what the output needs to be - thus making it redundant to call Chameleon. The local description is essentially a complete HTML template.

The first patch above demonstrates how that would work. It unlinks the components from Chameleon's base class, and still produces the same pixel-identical output, using none of Chameleon's code, yet also without having copied its logic.

Chameleon stylesheets

At 367KB, the CSS payload size is quite large. (This is not including core's own styles or the OOUI styles, loaded atop.)

Using the HelloWelt demo as reference point, I opened this in Chrome, started recording CSS code coverage, and interacted with the page as much I could (hover all elements, search suggestions, collapse/expand menus, and repeat for each responsive mode).

After that, it reported that 94% of the stylesheet was unused.

It seems Chameleon's stylesheets are thus largely non-applicable to this skin. This was surprising at first, but looking closer at the default Chameleon and the Portal design this is working toward, it is not entirely surprising. For every minor change to a component, the skin has essentially had to replicate its look locally first. The use of OOUI widgets and WMUI icons also suggest that most likely no styles could apply directly.

In the first patch I also tried unlinking the Chameleon stylesheets. There were small handful of inherited styles that weren't overridden (about 10 lines of code, e.g. the body font family). The rest were unused. With the font set (as the patch does) the result is still pixel-identical.

Bootstrap library

The Bootstrap library is easy to use and lends it self to re-usability and composability. It also has pretty good code quality, and follows good accessibility and performance practices.

I haven't done a full review of it since it's well-known and I've looked at it before. There are a number of custom components here though. It's probably worth to, later in the process, run this this by our design and accessibility experts (e.g. Volker) to make sure the custom Bootstrap and OOUI code is up to the same standards as the originals :)

In terms of performance overhead, using Bootstrap adds a high bandwidth cost for CSS and JavaScript. As with all frameworks of this size, one essentially trades page load cost for "getting to market" quicker. This isn't bad per-se though. We have frequently used Bootstrap at WMF as well (and still do, eg our CI microsite.

Depending on what kind of page load performance we are looking for, and whether mobile usage is expected/important, this might be fine even in the long-term. Especially if it reduces the cost of regular maintenance.

I do see some early signs that the skin might be having to fight against the Bootstrap defaults. Developers tend to intuitively avoid that when possible (to save time), by building new components standalone (instead of derived from something similar-but-different from Bootstrap). Anyway, something to keep an eye on and perhaps revisit a year from now if it ends up slowing down development.

Overall though Bootstrap seems fine. Launching with Bootstrap makes sense. It's a good fit and is actively used.

Monitoring

Given the steep page load cost it adds and the vast difference from other production page views, we'll want to bucket this wiki's page load metrics separately from the rest of Vector/Minerva production. We could also add a synthetic test to our WebPageTest pipeline to monitor its performance over time, so that you'd be know if the customisations do start to outweigh the framework.

SCSS (Sass) and Bootstrap extension

The SCSS extension is fairly small and reviewing it was easy. Although note that this extension is not standalone. It further depends on the scssphp/scssphp composer package, which would also need security and performance review.

I had to disable the SCSS cache locally as its cache does not seem to track the files included by SCSS modules, such as sass file imports from Chameleon. When testing out changes, the same cached value remained in use. For this to work, the module subclass needs to track the files used by the SCSS compiler, and pass them to the 'localFileRefs' member of ResourceLoaderFileModule. From there, ResourceLoader's file hashing takes care of the rest. As-is deployments may cause old stylesheets to remain stuck and become incompatible with new HTML.

Both the HTML template engine and the CSS processor engine are high-risk areas. They will be running over lots of arbitrary user input from search queries, urls, and other request data; and end up producing client-side executable code from a *.wikimedia.org domain. This is far from a "normal" MediaWiki extension that merely builds on top of well-known core concepts that only need to be checked for correct use. Instead, they introduce entirely new primitives and avenues for abuse.

And unfortunately, like with the HTML template, this is solving a problem we have already solved. For MediaWiki, the CSS processor we settled on is LESS. Both Performance and Security review upstream updates to the LESS and Mustache parsers. Doubling this on-going cost would come at the expense of supporting our own product development. I would advise against this (keep in mind my conflict of interest, as maintainer of ResourceLoader and likely the one to end up doing this).

As I understand it "adopt SCSS" and "compile Bootstrap in production" are not themselves key objectives of the API Portal. As such, given we already have infrastructure in place that solves the same problems, I would advise to reuse that whenever possible.

I realise while writing this that SCSS is only used as part of of the Bootstrap's extension re-creation of TWBS upstream's build step – in PHP. Upstream TWBS also publishes their artefacts and build process to NPM, which we can use directly and update following an occasional Bootstrap release. This would save considerable effort, and significantly reduce the upfront cost as well as on-going maintenance cost for API Portal. I've added a second patch (linked above) that demonstrates how that could work. Again, the output is pixel-identical.

This means the SCSS and Bootstrap extensions do not need to be installed.

Thank you for your analysis, @Krinkle. We look forward to discussing this in more detail when we meet next week.

@Gilles This should be now ready for final review.

Is this demo up-to-date? I see that Chameleon is still installed there, and the skin is still at the April 2020 draft version (source). Or is there a different demo wiki I can review instead?

Change 616866 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/skins/WikimediaApiPortal@master] Misc docsand code quality clean ups

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

@Krinkle, the skin is now deployed to and enabled on https://api.wikimedia.beta.wmflabs.org/. It is not yet the default skin, but it can be selected. It will be made the default and only skin in a future config change.

Change 616866 merged by jenkins-bot:
[mediawiki/skins/WikimediaApiPortal@master] Misc docs and code quality clean ups

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