r77516 MediaWiki - Code Review archive

I don't think this makes any sense. This always add the *current version* links, regardless of the right side of the diff. Testing confirms this.

Also, this could require a parse (on cache miss) before even the AJAX based preview placeholder div is added, which defeats the purpose of deferring the loading of the preview after the rest of the page.

Since the output location of category/IW links is controlled by the skin, I think it's non-trivial to AJAX those in. Perhaps all the AJAX diff code should just be reverted. Also, reverting the changes in how JS/CSS is added to pre-r75331 would restore the CSS load order (custom scripts after extensions).

Hrm....it took me a sec to figure out what you meant by this, but now I see it.

This seems like a problem with a reasonable solution, if not a perfect one. It seems that for both category and interwiki links, we could append a rendering of them to the content section rather than try to inject them into the proper place in the skin. While this wouldn't be a perfect rendering, it might actually be more useful for review purposes, because I'm going to bet that most reviewers don't even think to look in the sidebar for content related stuff.

Even if you add the links to the HTML in a way that is not correct for some/all skins, you'd still have to do a parse in order to get those links to begin with (with the exception of the current revision, since there are DB tables for that data). Of course, we'd want the parse to be the API parse called via AJAX, but it doesn't support any parameters that make it output an HTML (rendered) fragment of category/IW links. Doing so would require either ugly JS code to render them, a separate AJAX call to some PHP function that returns this (and somehow avoids a reparse), changing the API 'parse' module, or using a custom FlaggedRevs server-side function to use for the parse (rather than the API). All of these involve ugly code and duplication.

You'll have a really difficult time convincing me that anything is uglier than going back to the unacceptably slow rendering of diff pages, especially given our stance on bug 24315 ("Special:OldReviewedPages always uses diffonly=0, ignoring user preferences").

So, let's explore how ugly our options really are. These aren't mutually exclusive option

Option #1 - continue down the current path. Assuming this path, I think changing the API parse module is our best strategy, adding an option to provide easily parsable interwiki/category links (assuming they aren't already available), and then rendering that data somehow via Javascript (perhaps with another call to the API, but providing the data to the API rather than asking for another parse). This isn't a fantastic option, but it's not so repulsive as to be completely unacceptable.

Option #2 - cache oldrevs. We could limit this to recent (e.g. past week) and expensive (e.g. >2s parse time) revisions, which would hopefully keep us from overwhelming the cache with too much new stuff.

Option #3 - make this a user configurable feature, either default-on or per-wiki default. I really don't like adding yet more options to the preferences, and I really don't like adding per-wiki global options. However, it's clear enwiki really needs default-on, since Pending Changes seems to only be configured on expensive-to-render pages. For dewiki, plwiki and others that have it on for all pages, one could argue either side.

Option #4 - revisit bug 24315. We can't continue to force people to endure page parses even in the case where they've explicitly hunted down the "diffonly" user pref. I don't really think this is much of a solution, but I'm throwing this out there as the least we can do.

What should we do here?

Rendering iw/cat links through API calls is also necessary for a quick preview feature, which I assume we will want at some point.

action=parse can give you also categories and interwikilinks from the rendered page. Use prop=text|categories|langlinks. The HTML-Style is missing, that is right.