[webkit-reviews] review denied: [Bug 65626] Web Inspector: Show media queries associated with specific CSS rules : [Attachment 114082] Patch

[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 65626: Web Inspector: Show media queries associated with specific CSS rules
https://bugs.webkit.org/show_bug.cgi?id=65626

Attachment 114082: Patch
https://bugs.webkit.org/attachment.cgi?id=114082&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=114082&action=review


Could we get a screenshot for this? r- for a number of small problems.

> Source/WebCore/inspector/Inspector.json:-446
> -		   "description": "Enables console domain, sends the messages
collected so far to the client by means of the <code>messageAdded</code>
notification." 

This change will confuse the version control annotation. Could you not do it
please?

> Source/WebCore/inspector/InspectorStyleSheet.cpp:750
> +	   for (size_t i = mediaVector.size(); i > 0; --i)

We usually do = size - 1; i >=0; --i

So the order is self media followed by a reversed list (i.e. starting from
root?) This is confusing, the list should be either from root to current or
from current to root.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:843
> +	       this._mediaTextElement = this.titleElement.createChild("div",
"media");

So this section property will iterate over all of the elements and will not be
accessed from anywhere else? Can we use local variable instead?

> LayoutTests/inspector/styles/styles-iframe-expected.txt:9
> +[expanded] media="screen"@media screen#main { (styles-iframe.html:6)

A test involving nested medias would be great.