r111844 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r111843‎ | r111844 | r111845 >
Date:21:01, 18 February 2012
Author:au
Status:reverted (Comments)
Tags:
Comment:
* Always sort attributes (+1 test pass).

The performance impact for .sort is quite small (12.079s => 12.158s)
and Sanitizer is probably one of the more accessible places to do this.
Modified paths:
  • /trunk/extensions/VisualEditor/modules/parser/ext.core.Sanitizer.js (modified) (history)

Diff [purge]

Index: trunk/extensions/VisualEditor/modules/parser/ext.core.Sanitizer.js
——@@ -103,6 +103,9 @@
104104 kv.v = this.checkCss(kv.v);
105105 }
106106 }
 107+ token.attribs.sort(function(a, b) {
 108+ return a.k.localeCompare(b.k);
 109+ });
107110 }
108111 // XXX: Validate attributes
109112 return { token: token };

Comments

#Comment by GWicke (talk | contribs)   21:21, 20 February 2012

Sorting the attributes adds some more round-trip normalization, which again makes it harder to avoid dirty diffs.

I see a few options:

  • Remember the order in dataAttribs
  • Sort the attributes before comparing the html in parserTests, and avoid the reordering in the normal case
  • Always sort, and thus take the hit once but simplify matters later (what you just added)

The DOM spec does not guaranteed the preservation of order, but in practice most implementations seem to keep the order intact. So I guess all three options are possible. I would favor simply not touching the order for now, as the only motivation we have for it right now is to get a parser test to pass. Easier to just whitelist that test IMO.

#Comment by Au~mediawikiwiki (talk | contribs)   22:26, 20 February 2012

Concurred. This is now reverted in r111971 and whitelisted in r111972.

Status & tagging log