Page MenuHomePhabricator

Review TemplateStyles patches related to previewing site CSS changes
Closed, ResolvedPublic

Description

Review patches required for previewing site CSS changes

other patches which used to be part of this task, but not anymore:

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application removed a project: Patch-For-Review. · View Herald TranscriptNov 17 2017, 5:42 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
ggellerman added a subscriber: Tgr.Nov 17 2017, 5:44 PM
Anomie added a subscriber: Anomie.Nov 17 2017, 6:06 PM

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.

Anomie added a comment.EditedNov 24 2017, 9:43 PM

The new patches needing review are:

Krinkle updated the task description. (Show Details)Nov 28 2017, 8:40 PM

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!

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/?

Anomie added a comment.Dec 6 2017, 4:30 PM

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/

TheDJ awarded a token.Dec 7 2017, 2:41 PM
Tgr renamed this task from Review patches that block TemplateStyles deployment to Review outstanding TemplateStyles patches.Jan 30 2018, 12:37 AM
Tgr updated the task description. (Show Details)Jan 30 2018, 11:21 PM
Tgr added a comment.Jan 30 2018, 11:25 PM

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?

Tgr updated the task description. (Show Details)Jan 30 2018, 11:26 PM

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

Tgr updated the task description. (Show Details)Feb 2 2018, 12:32 AM
Tgr updated the task description. (Show Details)Feb 2 2018, 1:03 AM
Tgr updated the task description. (Show Details)Feb 2 2018, 2:52 AM
Tgr updated the task description. (Show Details)Feb 2 2018, 6:49 PM
Tgr changed the task status from Open to Stalled.Feb 5 2018, 1:27 AM

Every patch is either merged, +1'd and waiting for a dependency, waiting for a review from an RL maintainer, or has an issue that needs to be fixed.

@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!

Tgr updated the task description. (Show Details)Feb 11 2018, 7:17 AM
Tgr added a comment.Feb 11 2018, 7:33 AM

@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).

@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!

@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.

Krinkle claimed this task.Feb 26 2018, 10:45 PM

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

Tgr added a comment.Feb 26 2018, 11:36 PM

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 :)

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.

ggellerman changed the task status from Stalled to Open.Feb 27 2018, 6:52 PM
ggellerman triaged this task as Normal priority.
ggellerman moved this task from Up next to Doing on the TemplateStyles board.
Tgr renamed this task from Review outstanding TemplateStyles patches to Review TemplateStyles patches related to previewing site CSS changes.Feb 27 2018, 9:16 PM
Tgr updated the task description. (Show Details)
Tgr added a comment.Feb 27 2018, 9:25 PM

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.

Tgr updated the task description. (Show Details)Feb 27 2018, 9:25 PM

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.

Tgr added a comment.Feb 27 2018, 11:20 PM

See {T186391}, {T60291} 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 moved this task from Doing to Up next on the TemplateStyles board.Mar 27 2018, 6:12 PM
LGoto added a subscriber: LGoto.Apr 2 2018, 8:12 PM

@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!

ggellerman moved this task from Up next to Doing on the TemplateStyles board.Apr 11 2018, 4:57 PM

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.

Tgr updated the task description. (Show Details)Apr 25 2018, 7:52 AM
Tgr closed this task as Resolved.Apr 25 2018, 10:16 AM
Tgr awarded a token.
Deskana moved this task from Doing to Done on the TemplateStyles board.Apr 25 2018, 10:25 AM

@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!

Tgr added a comment.Apr 26 2018, 11:10 AM

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.

Krinkle updated the task description. (Show Details)Apr 27 2018, 7:56 PM
Krinkle updated the task description. (Show Details)