Backend for css rule tracking

- backend code for css rule tracking
- 2 new browser protocol functions : "startRuleUsageTracking" and "stopRuleUsageTracking" that also returns the rule usage list.

BUG=662260
Committed: https://crrev.com/68f60f6cfa8d0b968ed9a90e2188dd84c62788e5
Cr-Commit-Position: refs/heads/master@{#429792}

[valih, 2016-10-27 20:42:46]

valih@google.com changed reviewers:
+ caseq@chromium.org

[valih, 2016-10-27 20:45:28]

valih@google.com changed reviewers:
+ pfeldman@chromium.org, rune@opera.com

[tomjamri, 2016-10-27 20:46:41]

tomjamri@gmail.com changed reviewers:
+ tomjamri@gmail.com

[tomjamri, 2016-10-27 20:46:41]

lgtm

[caseq, 2016-10-27 20:59:56]

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/ElementRuleCollector.h (right):

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ElementRuleCollector.h:146:
HeapVector<MatchedRule, 32>& matchedRulesList() { return m_matchedRules; }
const?

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/resolver/StyleResolver.h:208: void
addMatchedRulesToTracker(ElementRuleCollector&);
const ElementRuleCollector&

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/StyleEngine.h:40: #include
"core/css/resolver/StyleRuleUsageTracker.h"
forward-declare instead.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/StyleEngine.h:169: void
setRuleUsageTracker(Member<StyleRuleUsageTracker>);
StyleRuleUsageTracker*, drop Member<>

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

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:690: if
(m_state->booleanProperty(CSSAgentState::cssAgentEnabled, false))
Nope -- wrong field name.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:784:
InspectorStyleSheet* insp =
s/insp/style_sheet/?

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:788: if
(rule->type() != CSSRule::kStyleRule)
So can we handle other types of rules?

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:790:
CSSStyleRule* rule2 = static_cast<CSSStyleRule*>(rule.get());
time to get more creative with variable names -- this would be css_rule

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:792: StyleRule*
rule3 = rule2->styleRule();
and this would be style_rule

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:794: if
(m_tracker.get()->getRuleList()->contains(rule3)) {
Can you reduce this to a single statement?

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2529: m_tracker =
new StyleRuleUsageTracker;
new StyleRuleUsageTracker()

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2531: m_tracker =
0;
s/0/nullptr/

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2546: if (enable)
{
get rid of this if, you don't need it.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2573: if
(m_tracker)
if (!m_tracker) {
  *errorString = "CSS rule usage tracking is not enabled";
  return;
}

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

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h:31: #include
"core/css/resolver/StyleRuleUsageTracker.h"
forward declare instead?

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2764: { "name":
"status", "type": "boolean", "optional": true, "description": "This flag  is
true if the rule was used." }
"description": "Indicates whether the rule was actually used by some element in
the page."

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3151:
"description": "The list of the rules that were used."
"The list of rules with an indication of whether these were used"

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js (right):

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1565: 
Please revet this file altogether.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js (right):

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js:607: var self =
this;
You no longer need this one, I fixed the jsdoc validator
(https://codereview.chromium.org/2449173002/)

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sdk/CSSRule.js (right):

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/CSSRule.js:106: * @param
{boolean=} status
s/status/wasUsed/

[valih, 2016-10-27 21:41:17]

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/ElementRuleCollector.h (right):

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ElementRuleCollector.h:146:
HeapVector<MatchedRule, 32>& matchedRulesList() { return m_matchedRules; }
On 2016/10/27 20:59:54, caseq wrote:
> const?

Done.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/resolver/StyleResolver.h:208: void
addMatchedRulesToTracker(ElementRuleCollector&);
On 2016/10/27 20:59:55, caseq wrote:
> const ElementRuleCollector&

Done.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/StyleEngine.h (right):

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/StyleEngine.h:40: #include
"core/css/resolver/StyleRuleUsageTracker.h"
On 2016/10/27 20:59:55, caseq wrote:
> forward-declare instead.

Done.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/StyleEngine.h:169: void
setRuleUsageTracker(Member<StyleRuleUsageTracker>);
On 2016/10/27 20:59:55, caseq wrote:
> StyleRuleUsageTracker*, drop Member<>

Done.

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

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:690: if
(m_state->booleanProperty(CSSAgentState::cssAgentEnabled, false))
On 2016/10/27 20:59:55, caseq wrote:
> Nope -- wrong field name.

Done.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:784:
InspectorStyleSheet* insp =
On 2016/10/27 20:59:55, caseq wrote:
> s/insp/style_sheet/?

Done.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:790:
CSSStyleRule* rule2 = static_cast<CSSStyleRule*>(rule.get());
On 2016/10/27 20:59:55, caseq wrote:
> time to get more creative with variable names -- this would be css_rule

Done.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:792: StyleRule*
rule3 = rule2->styleRule();
On 2016/10/27 20:59:55, caseq wrote:
> and this would be style_rule

Done.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:794: if
(m_tracker.get()->getRuleList()->contains(rule3)) {
On 2016/10/27 20:59:55, caseq wrote:
> Can you reduce this to a single statement?

Done.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2529: m_tracker =
new StyleRuleUsageTracker;
On 2016/10/27 20:59:55, caseq wrote:
> new StyleRuleUsageTracker()

Done.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2531: m_tracker =
0;
On 2016/10/27 20:59:55, caseq wrote:
> s/0/nullptr/

Done.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2546: if (enable)
{
On 2016/10/27 20:59:55, caseq wrote:
> get rid of this if, you don't need it.

Done.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2573: if
(m_tracker)
On 2016/10/27 20:59:55, caseq wrote:
> if (!m_tracker) {
>   *errorString = "CSS rule usage tracking is not enabled";
>   return;
> }

Done.

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

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h:31: #include
"core/css/resolver/StyleRuleUsageTracker.h"
On 2016/10/27 20:59:55, caseq wrote:
> forward declare instead?

Done.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2764: { "name":
"status", "type": "boolean", "optional": true, "description": "This flag  is
true if the rule was used." }
On 2016/10/27 20:59:56, caseq wrote:
> "description": "Indicates whether the rule was actually used by some element
in
> the page."

Done.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3151:
"description": "The list of the rules that were used."
On 2016/10/27 20:59:55, caseq wrote:
> "The list of rules with an indication of whether these were used"

Done.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js (right):

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1565: 
On 2016/10/27 20:59:56, caseq wrote:
> Please revet this file altogether.

Done.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js (right):

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js:607: var self =
this;
On 2016/10/27 20:59:56, caseq wrote:
> You no longer need this one, I fixed the jsdoc validator
> (https://codereview.chromium.org/2449173002/)

Done.

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sdk/CSSRule.js (right):

https://codereview.chromium.org/2455943003/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sdk/CSSRule.js:106: * @param
{boolean=} status
On 2016/10/27 20:59:56, caseq wrote:
> s/status/wasUsed/

Done.

[rune, 2016-10-28 09:09:41]

core/css and core/dom lgtm with the mentioned return value type change.

https://codereview.chromium.org/2455943003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleRuleUsageTracker.h
(right):

https://codereview.chromium.org/2455943003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleRuleUsageTracker.h:18: const
HeapHashSet<Member<StyleRule>>* getRuleList() const;
Since this always return something non-null, you could return a const reference
instead.

[rune, 2016-10-28 09:11:09]

On 2016/10/28 09:09:41, rune wrote:
> core/css and core/dom lgtm with the mentioned return value type change.

The CL description should be a longer story, and with a BUG=, btw.

[valih, 2016-10-28 18:32:09]

valih@google.com changed reviewers:
- tomjamri@gmail.com

[valih, 2016-10-28 18:32:40]

https://codereview.chromium.org/2455943003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleRuleUsageTracker.h
(right):

https://codereview.chromium.org/2455943003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleRuleUsageTracker.h:18: const
HeapHashSet<Member<StyleRule>>* getRuleList() const;
On 2016/10/28 09:09:41, rune wrote:
> Since this always return something non-null, you could return a const
reference
> instead.

Done.

[valih, 2016-10-28 18:36:57]

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

https://codereview.chromium.org/2455943003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:788: if
(rule->type() != CSSRule::kStyleRule)
deleted

[valih, 2016-10-31 18:18:00]

added ruleType to RuleUsage object

[valih, 2016-10-31 18:59:05]

https://codereview.chromium.org/2455943003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js (right):

https://codereview.chromium.org/2455943003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1: /*
this file is a mistake... ignore the changes here

[caseq, 2016-10-31 19:06:06]

Looks mostly good to me. Pavel, mind having a look?

https://codereview.chromium.org/2455943003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector-protocol/css/css-get-rule-list.html
(right):

https://codereview.chromium.org/2455943003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector-protocol/css/css-get-rule-list.html:24:
var rules = result.CSSRules, usedLines = [], unusedLines = [];
nit:

var usedLines = rules.filter(rule => rule.used);
var unusedLines = rules.filter(rule => !rule.used);

https://codereview.chromium.org/2455943003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/2455943003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleResolver.h:37: #include
"core/css/resolver/StyleRuleUsageTracker.h"
forward declare?

https://codereview.chromium.org/2455943003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleRuleUsageTracker.h
(right):

https://codereview.chromium.org/2455943003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleRuleUsageTracker.h:18: const
HeapHashSet<Member<StyleRule>>* getRuleList() const;
const HeapHashSet<Member<StyleRule>>&

https://codereview.chromium.org/2455943003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/2455943003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:773: void
InspectorCSSAgent::getUnusedRules(
nit: consider merging into getRuleList()

https://codereview.chromium.org/2455943003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:793: rule.get(),
m_tracker.get()->getRuleList()->contains(styleRule)));
just m_tracker->getRuleList(), remove .get()

https://codereview.chromium.org/2455943003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2455943003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2774: { "name":
"status", "type": "boolean", "description": "Indicates whether the rule was
actually used by some element in the page." }
"used" instead of "status"?

https://codereview.chromium.org/2455943003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js (right):

https://codereview.chromium.org/2455943003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1: /*
On 2016/10/31 18:59:05, valih wrote:
> this file is a mistake... ignore the changes here

let's revert it from the next one then :-)

[pfeldman, 2016-10-31 21:03:36]

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:854:
addMatchedRulesToTracker(collector);
I'd pull the check into the call site:

if (m_tracker)
    trackMatchedRules(collector);

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleRuleUsageTracker.h
(right):

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleRuleUsageTracker.h:16: void
markRuleAsUsed(StyleRule* rule) { m_ruleList.add(rule); }
rename: track(StyleRule* rule)

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/StyleEngine.cpp:440:
m_resolver->setRuleUsageTracker(m_tracker);
m_resolver->setRuleUsageTracker(true);

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/StyleEngine.cpp:446:
m_resolver->setRuleUsageTracker(m_tracker);
if (m_tracker)
  m_resolved->setRuleUsageTracker(true);


but where is m_resolved->setRuleUsageTracker(false) ?

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:782: Document*
document = loader->frame()->document();
It is better to listen to the DOM agent for new/departing documents.

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2502:
HeapVector<Member<Document>> documents = m_domAgent->documents();
What happens to the detached documents?

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h (right):

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h:132: void
setTrackerStatus(bool enable);
setUsageTrackerEnabled

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h:217: void
setSelectorRecording(ErrorString*, bool enable) override;
Be consistent with the rest of the profilers and call it
start/stopUsageTracking, stop returns the result.

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h:219: void
getRuleList(ErrorString*,
And you don't need this one.

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2773: { "name":
"ruleType", "type": "string", "optional": true, "enum": ["Style", "Charset",
"Import", "Media", "FontFace", "Page", "Keyframes", "Keyframe", "Namespace",
"Supports", "Viewport"] },
Can we expose it when we need it?

[caseq, 2016-10-31 21:06:46]

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/StyleEngine.cpp:440:
m_resolver->setRuleUsageTracker(m_tracker);
On 2016/10/31 21:03:35, pfeldman wrote:
> m_resolver->setRuleUsageTracker(true);

I think what you mean here is
if (m_resolver)
  m_resolver->setRuleUsageTracker(m_tracker)

https://codereview.chromium.org/2455943003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/StyleEngine.cpp:446:
m_resolver->setRuleUsageTracker(m_tracker);
On 2016/10/31 21:03:35, pfeldman wrote:
> if (m_tracker)
>   m_resolved->setRuleUsageTracker(true);
> 
> 
> but where is m_resolved->setRuleUsageTracker(false) ?

This is fine, we're just creating the new resolver here.

[caseq, 2016-10-31 23:47:59]

https://codereview.chromium.org/2455943003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/2455943003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2533: *result =
protocol::Array<protocol::CSS::RuleUsage>::create();
This needs to be after bail out on return -- you can't return both result and
error.

https://codereview.chromium.org/2455943003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2553:
CSSStyleRule* cssRule = static_cast<CSSStyleRule*>(rule.get());
You can't expect all of them are style rules, can you?

https://codereview.chromium.org/2455943003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h (right):

https://codereview.chromium.org/2455943003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h:326: void
didLoadDocument(Document*) override;
make it didAddDocument to emphasize it's symmetric to didRemoveDocument

[valih, 2016-11-01 01:24:36]

The CQ bit was checked by valih@google.com

[valih, 2016-11-01 01:24:36]

The patchset sent to the CQ was uploaded after l-g-t-m from tomjamri@gmail.com,
rune@opera.com
Link to the patchset: https://codereview.chromium.org/2455943003/#ps220001
(title: "Backend for css rule tracking")

[valih, 2016-11-01 01:24:37]

The CQ bit was unchecked by valih@google.com

[valih, 2016-11-01 01:24:39]

The CQ bit was checked by valih@google.com

[valih, 2016-11-01 01:24:40]

The CQ bit was unchecked by valih@google.com

[caseq, 2016-11-01 01:25:56]

The CQ bit was checked by caseq@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 01:26:24]

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

[commit-bot: I haz the power, 2016-11-01 01:28:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-01 01:28:40]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[caseq, 2016-11-01 01:28:42]

lgtm, but please also get one from pfeldman

[valih, 2016-11-01 19:06:43]

Patchset #13 (id:240001) has been deleted

[valih, 2016-11-01 19:18:29]

valih@google.com changed reviewers:
- rune@opera.com

[valih, 2016-11-01 19:23:16]

The CQ bit was checked by valih@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 19:23:42]

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

[commit-bot: I haz the power, 2016-11-01 20:53:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-01 20:53:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[valih, 2016-11-02 01:42:50]

The CQ bit was checked by valih@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 01:43:07]

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

[commit-bot: I haz the power, 2016-11-02 03:32:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 03:32:47]

Dry run: This issue passed the CQ dry run.

[valih, 2016-11-02 16:59:28]

Description was changed from

==========
Backend for css rule tracking
==========

to

==========
Patch 15 passed the tests. In patch 16 i just deleted 1 empty line that was
added when i rebased.
==========

[valih, 2016-11-02 16:59:54]

Description was changed from

==========
Patch 15 passed the tests. In patch 16 i just deleted 1 empty line that was
added when i rebased.
==========

to

==========
Patch 15 passed the tests.
In patch 16 I just deleted 1 empty line that was added when I rebased.
==========

[pfeldman, 2016-11-02 18:38:26]

https://codereview.chromium.org/2455943003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleRuleUsageTracker.h
(right):

https://codereview.chromium.org/2455943003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleRuleUsageTracker.h:18: const
HeapHashSet<Member<StyleRule>>& getRuleList() const;
You could all "contains" into the tracker API for more elegant use.

https://codereview.chromium.org/2455943003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/2455943003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2508: void
InspectorCSSAgent::setUsageTrackerStatus(bool enable) {
rename "enable" to "enabled"

https://codereview.chromium.org/2455943003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2533:
CSSStyleRule* cssRule = static_cast<CSSStyleRule*>(rule);
This will not always work, you need a type check (or change the method signature
to receive CSSStyleRule)

https://codereview.chromium.org/2455943003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2541:
inspectorStyleSheet->buildObjectForRuleUsage(rule, status);
You could use stylesheet to build rule and add usage here on your own.

https://codereview.chromium.org/2455943003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp (right):

https://codereview.chromium.org/2455943003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorStyleSheet.cpp:1605:
InspectorStyleSheet::buildObjectForRuleUsage(CSSRule* rule, bool enable) {
rename enable to wasUsed

https://codereview.chromium.org/2455943003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2455943003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:2811:
"description": "CSS rule usage information."
"experimental": true

https://codereview.chromium.org/2455943003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3191: { "name":
"CSSRules", "type": "array", "items": { "$ref": "RuleUsage" } }
Rename "CSSRules" to "ruleUsage"

https://codereview.chromium.org/2455943003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js (right):

https://codereview.chromium.org/2455943003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js:455: function
usedRulesCallback(error, CSSRules) {
variables start with lower case: cssRules or even better ruleUsage.

https://codereview.chromium.org/2455943003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js:1017:
WebInspector.CSSModel.RuleTextRange;
RuleTextRange should not operate terms such as "wasUsed". Why text range? You
should only operate raw locations for usage, then convert them to UILocations
using bindings. Talk to lushnikov@ about it!

https://codereview.chromium.org/2455943003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/CSSRule.js (right):

https://codereview.chromium.org/2455943003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/CSSRule.js:110: this.wasUsed =
wasUsed;
wasUsed || false;

[caseq, 2016-11-02 20:33:54]

https://codereview.chromium.org/2455943003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right):

https://codereview.chromium.org/2455943003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:2532:
InspectorCSSAgent::buildObjectForRuleUsage(CSSStyleRule* rule, bool status) {
s/status/used/?

https://codereview.chromium.org/2455943003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h (right):

https://codereview.chromium.org/2455943003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h:130: void
setUsageTrackerStatus(bool enable);
enabled

[valih, 2016-11-02 21:30:47]

Description was changed from

==========
Patch 15 passed the tests.
In patch 16 I just deleted 1 empty line that was added when I rebased.
==========

to

==========

==========

[caseq, 2016-11-03 00:13:08]

https://codereview.chromium.org/2455943003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js (right):

https://codereview.chromium.org/2455943003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/CSSModel.js:1017:
WebInspector.CSSModel.RuleUsageTracker;
Why is it called tracker? Is it tracking something? Let's call it just
WebInspector.CSSModel.RuleUsage

[caseq, 2016-11-03 20:00:17]

https://codereview.chromium.org/2455943003/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2455943003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3187:
"description": "Enables the selector recording."
"experimental": true

https://codereview.chromium.org/2455943003/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:3190: "name":
"stopRuleUsageTracking",
ditto

[pfeldman, 2016-11-03 21:23:30]

lgtm % comment.

As for the reportUsage(Tracker) you can rename your getter with
matchedRuleListForInspector() if you want an easy alternative.

https://codereview.chromium.org/2455943003/diff/440001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector-protocol/css/css-get-rule-list.html
(right):

https://codereview.chromium.org/2455943003/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector-protocol/css/css-get-rule-list.html:25:
var usedLines = rules.filter(rule => rule.used), unusedLines = rules.filter(rule
=> !rule.used);
Single statement per line please.

https://codereview.chromium.org/2455943003/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector-protocol/css/css-get-rule-list.html:31:
for(var i = 0; i < usedLines.length; ++i)
You can use for (var line of usedLines)

https://codereview.chromium.org/2455943003/diff/440001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector-protocol/css/css-get-rule-list.html:35:
for(var i = 0; i < unusedLines.length; ++i)
ditto

https://codereview.chromium.org/2455943003/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/ElementRuleCollector.h (right):

https://codereview.chromium.org/2455943003/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/ElementRuleCollector.h:146: const
HeapVector<MatchedRule, 32>& matchedRulesList() const {
I don't think you should expose otherwise encapsulated list. There is a better
alternative:

void reportUsage(Tracker tracker)

[valih, 2016-11-03 22:10:43]

The CQ bit was checked by valih@google.com

[valih, 2016-11-03 22:10:44]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org,
rune@opera.com, tomjamri@gmail.com, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2455943003/#ps460001
(title: "Backend for CSS Rule tracking.")

[valih, 2016-11-03 22:10:55]

The CQ bit was unchecked by valih@google.com

[valih, 2016-11-03 22:54:54]

The CQ bit was checked by valih@google.com

[commit-bot: I haz the power, 2016-11-03 22:55:46]

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

[commit-bot: I haz the power, 2016-11-03 22:59:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 22:59:52]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[valih, 2016-11-03 23:29:02]

The CQ bit was checked by valih@google.com

[valih, 2016-11-03 23:29:03]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org,
pfeldman@chromium.org, tomjamri@gmail.com, rune@opera.com
Link to the patchset: https://codereview.chromium.org/2455943003/#ps480001
(title: "Backend for CSS Rule tracking.")

[commit-bot: I haz the power, 2016-11-03 23:29:49]

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

[commit-bot: I haz the power, 2016-11-04 00:33:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 00:33:52]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[valih, 2016-11-04 01:41:41]

Description was changed from

==========

==========

to

==========
- backend code for css rule tracking
- 2 new browser protocol functions : "startRuleUsageTracking" and
"stopRuleUsageTracking" that also returns the rule usage list.

BUG=662260
==========

[valih, 2016-11-04 01:41:53]

The CQ bit was checked by valih@google.com

[commit-bot: I haz the power, 2016-11-04 01:43:12]

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

[commit-bot: I haz the power, 2016-11-04 05:19:49]

Description was changed from

==========
- backend code for css rule tracking
- 2 new browser protocol functions : "startRuleUsageTracking" and
"stopRuleUsageTracking" that also returns the rule usage list.

BUG=662260
==========

to

==========
- backend code for css rule tracking
- 2 new browser protocol functions : "startRuleUsageTracking" and
"stopRuleUsageTracking" that also returns the rule usage list.

BUG=662260
==========

[commit-bot: I haz the power, 2016-11-04 05:19:51]

Committed patchset #24 (id:480001)

[commit-bot: I haz the power, 2016-11-04 05:21:27]

Description was changed from

==========
- backend code for css rule tracking
- 2 new browser protocol functions : "startRuleUsageTracking" and
"stopRuleUsageTracking" that also returns the rule usage list.

BUG=662260
==========

to

==========
- backend code for css rule tracking
- 2 new browser protocol functions : "startRuleUsageTracking" and
"stopRuleUsageTracking" that also returns the rule usage list.

BUG=662260

Committed: https://crrev.com/68f60f6cfa8d0b968ed9a90e2188dd84c62788e5
Cr-Commit-Position: refs/heads/master@{#429792}
==========

[commit-bot: I haz the power, 2016-11-04 05:21:29]

Patchset 24 (id:??) landed as
https://crrev.com/68f60f6cfa8d0b968ed9a90e2188dd84c62788e5
Cr-Commit-Position: refs/heads/master@{#429792}

[valih, 2016-12-14 00:46:16]

Patchset #23 (id:460001) has been deleted