The only dependency the extension does have is (literally) PHP itself:

"require": {
	"php": ">=5.5.9"
}

All other dependencies (Phan, PHP Parallel Lint, Grunt stuff) are dev-only dependencies. Are these a problem for what this security review is about?

"Access Denied: Restricted Task". Sorry, there is really nothing I can do if I'm not allowed to know what you know.

That's rather cryptic for people who don't have admin access to the https://github.com/wikimedia/mediawiki-extensions-TwoColConflict mirror (like most of us). Can you please share the details of this alert?

@Schnark, I think I was able to fix it just a few minutes ago. Can you please try again?

Possible issues I see with the current collapsing algorithm:

• Why are the lines cut off so early? There is plenty of room (depending on the window size). I would like to see more text.
• The cut-off point does not dynamically respond to the browser in any way (window size, font size, zoom). This is a pity.
• Why is the last line not right-aligned?
• The three dots appear like they are part of the text. It's hard, sometimes impossible to tell the difference.
• Shouldn't there be a space separating the dots from the text?

Oh, that's my script. I'm very much interested in fixing it, if there is a problem.

The icon was in a circle before, see https://commons.wikimedia.org/wiki/File:Revisionslider-screenshot-de-large.png. So the fact it became a stray "?" is technically a regression, and the patch technically a bugfix. I went ahead and merged it. Can easily be rolled back in case WMDE-Design doesn't agree.

Notes for @Andrew-WMDE and possibly WMDE-Design:

• Linker::revUserTools is the function that combines the user and tools links in one call.
• MediaWiki core provides messages for hours-ago, minutes-ago, and seconds-ago.
• There is also an ago message we could use together with with the messages for days, weeks, months, and years. However, I suggest to not use these because conflicts that old are super-unlikely, but the code for this would be pretty complex. Instead, I would start showing the formatted date and time when a conflict happened more than 23 hours ago.

While reviewing the browser tests for this, I run into a surprising UX issue. According to the specifications above the reset button (back arrow) only appears in edit mode. I totally expected the button to do what every other reset button I have seen so far does: discard the current edit. But it undoes all edits (to the same paragraph) I have done and saved before.

WMDE, especially the WMDE-FUN-Team might be interested in this as well.

We had a quick chat with @Jan_Dittrich and concluded:

• Making the popups wider (as already discussed in T203066) would make the problem occur less often.
• We suggest to not mess with the scroll behavior, as this is a "standard" OOUI thing.
• When the scrollbar appears, the only action item (the button) disappears at the bottom. Idea: Have the button outside of the scroll area so it's always visible. Does OOUI allow this?

Note that with the investigation and bugfixes we plan to do for T203066: Fix the header overlapping the tour window these scrollbars will become much less of an issue.

A little more investigation:

• OOUI supports dialog (a.k.a. window) sizes "small" (300px), "medium" (500px), "large" (600px), "larger" (900px), and "full" (100%). Currently our code for the tour dialogs sets "large", but this is apparently ignored or not set correctly because the popups end up being 300px. Apparently this is an actual bug we should fix!
• Using the $container option as described in https://www.mediawiki.org/wiki/OOUI/Widgets/Popups#containerExample sounds great. I will continue the investigation @jkroll started.

I had a quick look and I believe this is all totally normal, expected behavior as defined by OOUI. Which of the 3 screenshots you think shows an actual issue, and what would be the expected behavior instead?

I had a quick look at the ResourceLoader module definitions for OOjs, see https://phabricator.wikimedia.org/source/mediawiki/browse/master/resources/Resources.php$2805 and below. My brief impression:

• The module "oojs" does have a scary name. It sounds like it loads the entirety of OOjs. But it doesn't.
• The module "oojs-ui" is the big boy that contains all of "oojs-ui-core", "oojs-ui-widgets", "oojs-ui-toolbars", and "oojs-ui-windows". But this is not used in Advanced-Search, as far as I can see.
• The "oojs-ui-widgets" module can not be split, because it's all in one file.

Even with the fixed .ods my biggest issues with this approach remain: All bars are smaller than before, and there is a huge spike that heavily exaggerates edits with 52 or more bytes, while edits with 51 or less bytes all look identical.

There is an other performance metric, and that is how long the user sees this:

It's hard to describe the current behavior of the patch. It feels somewhat unpredictable. I think we had a very productive IRC session with @Gopavasanth yesterday and he will figure this out.

I mapped the algorithms as graphs via LibreOffice Calc. It might be that I did something wrong, but this is what I get:

I tested with Firefox, and run into some issues, unfortunately:

the problem does appear to be the precision

There are indeed coordinate values in the database the current code can not deserialize any more: https://www.wikidata.org/wiki/Special:Diff/135258698

In T99664#3848557I suggested to apply normalization to Wikimedia Commons file names before they are stored. This would solve a few related issues. Should I open a new ticket for this?

I think tagging does not really need another review, when this was already done on the merged patches. However, I had a brief look and this looks good to me.

Personally, I believe a @private annotation is a hint at a suboptimal architecture. However, having a @private annotation on an otherwise public method is clearly better than not having one. And extra bonus points for having a comment – that's super helpful!

/…/ is the standard, and used in the majority of cases – unless there is a good reason to free the slash from being a special character. A typical example is something like /^http:\/\/\w+\/\w+\//, which is painful and often written as !^http://\w+/\w+/! instead. Alternative delimiters I see quite often in real-life code are !…! and @…@. I assume this is mostly because most developers don't know about the possibility to use {…}. Note this still allows to use the brackets as quantifiers, e.g. {1,2}, which is why I think this is the (almost, because it's uncommon) perfect delimiter.

I would find it very confusing to […] scroll to the end […] and see the arrow facing down again, now having the feeling there is still more to see.

@Hanna_Petruschat_WMDE, please correct me if I'm wrong, but if I remember correctly the style guide asks us to not flip such expand/collapse arrows any more. See T187099#4015542 and T190147 from the Advanced-Search project.

This is really awesome, thank you very much! I think the addition of the summary lines is very helpful, as well as the additional user links.

I support the idea for PHP. From my experience, I can't think of a reason to ever use !! in PHP. So as long as it's an auto-fix sniff, go for it.