Bug 75378 – Web Inspector: introduce "source" column in the CSS profiler.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

Otherwise, I can't jump to the rule definition and fix it.

[Alexander Pavlov (apavlov)]

(In reply to comment #0)
> Otherwise, I can't jump to the rule definition and fix it.

Good idea. As per the ChangeLog comment, multiple rules with the same selectorText are merged into one entry, so there's no good universal way to bind them to a source location. Suggestions?

[Pavel Feldman]

(In reply to comment #1)
> (In reply to comment #0)
> > Otherwise, I can't jump to the rule definition and fix it.
> 
> Good idea. As per the ChangeLog comment, multiple rules with the same selectorText are merged into one entry, so there's no good universal way to bind them to a source location. Suggestions?

I would stack them and make the row a bit taller:

selector      source
------------------
.foo,         foo.css:15
.bar          bar.css:25
------------------
.baz          baz.css:11
------------------
...

[Pavel Feldman]

Scratch that...

> selector      source
> ------------------
> .foo           foo.css:15
>                  bar.css:25
> ------------------
> .baz          baz.css:11
> ------------------
> ...

[Alexander Pavlov (apavlov)]

Created attachment 120795 [details]
Patch

[Alexander Pavlov (apavlov)]

(In reply to comment #4)
> Created an attachment (id=120795) [details]
> Patch

EWS Mac builder currently not available

[Alexander Pavlov (apavlov)]

Created attachment 120797 [details]
[PATCH] Enable ellipsis for long source links, fix leftmost statusBarItems position (heapshot button removed recently)

[Pavel Feldman]

Comment on attachment 120797 [details]
[PATCH] Enable ellipsis for long source links, fix leftmost statusBarItems position (heapshot button removed recently)

View in context: https://bugs.webkit.org/attachment.cgi?id=120797&action=review

> Source/WebCore/inspector/InspectorCSSAgent.cpp:103
> +    typedef std::pair<String, unsigned> SelectorLocation;

Is it time for SelectorProfile to move into its own file.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:152
> +inline bool SelectorProfile::isRegularRule(const CSSStyleRule* rule)

You should not need to inline heavy methods.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:154
> +    CSSStyleSheet* parentStyleSheet = rule->parentStyleSheet();

This logic looks familiar. InspectorCSSAgent::detectOrigin ?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:165
> +inline String SelectorProfile::sourceURL(const CSSStyleRule* rule)

ditto: no need to inline, looks like the logic could be reused.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:189
> +    RuleMatchingStatsMap::iterator statsByLocationIt = m_ruleMatchingStats.find(m_currentMatchData.selector);

Why did this change? It looks like a lot of boilerplate code. Could it be that you've chosen a wrong model for storing the data here?

[Pavel Feldman]

Comment on attachment 120797 [details]
[PATCH] Enable ellipsis for long source links, fix leftmost statusBarItems position (heapshot button removed recently)

View in context: https://bugs.webkit.org/attachment.cgi?id=120797&action=review

> Source/WebCore/inspector/front-end/profilesPanel.css:191
> +    color: white;

color: white should apply to all columns, why only > a?

[Alexander Pavlov (apavlov)]

(In reply to comment #8)
> (From update of attachment 120797 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120797&action=review
> 
> > Source/WebCore/inspector/front-end/profilesPanel.css:191
> > +    color: white;
> 
> color: white should apply to all columns, why only > a?

Selected TD color is set up in dataGrid.css but A does not inherit this as it has the a:-webkit-any-link rule in the user agent stylesheet (blue), so it is roughly the same color as the selected row background.

[Alexander Pavlov (apavlov)]

(In reply to comment #7)
> (From update of attachment 120797 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120797&action=review
> 
> > Source/WebCore/inspector/InspectorCSSAgent.cpp:103
> > +    typedef std::pair<String, unsigned> SelectorLocation;
> 
> Is it time for SelectorProfile to move into its own file.

This is not going to grow due to the following comments.

> > Source/WebCore/inspector/InspectorCSSAgent.cpp:152
> > +inline bool SelectorProfile::isRegularRule(const CSSStyleRule* rule)
> 
> You should not need to inline heavy methods.

It is no longer used, thanks to the fact that I made use of InspectorDOMAgent::documentURLString(). sourceURL() can be inlined (it only has one call site.)

> > Source/WebCore/inspector/InspectorCSSAgent.cpp:154
> > +    CSSStyleSheet* parentStyleSheet = rule->parentStyleSheet();
> 
> This logic looks familiar. InspectorCSSAgent::detectOrigin ?

Yeah, it's that the algo requires the document which is retrieved from the agent instance, which does not help in this case.

> > Source/WebCore/inspector/InspectorCSSAgent.cpp:165
> > +inline String SelectorProfile::sourceURL(const CSSStyleRule* rule)
> 
> ditto: no need to inline, looks like the logic could be reused.

Yes, will reuse.

> > Source/WebCore/inspector/InspectorCSSAgent.cpp:189
> > +    RuleMatchingStatsMap::iterator statsByLocationIt = m_ruleMatchingStats.find(m_currentMatchData.selector);
> 
> Why did this change? It looks like a lot of boilerplate code. Could it be that you've chosen a wrong model for storing the data here?

We used to coalesce rule stats by selector in the backend (yet we've always had enough data to collect all the stats rule-wise.) Now we split the selector-wise statistics by location (sourceURL + sourceLine) to avoid linear location lookup among the entries for a particular selector. Since we don't need this two-layer mapping in the protocol (and in the frontend to represent the data,) we flatten the second-level map down. Is this wrong?

[Alexander Pavlov (apavlov)]

Created attachment 121829 [details]
Patch

[Pavel Feldman]

Comment on attachment 121829 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121829&action=review

> Source/WebCore/inspector/Inspector.json:1632
> +                    { "name": "sourceURL", "type": "string", "description": "URL of the resource containing the corresponding rule." },

url

> Source/WebCore/inspector/Inspector.json:1633
> +                    { "name": "sourceLine", "type": "integer", "description": "Selector line number in the resource for the corresponding rule." },

lineNumber

[Alexander Pavlov (apavlov)]

Committed r104710: <http://trac.webkit.org/changeset/104710>