These patches only block TemplateStyles deployment if the decision in T155813 to not block TemplateStyles deployment on deduplication is being reversed.

Further, after some of the discussion with @Krinkle on T160563, I'm actually leaning towards not needing these patches to do the TemplateStyles deduplication since RL modules are reportedly not expensive to load multiple times after all. But I haven't had time to write new code. Maybe next week, if I finish gerrit:380669, don't get reviews to start updating extensions, and don't have too much other code to review.

That's not to say the patches are useless. https://gerrit.wikimedia.org/r/#/c/340768 is still useful for T112474, and https://gerrit.wikimedia.org/r/#/c/347442/ might still be useful for T160690.

The new patches needing review are:

• 393260: ParserOutput: Add stateless transforms to getText() (Krinkle proposed this idea a while back)
• 393261: Use ParserOutput stateless transforms
• 393284: ParserOutput: Add 'deduplicateStyles' post-cache transformation - The patch to do the deduplication in core
• 393285: Deduplicate embedded style rules - TemplateStyles patch to use it
• 393283: ApiMobileView: Disable 'deduplicateStyles' ParserOutput::getText() transformation - To avoid breaking MobileFrontend's action=mobileview if the above are merged

Thank you, @Anomie for the new patches for review!

Could you please confirm what remaining work is? It looks to me that:

https://gerrit.wikimedia.org/r/#/c/393284/
get response from @cscott re: your response to concern from @Krinkle: "Parsoid and VE are the only things I can think of that might have use for them, depending on how they decide to process the page. Let's ask them."

https://gerrit.wikimedia.org/r/#/c/393285/:
It's not clear to me what next steps are here. Is it to get review from @Krinkle or @Catrope ?

https://gerrit.wikimedia.org/r/#/c/393283/:
Ask if @ovasileva can prioritize review from @pmiazga

Many thanks!

In T180817#3814081, @ggellerman wrote:

https://gerrit.wikimedia.org/r/#/c/393284/
get response from @cscott re: your response to concern from @Krinkle: "Parsoid and VE are the only things I can think of that might have use for them, depending on how they decide to process the page. Let's ask them."

That'd work.

https://gerrit.wikimedia.org/r/#/c/393285/:
It's not clear to me what next steps are here. Is it to get review from @Krinkle or @Catrope ?

Review from someone, anyway. They're good candidates.

https://gerrit.wikimedia.org/r/#/c/393283/:
Ask if @ovasileva can prioritize review from @pmiazga

Probably anyone could review that, it seems simple enough.

Thanks, @Anomie ! For https://gerrit.wikimedia.org/r/#/c/393285/ does the reviewer need to be a ResourceLoader maintaniner or could it be anyone?

@cscott could you have a look at @Anomie 's question in https://gerrit.wikimedia.org/r/#/c/393284/?

In T180817#3814906, @ggellerman wrote:

Thanks, @Anomie ! For https://gerrit.wikimedia.org/r/#/c/393285/ does the reviewer need to be a ResourceLoader maintaniner or could it be anyone?

That patch has nothing to do with ResourceLoader. There's also now https://gerrit.wikimedia.org/r/#/c/394369/, which also doesn't have anything to do with ResourceLoader.

@Anomie Thanks for the clarification. I'm looking for reviewers for patches that don't involve ResourceLoader:

https://gerrit.wikimedia.org/r/#/c/394369/
https://gerrit.wikimedia.org/r/#/c/393285/
https://gerrit.wikimedia.org/r/#/c/393283/

@Tgr has agreed to review:

https://gerrit.wikimedia.org/r/#/c/394369/
https://gerrit.wikimedia.org/r/#/c/393285/
https://gerrit.wikimedia.org/r/#/c/393283/

Thanks!!

The task description contained a somewhat random selection of patches so I updated to include them all. Not sure about ResourceLoader: Add wildcard modules - that was a dependency for the now-abandoned Deduplicate embedded style rules. Is it still the plan to make TemplateStyles pages into RL modules eventually? Or are star modules something we consider nice to have in general, but not related to TemplateStyles anymore?

Something that might be nice to have, but aren't needed here.

@Tgr How can I help? Would you like me to try to find RL maintainers to review patches under "accurate edit previews" in the description or is there somewhere else I should start? Thanks!

In T180817#3950055, @ggellerman wrote:

@Tgr How can I help? Would you like me to try to find RL maintainers to review patches under "accurate edit previews" in the description or is there somewhere else I should start? Thanks!

Yeah, https://gerrit.wikimedia.org/r/#/c/340768/ would be the next step (the other patch I can review).

@Catrope @Krinkle

Are either of you available to review https://gerrit.wikimedia.org/r/#/c/340768/ ? Thanks!

@Krinkle

I checked with @Catrope who thought that you would be more qualified to review this patch:
https://gerrit.wikimedia.org/r/#/c/340768/

Do you have any time to take a look?

Many thanks!

In T180817#3964801, @ggellerman wrote:

@Catrope @Krinkle

Are either of you available to review https://gerrit.wikimedia.org/r/#/c/340768/ ? Thanks!

Yep, I can take this one. @ggellerman Just one thing I'd like before I review this, which is a short description of what this is needed for within TemplateStyles, and brief steps to being able to see the feature in action. E.g. apply this patch in core, and apply patch X (which one) of TemplateStyles, and then do X/Y to see the feature working. If the dependant code hasn't been written yet, then that's also fine, just let me know. I can still review it without that.

TemplateStyles doesn't need that patch anymore. It would have been needed had we still been planning on using RL to load the styles (either directly or https://gerrit.wikimedia.org/r/#/c/347441/), but that's no longer the case.

It's still useful for T112474: Allow previewing a specific page when editing site JS/CSS (e.g. Common.css, Vector.js), though. IIRC you can test it by editing something like MediaWiki:Common.css to do something obvious and previewing. There's also https://gerrit.wikimedia.org/r/#/c/340769/ to let MediaWiki-extensions-TemplateSandbox be used for such edits.

Thanks, @Krinkle !

Will review priority and timeline with @Deskana on 2018-02-27 and any other questions with @Tgr and will get back to you.

Thanks for clarification @Anomie

Oh, right, I completely forgot that the excludepage stuff depends on loading template styles as RL modules. So, since the TemplateStyles parser tag calls Parser::fetchCurrentRevisionOfTitle, that means it's already compatible with TemplateSandbox, right?

Nevertheless, I think that patch is still highly valuable, as it makes it easier to test gadget changes (and changes to sitewide styles, which will be required as part of the TemplateStyles migration, so it's not completely unrelated). Though I might be biased as I was hoping to use this for T187749 :)

In T180817#4004190, @Tgr wrote:

Oh, right, I completely forgot that the excludepage stuff depends on loading template styles as RL modules. So, since the TemplateStyles parser tag calls Parser::fetchCurrentRevisionOfTitle, that means it's already compatible with TemplateSandbox, right?

Yes.

Nevertheless, I think that patch is still highly valuable, as it makes it easier to test gadget changes (and changes to sitewide styles, which will be required as part of the TemplateStyles migration, so it's not completely unrelated).

That's a good point.

Having a task for an unspecified and constantly changing number of patches is not great for project management so let's split this up and make the current task to be about CSS previews which was the most important remaining issue.

As for the rest, 393263 (core): Hard-deprecate ParserOutput stateful transform methods is tech debt cleanup that doesn't really need a task; filed T188443: Wrap indicators in mw-parser-output class for 352835 (core): Add mw-parser-output to indicators container; and 347442 (core): ResourceLoader: Add wildcard modules is not related to TemplateStyles anymore but probably related to the OOUI-for-mobile project so it can be handled there.

I agree that the excludepage functionality is still valuable. It transforms an existing MediaWiki core feature (preview of user js) which is implemented with hacks in various places, with something that more generic that acknowledges the need to be able to temporally replace the content of one or more wiki pages of a ResourceLoaderWikiModule during a preview.

And as part of implementing that functionally, various bugs as a result of the old implementation being a hack, will naturally be solved and work as expected. Such as T112474, from 2015.

However, I don't think that bug is currently formalised as a quarterly or annual priority, and it seems too big for as an "interrupt" task to take on at this point within the current quarter. I'd have to cancel or delay other work in favour of it.

I think it makes sense for TemplateStyles, as a product, to focus on gadget previewing improvements, but I suggest getting that in for the next quarter and making sure Performance Team has time set aside for supporting on that feature, as it depends on coordination within ResourceLoader. The next quarter is only a few weeks from now, so it won't delay by much, but let's circle back to this at that time.

That way, I'll be able to do the code review, but beyond that also better assess the expected impact on future maintenance of ResourceLoader, and better weigh the different design choices and options. I'd like to emphasise that I'm not worried about code quality, someone else could easily take over the review of this patch if that was the (only) concern. My main concerns are of long-term impact and about the API design fitting well within ResourceLoader's overall model. And for that, I need more time than I can spare in a few hours today or next week.

See {T186391}, T60291: load.php should check read permissions on private wikis (exposes content of wiki pages part of a module) and T186390: Javascript preview feature broken when live previews are enabled for somewhat related discussion.

Thanks, @Krinkle ! Re: schedule- some time in the next few weeks would be good. No need to worry about finishing in the next week.

@ggellerman Just checking on this - Does Reading Infra need to do anything now or are they ok to move this to Tracking? Thanks!

@LGoto I think that this is still with @Krinkle so moving it to Tracking on Reading Infra board makes sense to me.

@Krinkle do you think that you'll have time to look at 340768 (core): Generalize ResourceLoader 'excludepage' functionality in Q4? Thanks!

Yes.

@Krinkle Many thanks!

Review is almost finished, but currently stalled as the review so far uncovered bug in ResourceLoader that needs to be addressed first, in order for the main patch to work as expected. I've submitted a patch to fix said bug, at https://gerrit.wikimedia.org/r/416619. This is pending review by @Anomie.

Thanks for the update @Krinkle ! @Anomie will you have a chance to look at this soon? Thanks!

Hopefully. It's already on my list.

Thanks, @Anomie!

@Tgr would it make sense to remove:

347442 (core): ResourceLoader: Add wildcard modules (should be reviewed by an RL maintainer; not TemplateStyles related)

from this ticket if it does not related to TemplateStyles? Thanks!

I don't think it matters much, now that it's resolved.

@Tgr - great news for TemplateStyles that the ticket is resolved :)

That said, the Resolved status does not reflect state of 347442 (core): ResourceLoader: Add wildcard modules (should be reviewed by an RL maintainer; not TemplateStyles related) which is still open. So I would advocate for tracking that patch in a different ticket.

Thank you, @Krinkle !