Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
75253002:
DevTools: [Elements] Implement "Copy CSS Path" context menu item for elements (Closed)
|
Created:
7 years, 1 month ago by apavlov Modified:
7 years, 1 month ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, alph+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, aandrey+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionDevTools: [Elements] Implement "Copy CSS Path" context menu item for elements
R=pfeldman, vsevik, aandrey, eustas
BUG=277286
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162552
Patch Set 1 #
Total comments: 17
Patch Set 2 : Comments addressed #
Total comments: 6
Patch Set 3 : Fixed leading/trailing spaces in the "class" attribute #Patch Set 4 : Improve JsDoc #
Total comments: 2
Patch Set 5 : Use Boolean filter #Patch Set 6 : Protect from ridiculous IDs #
Total comments: 5
Patch Set 7 : Implement identifier escaping for id and class attribute values #
Total comments: 18
Patch Set 8 : Move CSS path-building code from DOMAgent to DOMPresentationUtils, reuse in DOMNode.appropriateSele… #
Total comments: 10
Patch Set 9 : More comments addressed, __proto__ bug fixed #
Total comments: 2
Patch Set 10 : Resolve the module dependency issue #Patch Set 11 : Move XPath construction into DOMPresentationUtils #Patch Set 12 : Tentatively fix xpath test on Windows bot #Messages
Total messages: 33 (0 generated)
https://codereview.chromium.org/75253002/diff/1/LayoutTests/inspector/element... File LayoutTests/inspector/elements/elements-css-path.html (right): https://codereview.chromium.org/75253002/diff/1/LayoutTests/inspector/element... LayoutTests/inspector/elements/elements-css-path.html:10: var doc; make this a local var of the function below https://codereview.chromium.org/75253002/diff/1/LayoutTests/inspector/element... LayoutTests/inspector/elements/elements-css-path.html:42: var result = prefix + node.cssPath(true); can you also test that: (document.querySelectorAll(node.cssPath(true)).contains(node) === true); ? https://codereview.chromium.org/75253002/diff/1/Source/devtools/front_end/DOM... File Source/devtools/front_end/DOMAgent.js (right): https://codereview.chromium.org/75253002/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMAgent.js:723: var nodeName = this.nodeNameInCorrectCase(); move above and get rid of ownValue https://codereview.chromium.org/75253002/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMAgent.js:724: var uniqueClassNamesArray = (this.getAttribute("class") || "").split(/\s+/g); this array is not unique, and can contain empty strings https://codereview.chromium.org/75253002/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMAgent.js:727: var needsClassNames; plz init (with "false" I believe?) https://codereview.chromium.org/75253002/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMAgent.js:728: var needsNthChild; ditto https://codereview.chromium.org/75253002/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMAgent.js:745: var siblingClasses = (sibling.getAttribute("class") || "").split(/\s+/g).keySet(); code dup, plz extract https://codereview.chromium.org/75253002/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMAgent.js:750: if (!--uniqueClassNamesLeft) { Did you mean uniqueClassNamesLeft to be initialized with the size of the keySet, instead of an "split" array?
https://codereview.chromium.org/75253002/diff/1/LayoutTests/inspector/element... File LayoutTests/inspector/elements/elements-css-path.html (right): https://codereview.chromium.org/75253002/diff/1/LayoutTests/inspector/element... LayoutTests/inspector/elements/elements-css-path.html:10: var doc; On 2013/11/18 13:43:51, aandrey wrote: > make this a local var of the function below Done. https://codereview.chromium.org/75253002/diff/1/LayoutTests/inspector/element... LayoutTests/inspector/elements/elements-css-path.html:42: var result = prefix + node.cssPath(true); On 2013/11/18 13:43:51, aandrey wrote: > can you also test that: > > (document.querySelectorAll(node.cssPath(true)).contains(node) === true); > > ? This condition is too weak. We should test that the document contains exactly one element matching each selector. Needed a complete test rewrite. Done. https://codereview.chromium.org/75253002/diff/1/Source/devtools/front_end/DOM... File Source/devtools/front_end/DOMAgent.js (right): https://codereview.chromium.org/75253002/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMAgent.js:723: var nodeName = this.nodeNameInCorrectCase(); On 2013/11/18 13:43:51, aandrey wrote: > move above and get rid of ownValue Done. https://codereview.chromium.org/75253002/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMAgent.js:724: var uniqueClassNamesArray = (this.getAttribute("class") || "").split(/\s+/g); On 2013/11/18 13:43:51, aandrey wrote: > this array is not unique, and can contain empty strings It is not unique, but it cannot contain empty strings (\s+). Implemented manual length computation over the resulting map. https://codereview.chromium.org/75253002/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMAgent.js:727: var needsClassNames; On 2013/11/18 13:43:51, aandrey wrote: > plz init (with "false" I believe?) Done. https://codereview.chromium.org/75253002/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMAgent.js:728: var needsNthChild; On 2013/11/18 13:43:51, aandrey wrote: > ditto Done. https://codereview.chromium.org/75253002/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMAgent.js:745: var siblingClasses = (sibling.getAttribute("class") || "").split(/\s+/g).keySet(); On 2013/11/18 13:43:51, aandrey wrote: > code dup, plz extract Done. https://codereview.chromium.org/75253002/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMAgent.js:750: if (!--uniqueClassNamesLeft) { On 2013/11/18 13:43:51, aandrey wrote: > Did you mean uniqueClassNamesLeft to be initialized with the size of the keySet, > instead of an "split" array? Yep. Fixed
https://codereview.chromium.org/75253002/diff/1/Source/devtools/front_end/DOM... File Source/devtools/front_end/DOMAgent.js (right): https://codereview.chromium.org/75253002/diff/1/Source/devtools/front_end/DOM... Source/devtools/front_end/DOMAgent.js:724: var uniqueClassNamesArray = (this.getAttribute("class") || "").split(/\s+/g); On 2013/11/18 15:03:59, apavlov wrote: > On 2013/11/18 13:43:51, aandrey wrote: > > this array is not unique, and can contain empty strings > > It is not unique, but it cannot contain empty strings (\s+). Implemented manual > length computation over the resulting map. FYI. a = document.createElement("div"); a.className = " a b a "; (a.getAttribute("class") || "").split(/\s+/g); ===> ["", "a", "b", "a", ""]
https://codereview.chromium.org/75253002/diff/70001/LayoutTests/inspector/ele... File LayoutTests/inspector/elements/elements-css-path.html (right): https://codereview.chromium.org/75253002/diff/70001/LayoutTests/inspector/ele... LayoutTests/inspector/elements/elements-css-path.html:16: InspectorTest.runTestSuite([ I thought every function passed to the suite should be a "test" by itself, rather than just init functions. https://codereview.chromium.org/75253002/diff/70001/Source/devtools/front_end... File Source/devtools/front_end/DOMAgent.js (right): https://codereview.chromium.org/75253002/diff/70001/Source/devtools/front_end... Source/devtools/front_end/DOMAgent.js:725: * @return {Object} @return {!Object.<string, boolean>} https://codereview.chromium.org/75253002/diff/70001/Source/devtools/front_end... Source/devtools/front_end/DOMAgent.js:729: return (node.getAttribute("class") || "").split(/\s+/g).keySet(); again, empty strings. a = document.createElement("div"); a.className = " a b a "; console.log((a.getAttribute("class") || "").split(/\s+/g)); console.log(Array.prototype.slice.call(a.classList)); ===> ["", "a", "b", "a", ""] ===> ["a", "b", "a"] https://codereview.chromium.org/75253002/diff/70001/Source/devtools/front_end... Source/devtools/front_end/DOMAgent.js:761: break; this could break while ownIndex is still -1 https://codereview.chromium.org/75253002/diff/70001/Source/devtools/front_end... Source/devtools/front_end/DOMAgent.js:772: result += ":nth-child(" + (ownIndex + 1) + ")"; ownIndex may be -1 here. add a test.
On 2013/11/18 15:15:56, aandrey wrote: > https://codereview.chromium.org/75253002/diff/1/Source/devtools/front_end/DOM... > File Source/devtools/front_end/DOMAgent.js (right): > > https://codereview.chromium.org/75253002/diff/1/Source/devtools/front_end/DOM... > Source/devtools/front_end/DOMAgent.js:724: var uniqueClassNamesArray = > (this.getAttribute("class") || "").split(/\s+/g); > On 2013/11/18 15:03:59, apavlov wrote: > > On 2013/11/18 13:43:51, aandrey wrote: > > > this array is not unique, and can contain empty strings > > > > It is not unique, but it cannot contain empty strings (\s+). Implemented > manual > > length computation over the resulting map. > > FYI. > > a = document.createElement("div"); > a.className = " a b a "; > (a.getAttribute("class") || "").split(/\s+/g); > > ===> ["", "a", "b", "a", ""] Ahh, that's what you mean. Got it, thanks.
https://codereview.chromium.org/75253002/diff/70001/Source/devtools/front_end... File Source/devtools/front_end/DOMAgent.js (right): https://codereview.chromium.org/75253002/diff/70001/Source/devtools/front_end... Source/devtools/front_end/DOMAgent.js:772: result += ":nth-child(" + (ownIndex + 1) + ")"; On 2013/11/18 15:36:26, aandrey wrote: > ownIndex may be -1 here. add a test. Ah, seems I was wrong.
https://codereview.chromium.org/75253002/diff/180001/Source/devtools/front_en... File Source/devtools/front_end/DOMAgent.js (right): https://codereview.chromium.org/75253002/diff/180001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:713: return new WebInspector.DOMNode.PathStep("#" + this.getAttribute("id"), true); "id" can contain spaces. can it be written properly as a query selector string? https://codereview.chromium.org/75253002/diff/180001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:733: return classAttribute.split(/\s+/g).filter(function(name) { .filter(Boolean).keySet();
On 2013/11/18 16:27:21, aandrey wrote: > https://codereview.chromium.org/75253002/diff/180001/Source/devtools/front_en... > File Source/devtools/front_end/DOMAgent.js (right): > > https://codereview.chromium.org/75253002/diff/180001/Source/devtools/front_en... > Source/devtools/front_end/DOMAgent.js:713: return new > WebInspector.DOMNode.PathStep("#" + this.getAttribute("id"), true); > "id" can contain spaces. can it be written properly as a query selector string? As per http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#the..., "id" may not contain spaces. So, let it be the author's responsibility to run the CSS path copying against a broken page :) > https://codereview.chromium.org/75253002/diff/180001/Source/devtools/front_en... > Source/devtools/front_end/DOMAgent.js:733: return > classAttribute.split(/\s+/g).filter(function(name) { > .filter(Boolean).keySet(); Ahh, nice! Done.
On 2013/11/19 06:07:45, apavlov wrote: > On 2013/11/18 16:27:21, aandrey wrote: > > > https://codereview.chromium.org/75253002/diff/180001/Source/devtools/front_en... > > File Source/devtools/front_end/DOMAgent.js (right): > > > > > https://codereview.chromium.org/75253002/diff/180001/Source/devtools/front_en... > > Source/devtools/front_end/DOMAgent.js:713: return new > > WebInspector.DOMNode.PathStep("#" + this.getAttribute("id"), true); > > "id" can contain spaces. can it be written properly as a query selector > string? > > As per > http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#the..., > "id" may not contain spaces. So, let it be the author's responsibility to run > the CSS path copying against a broken page :) We could ignore such IDs nevertheless. Besides, seems like dots are valid chars in IDs, aren't they? They will form an incorrect query selector string, like "#id.with.dots". In general, I think we should validate ID and class attributes against query selector syntax, and ignore those that would result in a corrupted query string. wdyt? is it worth doing?
On 2013/11/19 07:53:32, aandrey wrote: > On 2013/11/19 06:07:45, apavlov wrote: > > On 2013/11/18 16:27:21, aandrey wrote: > > > > > > https://codereview.chromium.org/75253002/diff/180001/Source/devtools/front_en... > > > File Source/devtools/front_end/DOMAgent.js (right): > > > > > > > > > https://codereview.chromium.org/75253002/diff/180001/Source/devtools/front_en... > > > Source/devtools/front_end/DOMAgent.js:713: return new > > > WebInspector.DOMNode.PathStep("#" + this.getAttribute("id"), true); > > > "id" can contain spaces. can it be written properly as a query selector > > string? > > > > As per > > > http://www.whatwg.org/specs/web-apps/current-work/multipage/elements.html#the..., > > "id" may not contain spaces. So, let it be the author's responsibility to run > > the CSS path copying against a broken page :) > > We could ignore such IDs nevertheless. > Besides, seems like dots are valid chars in IDs, aren't they? They will form an > incorrect query selector string, like "#id.with.dots". > > In general, I think we should validate ID and class attributes against query > selector syntax, and ignore those that would result in a corrupted query string. > > wdyt? is it worth doing? Implemented sanitizing/protection, PTAL
https://codereview.chromium.org/75253002/diff/260001/Source/devtools/front_en... File Source/devtools/front_end/DOMAgent.js (right): https://codereview.chromium.org/75253002/diff/260001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:761: var hasQuotes = /"/.test(id); indexOf https://codereview.chromium.org/75253002/diff/260001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:762: var hasApostrophes = /'/.test(id); indexOf https://codereview.chromium.org/75253002/diff/260001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:767: id = id.replace(/"/g, "\\\\\""); this code path is not tested, and quite hard to review https://codereview.chromium.org/75253002/diff/260001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:809: result += "." + className; And what about className containing odd characters, like "#"? I thought we were going the full way :)
https://codereview.chromium.org/75253002/diff/260001/Source/devtools/front_en... File Source/devtools/front_end/DOMAgent.js (right): https://codereview.chromium.org/75253002/diff/260001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:757: if (/^[A-Za-z0-9_-]+$/.test(id)) FYI. document.querySelectorAll("#---") ===> SyntaxError: Failed to execute query: '#---' is not a valid selector.
On 2013/11/19 11:15:08, aandrey wrote: > https://codereview.chromium.org/75253002/diff/260001/Source/devtools/front_en... > File Source/devtools/front_end/DOMAgent.js (right): > > https://codereview.chromium.org/75253002/diff/260001/Source/devtools/front_en... > Source/devtools/front_end/DOMAgent.js:757: if (/^[A-Za-z0-9_-]+$/.test(id)) > FYI. > > document.querySelectorAll("#---") > ===> SyntaxError: Failed to execute query: '#---' is not a valid selector. http://www.w3.org/TR/CSS21/syndata.html#value-def-identifier "In CSS, identifiers (including element names, classes, and IDs in selectors) can contain only the characters [a-zA-Z0-9] and ISO 10646 characters U+00A0 and higher, plus the hyphen (-) and the underscore (_); they cannot start with a digit, two hyphens, or a hyphen followed by a digit. Identifiers can also contain escaped characters and any ISO 10646 character as a numeric code (see next item). For instance, the identifier "B&W?" may be written as "B\&W\?" or "B\26 W\3F"."
On 2013/11/19 11:16:10, aandrey wrote: > On 2013/11/19 11:15:08, aandrey wrote: > > > https://codereview.chromium.org/75253002/diff/260001/Source/devtools/front_en... > > File Source/devtools/front_end/DOMAgent.js (right): > > > > > https://codereview.chromium.org/75253002/diff/260001/Source/devtools/front_en... > > Source/devtools/front_end/DOMAgent.js:757: if (/^[A-Za-z0-9_-]+$/.test(id)) > > FYI. > > > > document.querySelectorAll("#---") > > ===> SyntaxError: Failed to execute query: '#---' is not a valid selector. > > http://www.w3.org/TR/CSS21/syndata.html#value-def-identifier > > "In CSS, identifiers (including element names, classes, and IDs in selectors) > can contain only the characters [a-zA-Z0-9] and ISO 10646 characters U+00A0 and > higher, plus the hyphen (-) and the underscore (_); they cannot start with a > digit, two hyphens, or a hyphen followed by a digit. Identifiers can also > contain escaped characters and any ISO 10646 character as a numeric code (see > next item). For instance, the identifier "B&W?" may be written as "B\&W\?" or > "B\26 W\3F"." maybe this regex will do: /^(?:[a-zA-Z_]|-[a-zA-Z_])[a-zA-Z0-9_-]*$/
On 2013/11/19 11:30:23, aandrey wrote: > On 2013/11/19 11:16:10, aandrey wrote: > > On 2013/11/19 11:15:08, aandrey wrote: > > > > > > https://codereview.chromium.org/75253002/diff/260001/Source/devtools/front_en... > > > File Source/devtools/front_end/DOMAgent.js (right): > > > > > > > > > https://codereview.chromium.org/75253002/diff/260001/Source/devtools/front_en... > > > Source/devtools/front_end/DOMAgent.js:757: if (/^[A-Za-z0-9_-]+$/.test(id)) > > > FYI. > > > > > > document.querySelectorAll("#---") > > > ===> SyntaxError: Failed to execute query: '#---' is not a valid selector. > > > > http://www.w3.org/TR/CSS21/syndata.html#value-def-identifier > > > > "In CSS, identifiers (including element names, classes, and IDs in selectors) > > can contain only the characters [a-zA-Z0-9] and ISO 10646 characters U+00A0 > and > > higher, plus the hyphen (-) and the underscore (_); they cannot start with a > > digit, two hyphens, or a hyphen followed by a digit. Identifiers can also > > contain escaped characters and any ISO 10646 character as a numeric code (see > > next item). For instance, the identifier "B&W?" may be written as "B\&W\?" or > > "B\26 W\3F"." > > maybe this regex will do: > /^(?:[a-zA-Z_]|-[a-zA-Z_])[a-zA-Z0-9_-]*$/ Implemented identifier escaping for ids and classes, since you cannot do [class="foo, bar"] (the class list is variable).
overall looks good, but I think my tests won't pass https://codereview.chromium.org/75253002/diff/350001/LayoutTests/inspector/el... File LayoutTests/inspector/elements/elements-css-path.html (right): https://codereview.chromium.org/75253002/diff/350001/LayoutTests/inspector/el... LayoutTests/inspector/elements/elements-css-path.html:34: var escapedPath = cssPath.replace(/\\/g, "\\\\"); do we really have to do this? maybe we should return already escaped result from node.cssPath() ? https://codereview.chromium.org/75253002/diff/350001/LayoutTests/inspector/el... LayoutTests/inspector/elements/elements-css-path.html:103: can you plz also add the following test: <div id="not-unique-classes"> <span class="c1"></span> <span class="c1"></span> <span class="c1 c2"></span> <span class="c1 c2 c3"></span> <span></span> <div class="c1"></div> <div class="c1 c2"></div> <div class="c3 c2"></div> <div class="c3 c4"></div> <div class="c1 c4"></div> <div></div> </div> I'd expect: #not-unique-classes > span:nth-child(1) #not-unique-classes > span:nth-child(2) #not-unique-classes > span:nth-child(3) #not-unique-classes > span.c3 #not-unique-classes > span:nth-child(5) #not-unique-classes > div:nth-child(1) #not-unique-classes > div.c1.c2 #not-unique-classes > div.c2.c3 (sorted? i.e. not ".c3.c2") #not-unique-classes > div.c3.c4 #not-unique-classes > div.c1.c4 #not-unique-classes > div:nth-child(6) https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... File Source/devtools/front_end/DOMAgent.js (right): https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:769: var shouldEscapeFirst = /^(?:[0-9]|-[0-9-]?)/.test(ident); let's use similar regex to the one you already use: var shouldEscapeFirst = !/^-?[a-zA-Z_]/.test(ident); https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:773: result += ((!i && shouldEscapeFirst) || !isCSSIdentChar(c)) ? escapeAsciiChar(c, i === lastIndex) : c; nit: hard to read, but I could not simplify this neither. this is what I got: return ident.replace(/./g, function(c, i) { return ((shouldEscapeFirst && i === 0) || !isCSSIdentChar(c)) ? escapeAsciiChar(c, i === lastIndex) : c }); https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:805: if (/[a-zA-Z0-9_-]/.test(c)) /^[a-zA-Z0-9_-]$/
> Implemented identifier escaping for ids and classes, since you cannot do > [class="foo, bar"] (the class list is variable). [class="foo"][class="bar"], no?
On 2013/11/20 12:50:59, aandrey wrote: > > Implemented identifier escaping for ids and classes, since you cannot do > > [class="foo, bar"] (the class list is variable). > > > [class="foo"][class="bar"], no? I meant: [class~="foo"][class~="bar"]
https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... File Source/devtools/front_end/DOMAgent.js (right): https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:769: var shouldEscapeFirst = /^(?:[0-9]|-[0-9-]?)/.test(ident); On 2013/11/20 12:49:02, aandrey wrote: > let's use similar regex to the one you already use: > > var shouldEscapeFirst = !/^-?[a-zA-Z_]/.test(ident); This regex does not cover the case of an identifier starting with two dashes. https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:773: result += ((!i && shouldEscapeFirst) || !isCSSIdentChar(c)) ? escapeAsciiChar(c, i === lastIndex) : c; On 2013/11/20 12:49:02, aandrey wrote: > nit: hard to read, but I could not simplify this neither. this is what I got: > > return ident.replace(/./g, function(c, i) { > return ((shouldEscapeFirst && i === 0) || !isCSSIdentChar(c)) ? > escapeAsciiChar(c, i === lastIndex) : c > }); Fine with that, even though running this code might be an overkill (not too efficient performance-wise) https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:805: if (/[a-zA-Z0-9_-]/.test(c)) On 2013/11/20 12:49:02, aandrey wrote: > /^[a-zA-Z0-9_-]$/ Does it make much sense given that |c| is guaranteed to contain a single character? This may turn the regex slower to run.
On 2013/11/20 12:54:24, aandrey wrote: > On 2013/11/20 12:50:59, aandrey wrote: > > > Implemented identifier escaping for ids and classes, since you cannot do > > > [class="foo, bar"] (the class list is variable). > > > > > > [class="foo"][class="bar"], no? > > I meant: [class~="foo"][class~="bar"] Do you like it? :)
On 2013/11/20 12:49:02, aandrey wrote: > overall looks good, but I think my tests won't pass > > https://codereview.chromium.org/75253002/diff/350001/LayoutTests/inspector/el... > File LayoutTests/inspector/elements/elements-css-path.html (right): > > https://codereview.chromium.org/75253002/diff/350001/LayoutTests/inspector/el... > LayoutTests/inspector/elements/elements-css-path.html:34: var escapedPath = > cssPath.replace(/\\/g, "\\\\"); > do we really have to do this? > maybe we should return already escaped result from node.cssPath() ? > > https://codereview.chromium.org/75253002/diff/350001/LayoutTests/inspector/el... > LayoutTests/inspector/elements/elements-css-path.html:103: > can you plz also add the following test: > > <div id="not-unique-classes"> > <span class="c1"></span> > <span class="c1"></span> > <span class="c1 c2"></span> > <span class="c1 c2 c3"></span> > <span></span> > <div class="c1"></div> > <div class="c1 c2"></div> > <div class="c3 c2"></div> > <div class="c3 c4"></div> > <div class="c1 c4"></div> > <div></div> > </div> > > I'd expect: > > #not-unique-classes > span:nth-child(1) > #not-unique-classes > span:nth-child(2) > #not-unique-classes > span:nth-child(3) > #not-unique-classes > span.c3 Let us do ourselves a favor and skip the selector count optimization step :)
https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... File Source/devtools/front_end/DOMAgent.js (right): https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:696: cssPath: function(optimized) This interesting little feature is about to become a large chunk of DOM agent comparable to the whole code in size. It needs to be in a class that loads lazily along with the elements panel. DOMPresentationUtils sounds like a good match, but it loads with core today. Lets put it there for starters though. https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:814: function isCSSIdentifier(value) Also, DOM should not know this much about CSS https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... File Source/devtools/front_end/ElementsTreeOutline.js (right): https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... Source/devtools/front_end/ElementsTreeOutline.js:2202: this._node.copyCSSPath(true); Why aren't we using this when creating new rules? Seems more useful there
https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... File Source/devtools/front_end/DOMAgent.js (right): https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:696: cssPath: function(optimized) On 2013/11/20 14:08:48, pfeldman wrote: > This interesting little feature is about to become a large chunk of DOM agent > comparable to the whole code in size. It needs to be in a class that loads > lazily along with the elements panel. DOMPresentationUtils sounds like a good > match, but it loads with core today. Lets put it there for starters though. Sounds good. https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:814: function isCSSIdentifier(value) On 2013/11/20 14:08:48, pfeldman wrote: > Also, DOM should not know this much about CSS I'm fine with adding a lazy-loaded class. Or will DOMPresentationUtils work for this, too? https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... File Source/devtools/front_end/ElementsTreeOutline.js (right): https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... Source/devtools/front_end/ElementsTreeOutline.js:2202: this._node.copyCSSPath(true); On 2013/11/20 14:08:48, pfeldman wrote: > Why aren't we using this when creating new rules? Seems more useful there An excellent idea!
https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... File Source/devtools/front_end/DOMAgent.js (right): https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:769: var shouldEscapeFirst = /^(?:[0-9]|-[0-9-]?)/.test(ident); On 2013/11/20 13:15:28, apavlov wrote: > On 2013/11/20 12:49:02, aandrey wrote: > > let's use similar regex to the one you already use: > > > > var shouldEscapeFirst = !/^-?[a-zA-Z_]/.test(ident); > > This regex does not cover the case of an identifier starting with two dashes. in console: /^(?:[0-9]|-[0-9-]?)/.test("--a") !/^-?[a-zA-Z_]/.test("--a") ==> true ==> true https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:805: if (/[a-zA-Z0-9_-]/.test(c)) On 2013/11/20 13:15:28, apavlov wrote: > On 2013/11/20 12:49:02, aandrey wrote: > > /^[a-zA-Z0-9_-]$/ > > Does it make much sense given that |c| is guaranteed to contain a single > character? This may turn the regex slower to run. I think adding ^ and $ restrictions should always hasten the regex, no? Anyway its up to you. https://codereview.chromium.org/75253002/diff/480001/Source/devtools/front_en... File Source/devtools/front_end/DOMPresentationUtils.js (right): https://codereview.chromium.org/75253002/diff/480001/Source/devtools/front_en... Source/devtools/front_end/DOMPresentationUtils.js:1: /* should we also move XPath to this file (maybe in another patch)? https://codereview.chromium.org/75253002/diff/480001/Source/devtools/front_en... Source/devtools/front_end/DOMPresentationUtils.js:162: WebInspector.DOMPresentationUtils.cssPathValue = function(node, optimized) jsdocs plz https://codereview.chromium.org/75253002/diff/480001/Source/devtools/front_en... Source/devtools/front_end/DOMPresentationUtils.js:177: // The "id" presence should terminate the traversal, hence |true|. sound obvious to me... maybe remove? https://codereview.chromium.org/75253002/diff/480001/Source/devtools/front_en... Source/devtools/front_end/DOMPresentationUtils.js:262: var uniqueClassNamesArray = elementClassNames(node); they are no longer unique, once you removed keySet() https://codereview.chromium.org/75253002/diff/480001/Source/devtools/front_en... Source/devtools/front_end/DOMPresentationUtils.js:279: var ownClassNames = uniqueClassNamesArray.keySet(); maybe write a utility method to remove a given array of properties from an object (the keySet)? plus smth like function hasAnyOwnProperty(obj) { for (var p in obj) { if (obj.hasOwnProperty(p)) return true; } return false; }
https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... File Source/devtools/front_end/DOMAgent.js (right): https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:769: var shouldEscapeFirst = /^(?:[0-9]|-[0-9-]?)/.test(ident); On 2013/11/20 16:20:49, aandrey wrote: > On 2013/11/20 13:15:28, apavlov wrote: > > On 2013/11/20 12:49:02, aandrey wrote: > > > let's use similar regex to the one you already use: > > > > > > var shouldEscapeFirst = !/^-?[a-zA-Z_]/.test(ident); > > > > This regex does not cover the case of an identifier starting with two dashes. > > in console: > > /^(?:[0-9]|-[0-9-]?)/.test("--a") > !/^-?[a-zA-Z_]/.test("--a") > > ==> true > ==> true Right. But I know what breaks. Valid unicode chars do. They do not match the regex but should NOT be replaced. https://codereview.chromium.org/75253002/diff/350001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:805: if (/[a-zA-Z0-9_-]/.test(c)) On 2013/11/20 16:20:49, aandrey wrote: > On 2013/11/20 13:15:28, apavlov wrote: > > On 2013/11/20 12:49:02, aandrey wrote: > > > /^[a-zA-Z0-9_-]$/ > > > > Does it make much sense given that |c| is guaranteed to contain a single > > character? This may turn the regex slower to run. > > I think adding ^ and $ restrictions should always hasten the regex, no? That's the case for multichar strings. In our case, the EOL check and additional compilation will eat up precious CPU cycles :) https://codereview.chromium.org/75253002/diff/480001/Source/devtools/front_en... File Source/devtools/front_end/DOMPresentationUtils.js (right): https://codereview.chromium.org/75253002/diff/480001/Source/devtools/front_en... Source/devtools/front_end/DOMPresentationUtils.js:1: /* On 2013/11/20 16:20:49, aandrey wrote: > should we also move XPath to this file (maybe in another patch)? XPath building deals equally much with DOM and presentation, so I'm not sure what to do :) https://codereview.chromium.org/75253002/diff/480001/Source/devtools/front_en... Source/devtools/front_end/DOMPresentationUtils.js:162: WebInspector.DOMPresentationUtils.cssPathValue = function(node, optimized) On 2013/11/20 16:20:49, aandrey wrote: > jsdocs plz Done. https://codereview.chromium.org/75253002/diff/480001/Source/devtools/front_en... Source/devtools/front_end/DOMPresentationUtils.js:177: // The "id" presence should terminate the traversal, hence |true|. On 2013/11/20 16:20:49, aandrey wrote: > sound obvious to me... maybe remove? Done. https://codereview.chromium.org/75253002/diff/480001/Source/devtools/front_en... Source/devtools/front_end/DOMPresentationUtils.js:262: var uniqueClassNamesArray = elementClassNames(node); On 2013/11/20 16:20:49, aandrey wrote: > they are no longer unique, once you removed keySet() Done. https://codereview.chromium.org/75253002/diff/480001/Source/devtools/front_en... Source/devtools/front_end/DOMPresentationUtils.js:279: var ownClassNames = uniqueClassNamesArray.keySet(); On 2013/11/20 16:20:49, aandrey wrote: > maybe write a utility method to remove a given array of properties from an > object (the keySet)? The problem is that we still need to know how many elements have remained in the keySet and set a flag value based on that. And the greatest drawback of our Set is that it does not work with strings :( > > plus smth like > > function hasAnyOwnProperty(obj) { > for (var p in obj) { > if (obj.hasOwnProperty(p)) > return true; > } > return false; > } How would it be useful for us? We need to detect if one set contains another, but without using the Set class, this becomes tedious to implement as a separate method. On a related note, I've found a bug. The "__proto__" class would not get stored in an object-based map, so I now prefix all classes with a "$" and cut it off when writing the class list.
https://codereview.chromium.org/75253002/diff/520001/Source/devtools/front_en... File Source/devtools/front_end/DOMAgent.js (right): https://codereview.chromium.org/75253002/diff/520001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:675: var step = WebInspector.DOMPresentationUtils.cssPathValue(contextNode, optimized); there is a dependency problem: "sdk" module (containing DOMAgent.js) does not depend on "components" module (containing DOMPresentationUtils.js) https://codereview.chromium.org/75253002/diff/520001/Source/devtools/front_en... Source/devtools/front_end/DOMAgent.js:692: _cssPathValue: function(optimized) remove this method
On 2013/11/21 10:17:32, aandrey wrote: > https://codereview.chromium.org/75253002/diff/520001/Source/devtools/front_en... > File Source/devtools/front_end/DOMAgent.js (right): > > https://codereview.chromium.org/75253002/diff/520001/Source/devtools/front_en... > Source/devtools/front_end/DOMAgent.js:675: var step = > WebInspector.DOMPresentationUtils.cssPathValue(contextNode, optimized); > there is a dependency problem: "sdk" module (containing DOMAgent.js) does not > depend on "components" module (containing DOMPresentationUtils.js) > > https://codereview.chromium.org/75253002/diff/520001/Source/devtools/front_en... > Source/devtools/front_end/DOMAgent.js:692: _cssPathValue: function(optimized) > remove this method Fixed both
LGTM. Thank you for your patience!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apavlov@chromium.org/75253002/630001
Retried try job too often on win_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apavlov@chromium.org/75253002/810001
Message was sent while issue was closed.
Change committed as 162552 |
