From 0905947b3b0355164fbade512ca1c60732c05e9a Mon Sep 17 00:00:00 2001 From: Nicolas Chevobbe Date: Thu, 11 Jul 2019 08:53:53 +0000 Subject: [PATCH] Bug 1564480 - Enhance performance on parseAttribute function. r=gl. `hasAttribute` and `getAttribute` were using for..of loop to get what they wanted, which wasn't ideal. Moreover, most of those operations were happening because we didn't pass the attribute value to the parseAttribute function, only an array of all the attributes. This is a bit unfortunate there's only one place where we call the parseAttribute function, and there, we have easy access to the attribute value. This patch removes direct calls to hasAttribute and getAttribute. hasAttribute is removed, but we need to keep getAttribute for the isValid function in parsers. We add an `attributeValue` parameter to the parseAttribute function. The unit test for this function is also modified to pass the attribute value. perfherder reports a ~2.5% improvement on custom open and ~3% on custom reload. Differential Revision: https://phabricator.services.mozilla.com/D37551 --HG-- extra : moz-landing-system : lando --- .../inspector/markup/views/element-editor.js | 10 +- .../client/shared/node-attribute-parser.js | 110 ++++++++++-------- .../test/unit/test_attribute-parsing-02.js | 3 +- 3 files changed, 67 insertions(+), 56 deletions(-) diff --git a/devtools/client/inspector/markup/views/element-editor.js b/devtools/client/inspector/markup/views/element-editor.js index 6312079a7985..252df0b24c54 100644 --- a/devtools/client/inspector/markup/views/element-editor.js +++ b/devtools/client/inspector/markup/views/element-editor.js @@ -630,15 +630,17 @@ ElementEditor.prototype = { // it (make sure to pass a complete list of existing attributes to the // parseAttribute function, by concatenating attribute, because this could // be a newly added attribute not yet on this.node). - const attributes = this.node.attributes.filter(existingAttribute => { - return existingAttribute.name !== attribute.name; - }); + const attributes = this.node.attributes.filter( + existingAttribute => existingAttribute.name !== attribute.name + ); + attributes.push(attribute); const parsedLinksData = parseAttribute( this.node.namespaceURI, this.node.tagName, attributes, - attribute.name + attribute.name, + attribute.value ); // Create links in the attribute value, and collapse long attributes if diff --git a/devtools/client/shared/node-attribute-parser.js b/devtools/client/shared/node-attribute-parser.js index eee69c42f7df..956498081efd 100644 --- a/devtools/client/shared/node-attribute-parser.js +++ b/devtools/client/shared/node-attribute-parser.js @@ -37,6 +37,8 @@ const TYPE_CSS_RESOURCE_URI = "cssresource"; const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"; const HTML_NS = "http://www.w3.org/1999/xhtml"; +const WILDCARD = Symbol(); + const ATTRIBUTE_TYPES = new Map([ ["action", [{ namespaceURI: HTML_NS, tagName: "form", type: TYPE_URI }]], ["background", [{ namespaceURI: HTML_NS, tagName: "body", type: TYPE_URI }]], @@ -64,7 +66,10 @@ const ATTRIBUTE_TYPES = new Map([ { namespaceURI: XUL_NS, tagName: "key", type: TYPE_IDREF }, ], ], - ["contextmenu", [{ namespaceURI: "*", tagName: "*", type: TYPE_IDREF }]], + [ + "contextmenu", + [{ namespaceURI: WILDCARD, tagName: WILDCARD, type: TYPE_IDREF }], + ], ["data", [{ namespaceURI: HTML_NS, tagName: "object", type: TYPE_URI }]], [ "for", @@ -107,14 +112,14 @@ const ATTRIBUTE_TYPES = new Map([ { namespaceURI: HTML_NS, tagName: "a", type: TYPE_URI }, { namespaceURI: HTML_NS, tagName: "area", type: TYPE_URI }, { - namespaceURI: "*", + namespaceURI: WILDCARD, tagName: "link", type: TYPE_CSS_RESOURCE_URI, isValid: (namespaceURI, tagName, attributes) => { return getAttribute(attributes, "rel") === "stylesheet"; }, }, - { namespaceURI: "*", tagName: "link", type: TYPE_URI }, + { namespaceURI: WILDCARD, tagName: "link", type: TYPE_URI }, { namespaceURI: HTML_NS, tagName: "base", type: TYPE_URI }, ], ], @@ -133,7 +138,7 @@ const ATTRIBUTE_TYPES = new Map([ "menu", [ { namespaceURI: HTML_NS, tagName: "button", type: TYPE_IDREF }, - { namespaceURI: XUL_NS, tagName: "*", type: TYPE_IDREF }, + { namespaceURI: XUL_NS, tagName: WILDCARD, type: TYPE_IDREF }, ], ], [ @@ -148,7 +153,7 @@ const ATTRIBUTE_TYPES = new Map([ [ "src", [ - { namespaceURI: "*", tagName: "script", type: TYPE_JS_RESOURCE_URI }, + { namespaceURI: WILDCARD, tagName: "script", type: TYPE_JS_RESOURCE_URI }, { namespaceURI: HTML_NS, tagName: "input", type: TYPE_URI }, { namespaceURI: HTML_NS, tagName: "frame", type: TYPE_URI }, { namespaceURI: HTML_NS, tagName: "iframe", type: TYPE_URI }, @@ -169,39 +174,51 @@ const ATTRIBUTE_TYPES = new Map([ { namespaceURI: HTML_NS, tagName: "object", type: TYPE_URI }, ], ], - ["xmlns", [{ namespaceURI: "*", tagName: "*", type: TYPE_URI }]], - ["containment", [{ namespaceURI: XUL_NS, tagName: "*", type: TYPE_URI }]], - ["context", [{ namespaceURI: XUL_NS, tagName: "*", type: TYPE_IDREF }]], + ["xmlns", [{ namespaceURI: WILDCARD, tagName: WILDCARD, type: TYPE_URI }]], + [ + "containment", + [{ namespaceURI: XUL_NS, tagName: WILDCARD, type: TYPE_URI }], + ], + ["context", [{ namespaceURI: XUL_NS, tagName: WILDCARD, type: TYPE_IDREF }]], [ "datasources", - [{ namespaceURI: XUL_NS, tagName: "*", type: TYPE_URI_LIST }], + [{ namespaceURI: XUL_NS, tagName: WILDCARD, type: TYPE_URI_LIST }], ], - ["insertafter", [{ namespaceURI: XUL_NS, tagName: "*", type: TYPE_IDREF }]], - ["insertbefore", [{ namespaceURI: XUL_NS, tagName: "*", type: TYPE_IDREF }]], - ["observes", [{ namespaceURI: XUL_NS, tagName: "*", type: TYPE_IDREF }]], - ["popup", [{ namespaceURI: XUL_NS, tagName: "*", type: TYPE_IDREF }]], - ["ref", [{ namespaceURI: XUL_NS, tagName: "*", type: TYPE_URI }]], - ["removeelement", [{ namespaceURI: XUL_NS, tagName: "*", type: TYPE_IDREF }]], - ["template", [{ namespaceURI: XUL_NS, tagName: "*", type: TYPE_IDREF }]], - ["tooltip", [{ namespaceURI: XUL_NS, tagName: "*", type: TYPE_IDREF }]], + [ + "insertafter", + [{ namespaceURI: XUL_NS, tagName: WILDCARD, type: TYPE_IDREF }], + ], + [ + "insertbefore", + [{ namespaceURI: XUL_NS, tagName: WILDCARD, type: TYPE_IDREF }], + ], + ["observes", [{ namespaceURI: XUL_NS, tagName: WILDCARD, type: TYPE_IDREF }]], + ["popup", [{ namespaceURI: XUL_NS, tagName: WILDCARD, type: TYPE_IDREF }]], + ["ref", [{ namespaceURI: XUL_NS, tagName: WILDCARD, type: TYPE_URI }]], + [ + "removeelement", + [{ namespaceURI: XUL_NS, tagName: WILDCARD, type: TYPE_IDREF }], + ], + ["template", [{ namespaceURI: XUL_NS, tagName: WILDCARD, type: TYPE_IDREF }]], + ["tooltip", [{ namespaceURI: XUL_NS, tagName: WILDCARD, type: TYPE_IDREF }]], // SVG links aren't handled yet, see bug 1158831. // ["fill", [ - // {namespaceURI: SVG_NS, tagName: "*", type: }, + // {namespaceURI: SVG_NS, tagName: WILDCARD, type: }, // ]], // ["stroke", [ - // {namespaceURI: SVG_NS, tagName: "*", type: }, + // {namespaceURI: SVG_NS, tagName: WILDCARD, type: }, // ]], // ["markerstart", [ - // {namespaceURI: SVG_NS, tagName: "*", type: }, + // {namespaceURI: SVG_NS, tagName: WILDCARD, type: }, // ]], // ["markermid", [ - // {namespaceURI: SVG_NS, tagName: "*", type: }, + // {namespaceURI: SVG_NS, tagName: WILDCARD, type: }, // ]], // ["markerend", [ - // {namespaceURI: SVG_NS, tagName: "*", type: }, + // {namespaceURI: SVG_NS, tagName: WILDCARD, type: }, // ]], // ["xlink:href", [ - // {namespaceURI: SVG_NS, tagName: "*", type: }, + // {namespaceURI: SVG_NS, tagName: WILDCARD, type: }, // ]], ]); @@ -266,6 +283,7 @@ var parsers = { * @param {Array} attributes The list of all attributes of the node. This should * be an array of {name, value} objects. * @param {String} attributeName The name of the attribute to parse. + * @param {String} attributeValue The value of the attribute to parse. * @return {Array} An array of tokens that represents the value. Each token is * an object {type: [string|uri|jsresource|cssresource|idref], value}. * For instance parsing the ping attribute in returns: @@ -275,24 +293,24 @@ var parsers = { * {type: "uri", value: "uri1"} * ] */ -function parseAttribute(namespaceURI, tagName, attributes, attributeName) { - if (!hasAttribute(attributes, attributeName)) { - throw new Error( - `Attribute ${attributeName} isn't part of the ` + "provided attributes" - ); - } - +function parseAttribute( + namespaceURI, + tagName, + attributes, + attributeName, + attributeValue +) { const type = getType(namespaceURI, tagName, attributes, attributeName); if (!type) { return [ { type: TYPE_STRING, - value: getAttribute(attributes, attributeName), + value: attributeValue, }, ]; } - return parsers[type](getAttribute(attributes, attributeName)); + return parsers[type](attributeValue); } /** @@ -306,15 +324,18 @@ function parseAttribute(namespaceURI, tagName, attributes, attributeName) { * type object otherwise. */ function getType(namespaceURI, tagName, attributes, attributeName) { - if (!ATTRIBUTE_TYPES.get(attributeName)) { + const attributeType = ATTRIBUTE_TYPES.get(attributeName); + if (!attributeType) { return null; } - for (const typeData of ATTRIBUTE_TYPES.get(attributeName)) { + const lcTagName = tagName.toLowerCase(); + for (const typeData of attributeType) { const hasNamespace = - namespaceURI === typeData.namespaceURI || typeData.namespaceURI === "*"; + typeData.namespaceURI === WILDCARD || + typeData.namespaceURI === namespaceURI; const hasTagName = - tagName.toLowerCase() === typeData.tagName || typeData.tagName === "*"; + typeData.tagName === WILDCARD || typeData.tagName === lcTagName; const isValid = typeData.isValid ? typeData.isValid(namespaceURI, tagName, attributes, attributeName) : true; @@ -328,21 +349,8 @@ function getType(namespaceURI, tagName, attributes, attributeName) { } function getAttribute(attributes, attributeName) { - for (const { name, value } of attributes) { - if (name === attributeName) { - return value; - } - } - return null; -} - -function hasAttribute(attributes, attributeName) { - for (const { name } of attributes) { - if (name === attributeName) { - return true; - } - } - return false; + const attribute = attributes.find(x => x.name === attributeName); + return attribute ? attribute.value : null; } /** diff --git a/devtools/client/shared/test/unit/test_attribute-parsing-02.js b/devtools/client/shared/test/unit/test_attribute-parsing-02.js index b9d9606afc8d..96140a2c6550 100644 --- a/devtools/client/shared/test/unit/test_attribute-parsing-02.js +++ b/devtools/client/shared/test/unit/test_attribute-parsing-02.js @@ -129,7 +129,8 @@ function run_test() { namespaceURI, tagName, attributes, - attributeName + attributeName, + attributeValue ); if (!expected) { Assert.ok(!tokens);