[webkit-reviews] review denied: [Bug 75378] Web Inspector: introduce "source" column in the CSS profiler. : [Attachment 120797] [PATCH] Enable ellipsis for long source links, fix leftmost statusBarItems position (heapshot button removed recently)

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 75378: Web Inspector: introduce "source" column in the CSS profiler.
https://bugs.webkit.org/show_bug.cgi?id=75378

Attachment 120797: [PATCH] Enable ellipsis for long source links, fix leftmost
statusBarItems position (heapshot button removed recently)
https://bugs.webkit.org/attachment.cgi?id=120797&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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?