DevTools: Add features to collect classnames from Stylesheets and DOM

DevTools: Add features to collect classnames from Stylesheets and DOM

BUG=636089
Committed: https://crrev.com/9f5d339452d3d4f76337ecb08e86227e13f15a79
Cr-Commit-Position: refs/heads/master@{#417780}

[ahmetemirercin, 2016-08-31 20:49:03]

Description was changed from

==========
DevTools: Add features to collect classnames from Stylesheets and DOM


DevTools: Add features to collect classnames from Stylesheets and DOM


BUG=

DevTools: Add features to collect classnames from CSS and DOM
==========

to

==========
DevTools: Add features to collect classnames from Stylesheets and DOM




BUG=636089
==========

[ahmetemirercin, 2016-08-31 20:49:04]

ahmetemiremir@gmail.com changed reviewers:
+ lushnikov@chromium.org, paulirish@chromium.org, pfeldman@chromium.org

[ahmetemirercin, 2016-08-31 20:52:27]

I split my previous issue (2282653002) as lushnikov suggested and this is the
first part about backend. Can you please review it?

[lushnikov, 2016-09-01 20:50:36]

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/inspector-protocol/css/css-collect-class-names.html
(right):

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector-protocol/css/css-collect-class-names.html:30:
if (a < b)
isn't default comparator good enough?

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector-protocol/css/css-collect-class-names.html:46:
for(var i=0; i < classNames.length; i++) {
style: we don't put {} around one-line block

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector-protocol/css/css-collect-class-names.html:52:

nit: stray line

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-collect-class-names.html
(right):

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-collect-class-names.html:15:
function onClassNamesCollected(response)
nit: let's have a line between functions

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-collect-class-names.html:17:

nit: stray line

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-collect-class-names.html:19:
classNames.sort(function (a, b)
doesn't the default comparator work good enough?

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-collect-class-names.html:39:
<ul class='class3'>
let's make sure it collects from shadow DOM as well

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h (left):

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h:140: 
let's keep this - it separated media query builders from other methods

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h (right):

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h:108: 
nit: stray line

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:590: return;
let's return an error in this case

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:597: for
(unsigned i = 0; i < classNameList.size(); ++i) {
style: drop {} around one-line blocks

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp (right):

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:1327: for
(unsigned i = ruleList ? ruleList->length() : 0; i > 0; --i) {
Let's reuse collectFlatRules(). It will free you from iterating these nested
structures.

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:1539: void
InspectorStyleSheet::getClassNameFromRule(CSSRule* rule, HashSet<String>&
uniqueNames)
let's have CSSStyleRule here

[ahmetemirercin, 2016-09-01 21:47:58]

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp (right):

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:1327: for
(unsigned i = ruleList ? ruleList->length() : 0; i > 0; --i) {
On 2016/09/01 20:50:36, lushnikov wrote:
> Let's reuse collectFlatRules(). It will free you from iterating these nested
> structures.

I specially didn't used it since "collectFlatRules" will collect rules that
can't have class names also.So collecting in this way comes to me faster than
adding them to list and filtering out later.

[lushnikov, 2016-09-01 22:04:25]

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp (right):

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:1327: for
(unsigned i = ruleList ? ruleList->length() : 0; i > 0; --i) {
I see.

Performance-wise, it should not be any measurable difference
Code-wise, gonna be much clearer!

[ahmetemirercin, 2016-09-02 19:59:55]

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/inspector-protocol/css/css-collect-class-names.html
(right):

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector-protocol/css/css-collect-class-names.html:30:
if (a < b)
On 2016/09/01 20:50:35, lushnikov wrote:
> isn't default comparator good enough?

Done.

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector-protocol/css/css-collect-class-names.html:46:
for(var i=0; i < classNames.length; i++) {
On 2016/09/01 20:50:35, lushnikov wrote:
> style: we don't put {} around one-line block

Done.

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector-protocol/css/css-collect-class-names.html:52:

On 2016/09/01 20:50:35, lushnikov wrote:
> nit: stray line

Done.

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-collect-class-names.html
(right):

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-collect-class-names.html:15:
function onClassNamesCollected(response)
On 2016/09/01 20:50:35, lushnikov wrote:
> nit: let's have a line between functions

Done.

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-collect-class-names.html:17:

On 2016/09/01 20:50:35, lushnikov wrote:
> nit: stray line 

Done.

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-collect-class-names.html:19:
classNames.sort(function (a, b)
On 2016/09/01 20:50:35, lushnikov wrote:
> doesn't the default comparator work good enough?

Done.

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-collect-class-names.html:39:
<ul class='class3'>
On 2016/09/01 20:50:35, lushnikov wrote:
> let's make sure it collects from shadow DOM as well

Done.

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h (left):

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h:140: 
On 2016/09/01 20:50:35, lushnikov wrote:
> let's keep this - it separated media query builders from other methods

Done.

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h (right):

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h:108: 
On 2016/09/01 20:50:35, lushnikov wrote:
> nit: stray line

Done.

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:590: return;
On 2016/09/01 20:50:35, lushnikov wrote:
> let's return an error in this case

Done.

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:597: for
(unsigned i = 0; i < classNameList.size(); ++i) {
On 2016/09/01 20:50:36, lushnikov wrote:
> style: drop {} around one-line blocks

Done.

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp (right):

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:1327: for
(unsigned i = ruleList ? ruleList->length() : 0; i > 0; --i) {
On 2016/09/01 22:04:24, lushnikov wrote:
> I see.
> 
> Performance-wise, it should not be any measurable difference
> Code-wise, gonna be much clearer!

Done.

https://codereview.chromium.org/2296323002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:1539: void
InspectorStyleSheet::getClassNameFromRule(CSSRule* rule, HashSet<String>&
uniqueNames)
On 2016/09/01 20:50:36, lushnikov wrote:
> let's have CSSStyleRule here

Done.

[lushnikov, 2016-09-06 17:51:39]

https://codereview.chromium.org/2296323002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-collect-class-names.html
(right):

https://codereview.chromium.org/2296323002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-collect-class-names.html:13:
InspectorTest.sendCommandOrDie("DOM.collectClassNames", { nodeId: rootNodeId },
onClassNamesCollected);
could you please also add a test that verifies that classNames are propertly
collected for subtrees as well?

https://codereview.chromium.org/2296323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp (right):

https://codereview.chromium.org/2296323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:1535: const
CSSSelector* subSelector = selectorList.first();
no need to extract from for-loop

https://codereview.chromium.org/2296323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:1539: if
(simpleSelector->match() == CSSSelector::Class) {
nit: drop {}

https://codereview.chromium.org/2296323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2296323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2955: "name":
"collectClassNamesFromStyleSheet",
let's name this collectClassNames as well

[ahmetemirercin, 2016-09-07 00:02:28]

https://codereview.chromium.org/2296323002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-collect-class-names.html
(right):

https://codereview.chromium.org/2296323002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector-protocol/dom/dom-collect-class-names.html:13:
InspectorTest.sendCommandOrDie("DOM.collectClassNames", { nodeId: rootNodeId },
onClassNamesCollected);
On 2016/09/06 17:51:38, lushnikov wrote:
> could you please also add a test that verifies that classNames are propertly
> collected for subtrees as well?

Done.

https://codereview.chromium.org/2296323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp (right):

https://codereview.chromium.org/2296323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:1535: const
CSSSelector* subSelector = selectorList.first();
On 2016/09/06 17:51:38, lushnikov wrote:
> no need to extract from for-loop

Done.

https://codereview.chromium.org/2296323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:1539: if
(simpleSelector->match() == CSSSelector::Class) {
On 2016/09/06 17:51:38, lushnikov wrote:
> nit: drop {} 

Done.

https://codereview.chromium.org/2296323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2296323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2955: "name":
"collectClassNamesFromStyleSheet",
On 2016/09/06 17:51:39, lushnikov wrote:
> let's name this collectClassNames as well

Done.

[lushnikov, 2016-09-07 22:31:37]

Great, almost there!

https://codereview.chromium.org/2296323002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/2296323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:594: for (; node;
node = FlatTreeTraversal::next(*node)) {
we should use the variation of method with "stayWithin" argument as the second
parameter;
this traversal occasionally traverses a bigger part of the tree (can we cover it
with test?)

https://codereview.chromium.org/2296323002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2296323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2173:
"description": "Collects class names for the node with given id and all of it's
child nodes."
let's mark this method as experimental as well

https://codereview.chromium.org/2296323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2962:
"description": "Returns all css class names for specified stylesheet."
let's mark this method as experimental

[ahmetemirercin, 2016-09-08 17:51:14]

https://codereview.chromium.org/2296323002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp (right):

https://codereview.chromium.org/2296323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp:594: for (; node;
node = FlatTreeTraversal::next(*node)) {
On 2016/09/07 22:31:37, lushnikov wrote:
> we should use the variation of method with "stayWithin" argument as the second
> parameter;
> this traversal occasionally traverses a bigger part of the tree (can we cover
it
> with test?)

Done.

https://codereview.chromium.org/2296323002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2296323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2173:
"description": "Collects class names for the node with given id and all of it's
child nodes."
On 2016/09/07 22:31:37, lushnikov wrote:
> let's mark this method as experimental as well

Done.

https://codereview.chromium.org/2296323002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2962:
"description": "Returns all css class names for specified stylesheet."
On 2016/09/07 22:31:37, lushnikov wrote:
> let's mark this method as experimental

Done.

[lushnikov, 2016-09-08 23:39:24]

thank you, lgtm

[lushnikov, 2016-09-08 23:41:21]

Could you please also add an issue description to briefly describes what this CL
does

[dgozman, 2016-09-09 01:57:55]

dgozman@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgozman, 2016-09-09 01:57:57]

Nice one!

https://codereview.chromium.org/2296323002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector-protocol/css/resources/collect-class-names.css
(right):

https://codereview.chromium.org/2296323002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector-protocol/css/resources/collect-class-names.css:15:
#testdiv > .test2 {
Let's have duplicates in this file to check they are not duplicated in protocol
response.

https://codereview.chromium.org/2296323002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp (right):

https://codereview.chromium.org/2296323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:1328:
collectFlatRules(m_pageStyleSheet.get(), &flatRules);
Why don't we use m_parsedFlatRules?

https://codereview.chromium.org/2296323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:1530: void
InspectorStyleSheet::getClassNameFromRule(CSSStyleRule* rule, HashSet<String>&
uniqueNames)
This doesn't have to be an instance function, let's move it to namespace {}
above (similar to findMagicComment).

https://codereview.chromium.org/2296323002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2296323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2166: "name":
"collectClassNames",
collectClassNamesFromSubtree

https://codereview.chromium.org/2296323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2171: {"name":
"classNames", "type": "array", "items": { "type": "string"}, "optional": true,
"description": "Class name list." }
- Don't make it optional.
- style: space between " and }

https://codereview.chromium.org/2296323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2961: {"name":
"classNames", "type": "array", "items": { "type": "string"}, "optional": true,
"description": "Class name list." }
- Don't make it optional.
- style: space between " and }

[ahmetemirercin, 2016-09-09 10:07:55]

https://codereview.chromium.org/2296323002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/inspector-protocol/css/resources/collect-class-names.css
(right):

https://codereview.chromium.org/2296323002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/inspector-protocol/css/resources/collect-class-names.css:15:
#testdiv > .test2 {
On 2016/09/09 01:57:56, dgozman wrote:
> Let's have duplicates in this file to check they are not duplicated in
protocol
> response.

Done.

https://codereview.chromium.org/2296323002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp (right):

https://codereview.chromium.org/2296323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:1328:
collectFlatRules(m_pageStyleSheet.get(), &flatRules);
On 2016/09/09 01:57:56, dgozman wrote:
> Why don't we use m_parsedFlatRules?

Done.

https://codereview.chromium.org/2296323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:1530: void
InspectorStyleSheet::getClassNameFromRule(CSSStyleRule* rule, HashSet<String>&
uniqueNames)
On 2016/09/09 01:57:56, dgozman wrote:
> This doesn't have to be an instance function, let's move it to namespace {}
> above (similar to findMagicComment).

Done.

https://codereview.chromium.org/2296323002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2296323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2166: "name":
"collectClassNames",
On 2016/09/09 01:57:56, dgozman wrote:
> collectClassNamesFromSubtree

Done.

https://codereview.chromium.org/2296323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2171: {"name":
"classNames", "type": "array", "items": { "type": "string"}, "optional": true,
"description": "Class name list." }
On 2016/09/09 01:57:56, dgozman wrote:
> - Don't make it optional.
> - style: space between " and }

Done.

https://codereview.chromium.org/2296323002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2961: {"name":
"classNames", "type": "array", "items": { "type": "string"}, "optional": true,
"description": "Class name list." }
On 2016/09/09 01:57:56, dgozman wrote:
> - Don't make it optional.
> - style: space between " and }

Done.

[pfeldman, 2016-09-09 15:55:23]

https://codereview.chromium.org/2296323002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2296323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2166: "name":
"collectClassNamesFromSubtree",
What about node ids? Will that be a separate method or should we collect them as
we traverse dom? We could name it "selectorsForAutocomplete" here and collect
#ids as well.

[ahmetemirercin, 2016-09-09 17:13:30]

On 2016/09/09 15:55:23, pfeldman wrote:
>
https://codereview.chromium.org/2296323002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):
> 
>
https://codereview.chromium.org/2296323002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/inspector/browser_protocol.json:2166: "name":
> "collectClassNamesFromSubtree",
> What about node ids? Will that be a separate method or should we collect them
as
> we traverse dom? We could name it "selectorsForAutocomplete" here and collect
> #ids as well.

I know I'm not in a position to make a decision but in my opinion, if we are to
suggest "ids", we should collect them from stylesheets not from DOM since ids
are expected to be unique and collecting from DOM and reassigning back to some
others conflicts with uniqueness. But there may be situations where stylesheets
can have rules with #id selectors and no counterpart in DOM.So expanding just
the method that collects from stylesheets seems to be more feasible.

[lushnikov, 2016-09-09 18:21:35]

https://codereview.chromium.org/2296323002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp (right):

https://codereview.chromium.org/2296323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:1343: for
(size_t i = 0; i < m_parsedFlatRules.size(); ++i) {
oh, m_parsedFlatRules, nice! I had a memory of them to be lazily populated under
some conditions, but turns out it's not the case any more!

[ahmetemirercin, 2016-09-09 18:48:50]

On 2016/09/09 18:21:35, lushnikov wrote:
>
https://codereview.chromium.org/2296323002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp (right):
> 
>
https://codereview.chromium.org/2296323002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:1343: for
> (size_t i = 0; i < m_parsedFlatRules.size(); ++i) {
> oh, m_parsedFlatRules, nice! I had a memory of them to be lazily populated
under
> some conditions, but turns out it's not the case any more!

Before dgozman's suggestion I wasn't sure about it's being an updated list and
prefered to use collectFlatRules. But Layout tests showed that we can trust it.

[ahmetemirercin, 2016-09-09 20:33:59]

Is the final patch ready for committing or needs improvement?

[dgozman, 2016-09-09 22:13:56]

lgtm

[paulirish, 2016-09-09 23:07:51]

The CQ bit was checked by paulirish@chromium.org

[paulirish, 2016-09-09 23:07:51]

lgtm

[paulirish, 2016-09-09 23:07:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
lushnikov@chromium.org
Link to the patchset: https://codereview.chromium.org/2296323002/#ps80001
(title: "DevTools: Add features to collect classnames from Stylesheets and DOM")

[commit-bot: I haz the power, 2016-09-09 23:08:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[paulirish, 2016-09-09 23:09:05]

I've just added the CL to the commit queue. If all goes well, it should be
landed within an hour or so.

Thanks Ahmet for the excellent patch! Really appreciate this feature and we're
excited for the next CL from you. :)

Cheers

[commit-bot: I haz the power, 2016-09-10 00:34:33]

Description was changed from

==========
DevTools: Add features to collect classnames from Stylesheets and DOM




BUG=636089
==========

to

==========
DevTools: Add features to collect classnames from Stylesheets and DOM




BUG=636089
==========

[commit-bot: I haz the power, 2016-09-10 00:34:37]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-10 00:35:46]

Description was changed from

==========
DevTools: Add features to collect classnames from Stylesheets and DOM




BUG=636089
==========

to

==========
DevTools: Add features to collect classnames from Stylesheets and DOM

BUG=636089

Committed: https://crrev.com/9f5d339452d3d4f76337ecb08e86227e13f15a79
Cr-Commit-Position: refs/heads/master@{#417780}
==========

[commit-bot: I haz the power, 2016-09-10 00:35:47]

Patchset 5 (id:??) landed as
https://crrev.com/9f5d339452d3d4f76337ecb08e86227e13f15a79
Cr-Commit-Position: refs/heads/master@{#417780}