<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:timothy@apple.com" title="Timothy Hatcher <timothy@apple.com>"> <span class="fn">Timothy Hatcher</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: Jump from a computed style to the rule it came from"
href="https://bugs.webkit.org/show_bug.cgi?id=120640">bug 120640</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #253718 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: Jump from a computed style to the rule it came from"
href="https://bugs.webkit.org/show_bug.cgi?id=120640#c6">Comment # 6</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: Jump from a computed style to the rule it came from"
href="https://bugs.webkit.org/show_bug.cgi?id=120640">bug 120640</a>
from <span class="vcard"><a class="email" href="mailto:timothy@apple.com" title="Timothy Hatcher <timothy@apple.com>"> <span class="fn">Timothy Hatcher</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=253718&action=diff" name="attach_253718" title="Patch">attachment 253718</a> <a href="attachment.cgi?id=253718&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=253718&action=review">https://bugs.webkit.org/attachment.cgi?id=253718&action=review</a>
Very good first patch! I commented most about style nits. But there is some changes you can make to make this more integrated and not have as many changes.
<span class="quote">> Source/WebInspectorUI/ChangeLog:9
> + * UserInterface/Views/CSSStyleDeclarationSection.js:
> + (WebInspector.CSSStyleDeclarationSection.prototype.refreshPropertiesTextEditor):</span >
You should add comments under each function or file group about what you did and why.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:263
> + refreshPropertiesTextEditor: function() {</span >
{ on newline.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:264
> + this._propertiesTextEditor.refresh();</span >
Might not need this in the end, see below.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:440
> + } else if(this._delegate instanceof WebInspector.ComputedStyleDetailsPanel) {</span >
Add a space after if.
This check, while correct, is a layering violation. This code should not know about ComputedStyleDetailsPanel.
What this should do is check for a function on the delegate (like "this._delegate.cssStyleDeclarationTextEditorShouldAddPropertyGoToArrows") then call that function to see if it returns true. if it returns true, then you can do the code like you have it. And you should change WebInspector.cssStyleDetailsSidebarPanel.switchViewToStyleInRulesStyleDetailsPanel to another delegate call.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:470
> + if(property.highlighted) {</span >
Space after if.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:159
> + var rsdp = this._rulesStyleDetailsPanel;</span >
We don't abbreviate things like this. Just use "rulesStyleDetailsPanel" or "rulesPanel".
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:160
> + if(rsdp.sections.length == 0) {</span >
Space after if.
!rsdp.sections.length instead of "== 0" (should have been === too)
Should add a comment why this code is needed. Why the timeout?
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:161
> + var interval = setInterval(function() {</span >
No need to save the interval.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:162
> + if(rsdp.sections.length > 0) {</span >
if (rsdp.sections.length)
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:164
> + clearInterval(interval);</span >
No need to clearInterval for a timeout, it is already single fire.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:169
> + } else {
> + rsdp.scrollToSectionAndHighlightProperty(property);
> + }</span >
No { } for single line blocks.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:248
> + for(var i = 0; i < this._sections.length; ++i) {</span >
Space after for.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:249
> + var section = this._sections[i];</span >
Use for (var section of this._sections) and you don't need the i or this line.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:251
> + for(var j = 0; j < section.style.properties.length; ++j) {</span >
Use for (...of...)
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:253
> + var elements = section.element.querySelectorAll(".properties .css-style-text-editor .CodeMirror-scroll .CodeMirror-code pre");</span >
This is fragile. It could easily break with a CodeMirror change and we wouldn't catch it.
This should be contained in a helper on CSSStyleDeclarationTextEditor, as a revealAndHighlightProperty function. CSSProperty.js has styleDeclarationTextRange and CSSStyleDeclarationTextEditor can use it to scroll to a property.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:255
> + for(var k = 0; k < elements.length; ++k) {</span >
Use for (...of...)
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:260
> + section.style.properties[j].highlighted = true;
> + section.refreshPropertiesTextEditor();
> + section.needsToUpdate = true;</span >
Doing a refresh to get the highlight works, but it is heavy. _updateTextMarkers on CSSStyleDeclarationTextEditor is all you should need to do. That is a private function, so moving this logic to CSSStyleDeclarationTextEditor will make sense.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:270
> + if(propertyName) {
> + break;
> + }</span >
Space after if. No need for { }
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:273
> + function propertiesMatch(cssProperty) {</span >
Newline before {.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:274
> + if(cssProperty.enabled && !cssProperty.overridden) {</span >
Space before if.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:275
> + if(cssProperty.canonicalName == property.canonicalName || hasMatchingLonghandProperty(cssProperty)) {</span >
Ditto.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:280
> + }
> + return false;</span >
Newline between.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:283
> + function hasMatchingLonghandProperty(cssProperty) {</span >
Newline before {.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:286
> + if(cssProperties.length === 0)</span >
Space after if. !cssProperties.length
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:289
> + for(var i = 0; i < cssProperties.length; ++i) {</span >
for (...of...)
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:290
> + if(propertiesMatch(cssProperties[i])) {</span >
No need for {}.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:294
> + }
> + return false;</span >
Newline between.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:317
> + if(section.needsToUpdate) {
> + section.refreshPropertiesTextEditor();
> + delete section.needsToUpdate;
> + }</span >
Might not need this if you move things into CSSStyleDeclarationTextEditor and don't do a full refresh.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>