jenkins-bot has submitted this change and it was merged.
Change subject: Replace ve.dm.Model#matchesAttributeSpec
......................................................................
Replace ve.dm.Model#matchesAttributeSpec
Attribute matcher specifications can take any form from the list below:
* Object with an array 'whitelist' property, containing any mix of booleans,
regular expressions, and strings.
* Object with an array 'blacklist' property (same composition as above).
* Object with both 'whitelist' and 'blacklist' properties (ditto).
* Array (ditto).
* Regular expression.
* Boolean.
This flexibility is purchased at the cost of a lot of branching in a very hot
code path (ve.dm.Model#matchesAttributeSpec gets called for every property of
every element).
This patch allows attribute specifications to be either a boolean value or a
function which takes an attribute name and returns a boolean value. This allows
for dynamic and complex specifications without incurring costs for the common
cases.
Change-Id: Ia0e0fe4fc2385dab67c816148cfc362a39413dac
---
M build/modules.json
M src/ce/ve.ce.View.js
M src/dm/nodes/ve.dm.BlockImageNode.js
M src/dm/nodes/ve.dm.TableCellNode.js
M src/dm/ve.dm.Converter.js
M src/dm/ve.dm.Model.js
D tests/dm/ve.dm.Model.test.js
M tests/index.html
8 files changed, 24 insertions(+), 193 deletions(-)
Approvals:
Catrope: Looks good to me, approved
jenkins-bot: Verified
diff --git a/build/modules.json b/build/modules.json
index 07a9752..ba37a62 100644
--- a/build/modules.json
+++ b/build/modules.json
@@ -458,7 +458,6 @@
"tests/dm/ve.dm.SurfaceFragment.test.js",
"tests/dm/ve.dm.ModelRegistry.test.js",
"tests/dm/ve.dm.MetaList.test.js",
- "tests/dm/ve.dm.Model.test.js",
"tests/dm/lineardata/ve.dm.FlatLinearData.test.js",
"tests/dm/lineardata/ve.dm.ElementLinearData.test.js",
"tests/dm/lineardata/ve.dm.MetaLinearData.test.js",
diff --git a/src/ce/ve.ce.View.js b/src/ce/ve.ce.View.js
index 4a4e98a..168f51c 100644
--- a/src/ce/ve.ce.View.js
+++ b/src/ce/ve.ce.View.js
@@ -83,14 +83,16 @@
* @property {boolean|string|RegExp|Array|Object}
* @inheritable
*/
-ve.ce.View.static.renderHtmlAttributes = [
- 'abbr', 'about', 'align', 'alt', 'axis', 'bgcolor', 'border',
'cellpadding', 'cellspacing',
- 'char', 'charoff', 'cite', 'class', 'clear', 'color', 'colspan',
'datatype', 'datetime',
- 'dir', 'face', 'frame', 'headers', 'height', 'href', 'id', 'itemid',
'itemprop', 'itemref',
- 'itemscope', 'itemtype', 'lang', 'noshade', 'nowrap', 'property',
'rbspan', 'rel',
- 'resource', 'rev', 'rowspan', 'rules', 'scope', 'size', 'span', 'src',
'start', 'style',
- 'summary', 'title', 'type', 'typeof', 'valign', 'value', 'width'
-];
+ve.ce.View.static.renderHtmlAttributes = function ( attribute ) {
+ return ve.indexOf( attribute, [
+ 'abbr', 'about', 'align', 'alt', 'axis', 'bgcolor', 'border',
'cellpadding', 'cellspacing',
+ 'char', 'charoff', 'cite', 'class', 'clear', 'color',
'colspan', 'datatype', 'datetime',
+ 'dir', 'face', 'frame', 'headers', 'height', 'href', 'id',
'itemid', 'itemprop', 'itemref',
+ 'itemscope', 'itemtype', 'lang', 'noshade', 'nowrap',
'property', 'rbspan', 'rel',
+ 'resource', 'rev', 'rowspan', 'rules', 'scope', 'size', 'span',
'src', 'start', 'style',
+ 'summary', 'title', 'type', 'typeof', 'valign', 'value', 'width'
+ ] ) !== -1;
+};
/* Methods */
diff --git a/src/dm/nodes/ve.dm.BlockImageNode.js
b/src/dm/nodes/ve.dm.BlockImageNode.js
index 9b8bd15..18b270b 100644
--- a/src/dm/nodes/ve.dm.BlockImageNode.js
+++ b/src/dm/nodes/ve.dm.BlockImageNode.js
@@ -37,8 +37,8 @@
ve.dm.BlockImageNode.static.name = 'blockImage';
-ve.dm.BlockImageNode.static.preserveHtmlAttributes = {
- blacklist: [ 'src', 'width', 'height', 'href' ]
+ve.dm.BlockImageNode.static.preserveHtmlAttributes = function ( attribute ) {
+ return ve.indexOf( attribute, [ 'src', 'width', 'height', 'href' ] )
=== -1;
};
ve.dm.BlockImageNode.static.handlesOwnChildren = true;
diff --git a/src/dm/nodes/ve.dm.TableCellNode.js
b/src/dm/nodes/ve.dm.TableCellNode.js
index ba84d5b..5b52d13 100644
--- a/src/dm/nodes/ve.dm.TableCellNode.js
+++ b/src/dm/nodes/ve.dm.TableCellNode.js
@@ -39,8 +39,8 @@
ve.dm.TableCellNode.static.matchTagNames = [ 'td', 'th' ];
// Blacklisting 'colspan' and 'rowspan' as they are managed explicitly
-ve.dm.TableCellNode.static.preserveHtmlAttributes = {
- blacklist: ['colspan', 'rowspan']
+ve.dm.TableCellNode.static.preserveHtmlAttributes = function ( attribute ) {
+ return attribute !== 'colspan' && attribute !== 'rowspan';
};
/* Static Methods */
diff --git a/src/dm/ve.dm.Converter.js b/src/dm/ve.dm.Converter.js
index 74ee194..025a3e3 100644
--- a/src/dm/ve.dm.Converter.js
+++ b/src/dm/ve.dm.Converter.js
@@ -134,16 +134,16 @@
* @static
* @param {HTMLElement[]} originalDomElements Array of DOM elements to render
from
* @param {HTMLElement[]} targetDomElements Array of DOM elements to render
onto
- * @param {boolean|string|RegExp|Array|Object} [spec=true] Attribute
specification, see ve.dm.Model
+ * @param {boolean|Function} [filter=true] Attribute filter
* @param {boolean} [computed=false] If true, use the computed values of
attributes where available
* @param {boolean} [deep=false] Recurse into child nodes
*/
-ve.dm.Converter.renderHtmlAttributeList = function ( originalDomElements,
targetDomElements, spec, computed, deep ) {
+ve.dm.Converter.renderHtmlAttributeList = function ( originalDomElements,
targetDomElements, filter, computed, deep ) {
var i, ilen, j, jlen, attrs, value;
- if ( spec === undefined ) {
- spec = true;
+ if ( filter === undefined ) {
+ filter = true;
}
- if ( spec === false ) {
+ if ( filter === false ) {
return;
}
@@ -158,7 +158,7 @@
for ( j = 0, jlen = attrs.length; j < jlen; j++ ) {
if (
!targetDomElements[i].hasAttribute(
attrs[j].name ) &&
- ( spec === true ||
ve.dm.Model.matchesAttributeSpec( attrs[j].name, spec ) )
+ ( filter === true || filter( attrs[j].name ) )
) {
if ( computed &&
ve.dm.Converter.computedAttributes.indexOf( attrs[j].name ) !== -1 ) {
value =
originalDomElements[i][attrs[j].name];
@@ -168,7 +168,7 @@
targetDomElements[i].setAttribute(
attrs[j].name, value );
}
- if ( spec === true || ve.dm.Model.matchesAttributeSpec(
attrs[j].name, spec ) ) {
+ if ( filter === true || filter( attrs[j].name ) ) {
value = computed &&
ve.dm.Converter.computedAttributes.indexOf( attrs[j].name ) !== -1 ?
originalDomElements[i][attrs[j].name] :
attrs[j].value;
@@ -180,7 +180,7 @@
ve.dm.Converter.renderHtmlAttributeList(
originalDomElements[i].children,
targetDomElements[i].children,
- spec,
+ filter,
computed,
true
);
diff --git a/src/dm/ve.dm.Model.js b/src/dm/ve.dm.Model.js
index bfb21b9..809d50e 100644
--- a/src/dm/ve.dm.Model.js
+++ b/src/dm/ve.dm.Model.js
@@ -178,63 +178,15 @@
*
* - true, to preserve all attributes (default)
* - false, to preserve none
- * - a string, to preserve only that attribute
- * - a regular expression matching attributes that should be preserved
- * - an array of strings or regular expressions
- * - an object with the following keys:
- * - blacklist: specification of attributes not to preserve
(boolean|string|RegExp|Array)
- * - whitelist: specification of attributes to preserve
- *
- * If only a blacklist is specified, all attributes will be preserved except
the ones matching
- * the blacklist. If only a whitelist is specified, only those attributes
matching the whitelist
- * will be preserved. If both are specified, only attributes that both match
the whitelist and
- * do not match the blacklist will be preserved.
+ * - a function that takes an attribute name and returns true or false
*
* @static
- * @property {boolean|string|RegExp|Array|Object}
+ * @property {boolean|Function}
* @inheritable
*/
ve.dm.Model.static.preserveHtmlAttributes = true;
/* Static methods */
-
-/**
- * Determine whether an attribute name matches an attribute specification.
- *
- * @param {string} attribute Attribute name
- * @param {boolean|string|RegExp|Array|Object} spec Attribute specification,
see #preserveHtmlAttributes
- * @returns {boolean} Attribute matches spec
- */
-ve.dm.Model.matchesAttributeSpec = function ( attribute, spec ) {
- function matches( subspec ) {
- if ( subspec instanceof RegExp ) {
- return !!subspec.exec( attribute );
- }
- if ( typeof subspec === 'boolean' ) {
- return subspec;
- }
- return attribute === subspec;
- }
-
- function matchesArray( specArray ) {
- var i, len;
- if ( !Array.isArray( specArray ) ) {
- specArray = [ specArray ];
- }
- for ( i = 0, len = specArray.length; i < len; i++ ) {
- if ( matches( specArray[i] ) ) {
- return true;
- }
- }
- return false;
- }
-
- if ( spec.whitelist === undefined && spec.blacklist === undefined ) {
- // Not an object, treat spec as a whitelist
- return matchesArray( spec );
- }
- return matchesArray( spec.whitelist || true ) && !matchesArray(
spec.blacklist || false );
-};
/**
* Get hash object of a linear model data element.
diff --git a/tests/dm/ve.dm.Model.test.js b/tests/dm/ve.dm.Model.test.js
deleted file mode 100644
index e38a4e2..0000000
--- a/tests/dm/ve.dm.Model.test.js
+++ /dev/null
@@ -1,121 +0,0 @@
-/*!
- * VisualEditor DataModel Model tests.
- *
- * @copyright 2011-2015 VisualEditor Team and others; see
http://ve.mit-license.org
- */
-
-QUnit.module( 've.dm.Model' );
-
-QUnit.test( 'matchesAttributeSpec', function ( assert ) {
- var i, cases = [
- {
- spec: true,
- attr: 'foo',
- result: true,
- msg: 'true matches anything'
- },
- {
- spec: false,
- attr: 'foo',
- result: false,
- msg: 'false matches nothing'
- },
- {
- spec: 'foo',
- attr: 'foo',
- result: true,
- msg: 'string matches exact value'
- },
- {
- spec: 'foo',
- attr: 'bar',
- result: false,
- msg: 'string does not match anything else'
- },
- {
- spec: /^foo/,
- attr: 'foobar',
- result: true,
- msg: 'regex matches'
- },
- {
- spec: /^foo/,
- attr: 'barfoo',
- result: false,
- msg: 'regex does not match anything else'
- },
- {
- spec: [ 'foo', /^bar/ ],
- attr: 'foo',
- result: true,
- msg: 'array of string and regex matches string'
- },
- {
- spec: [ 'foo', /^bar/ ],
- attr: 'barbaz',
- result: true,
- msg: 'array of string and regex matches regex match'
- },
- {
- spec: {
- blacklist: 'foo'
- },
- attr: 'foo',
- result: false,
- msg: 'blacklisted attribute does not match'
- },
- {
- spec: {
- blacklist: 'foo'
- },
- attr: 'bar',
- result: true,
- msg: 'non-blacklisted attribute matches'
- },
- {
- spec: {
- whitelist: /^foo/,
- blacklist: [ 'foobar', 'foobaz' ]
- },
- attr: 'foobar',
- result: false,
- msg: 'blacklist overrides whitelist'
- },
- {
- spec: {
- whitelist: /^foo/,
- blacklist: [ 'foobar', 'foobaz' ]
- },
- attr: 'foobaz',
- result: false,
- msg: 'blacklist overrides whitelist (2)'
- },
- {
- spec: {
- whitelist: /^foo/,
- blacklist: [ 'foobar', 'foobaz' ]
- },
- attr: 'fooquux',
- result: true,
- msg: 'attribute on whitelist and not on blacklist
matches'
- },
- {
- spec: {
- whitelist: /^foo/,
- blacklist: [ 'foobar', 'foobaz' ]
- },
- attr: 'bar',
- result: false,
- msg: 'attribute on neither whitelist nor blacklist does
not match'
- }
- ];
-
- QUnit.expect( cases.length );
- for ( i = 0; i < cases.length; i++ ) {
- assert.strictEqual(
- ve.dm.Model.matchesAttributeSpec( cases[i].attr,
cases[i].spec ),
- cases[i].result,
- cases[i].msg
- );
- }
-} );
diff --git a/tests/index.html b/tests/index.html
index 5e2f0ea..81917c4 100644
--- a/tests/index.html
+++ b/tests/index.html
@@ -352,7 +352,6 @@
<script
src="../tests/dm/ve.dm.SurfaceFragment.test.js"></script>
<script src="../tests/dm/ve.dm.ModelRegistry.test.js"></script>
<script src="../tests/dm/ve.dm.MetaList.test.js"></script>
- <script src="../tests/dm/ve.dm.Model.test.js"></script>
<script
src="../tests/dm/lineardata/ve.dm.FlatLinearData.test.js"></script>
<script
src="../tests/dm/lineardata/ve.dm.ElementLinearData.test.js"></script>
<script
src="../tests/dm/lineardata/ve.dm.MetaLinearData.test.js"></script>
--
To view, visit https://gerrit.wikimedia.org/r/191287
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia0e0fe4fc2385dab67c816148cfc362a39413dac
Gerrit-PatchSet: 6
Gerrit-Project: VisualEditor/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Ori.livneh <[email protected]>
Gerrit-Reviewer: Catrope <[email protected]>
Gerrit-Reviewer: Krinkle <[email protected]>
Gerrit-Reviewer: Ori.livneh <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
