Summary: | Web Inspector: Show media queries associated with specific CSS rules | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zach Leatherman <zachleatherman> | ||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Alexander Pavlov (apavlov) <apavlov> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, joepeck, keishi, koivisto, loislo, macpherson, mathias, paulirish, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Zach Leatherman
2011-08-03 10:20:41 PDT
Created attachment 114082 [details]
Patch
Comment on attachment 114082 [details] Patch 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. Created attachment 114289 [details]
Patch
A more complete solution has been attached, which enables navigation to media lists in the chain, when possible. (In reply to comment #2) > (From update of attachment 114082 [details]) > 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? Fixed > > Source/WebCore/inspector/InspectorStyleSheet.cpp:750 > > + for (size_t i = mediaVector.size(); i > 0; --i) > > We usually do = size - 1; i >=0; --i Heh. It's a size_t which is unsigned. Resorted to building an outward-traversal list and reversing it while rendering. > 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. It was not SELF MEDIA but media query of the parent stylesheet, the root one. But I've found out that @import rules can also have media queries, so the traversal is slightly more involved (see the extracted method). > > 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? Fixed. > > 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. Done. Comment on attachment 114289 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114289&action=review Inspector looks good. Please make sure CSS folks are file with the m_lastLine. It looks sane to me. > Source/WebCore/inspector/InspectorStyleSheet.cpp:136 > + default: Removing default will convert it from runtime to compile time error. Created attachment 114292 [details]
[IMAGE] Screenshot of the Styles sidebar pane with media queries displayed
Created attachment 114455 [details]
Patch
[PATCH] Fix winbot compilability, adjust linked URLs display
Created attachment 114458 [details]
Patch
[PATCH] Re-submit for Windows EWS bot (some weird bot issue occurred)
Comment on attachment 114458 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114458&action=review > Source/WebCore/css/MediaList.h:89 > + int lastLine() const { return m_lastLine; } > + void updateLastLine(int lastLine) { m_lastLine = lastLine; } Can lastLine be negative? If not, why signed type? WebKit coding style is to usually name setters as "setFoo". Looks ok otherwise. (In reply to comment #9) > (From update of attachment 114458 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114458&action=review > > > Source/WebCore/css/MediaList.h:89 > > + int lastLine() const { return m_lastLine; } > > + void updateLastLine(int lastLine) { m_lastLine = lastLine; } > > Can lastLine be negative? If not, why signed type? CSS classes (CSSParser, CSSStyleRule) use "int" to that end, so I decided to stick to the same type. > WebKit coding style is to usually name setters as "setFoo". Will fix. Created attachment 114480 [details]
Patch
Comment from anttik fixed
Committed r99849: <http://trac.webkit.org/changeset/99849> |