[Webkit-unassigned] [Bug 120640] Web Inspector: Jump from a computed style to the rule it came from

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=120640

Timothy Hatcher <timothy at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #253718|review?                     |review-
              Flags|                            |

--- Comment #6 from Timothy Hatcher <timothy at apple.com> ---
Comment on attachment 253718
  --> https://bugs.webkit.org/attachment.cgi?id=253718
Patch

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

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.

> Source/WebInspectorUI/ChangeLog:9
> +        * UserInterface/Views/CSSStyleDeclarationSection.js:
> +        (WebInspector.CSSStyleDeclarationSection.prototype.refreshPropertiesTextEditor):

You should add comments under each function or file group about what you did and why.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:263
> +    refreshPropertiesTextEditor: function() {

{ on newline.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:264
> +        this._propertiesTextEditor.refresh();

Might not need this in the end, see below.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:440
> +        } else if(this._delegate instanceof WebInspector.ComputedStyleDetailsPanel) {

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.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:470
> +        if(property.highlighted) {

Space after if.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:159
> +        var rsdp = this._rulesStyleDetailsPanel;

We don't abbreviate things like this. Just use "rulesStyleDetailsPanel" or "rulesPanel".

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:160
> +        if(rsdp.sections.length == 0) {

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?

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:161
> +            var interval = setInterval(function() {

No need to save the interval.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:162
> +                if(rsdp.sections.length > 0) {

if (rsdp.sections.length)

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:164
> +                    clearInterval(interval);

No need to clearInterval for a timeout, it is already single fire.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:169
> +        } else {
> +            rsdp.scrollToSectionAndHighlightProperty(property);
> +        }

No { } for single line blocks.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:248
> +        for(var i = 0; i < this._sections.length; ++i) {

Space after for.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:249
> +            var section = this._sections[i];

Use for (var section of this._sections) and you don't need the i or this line.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:251
> +            for(var j = 0; j < section.style.properties.length; ++j) {

Use for (...of...)

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:253
> +                    var elements = section.element.querySelectorAll(".properties .css-style-text-editor .CodeMirror-scroll .CodeMirror-code pre");

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.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:255
> +                    for(var k = 0; k < elements.length; ++k) {

Use for (...of...)

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:260
> +                            section.style.properties[j].highlighted = true;
> +                            section.refreshPropertiesTextEditor();
> +                            section.needsToUpdate = true;

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.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:270
> +            if(propertyName) {
> +                break;
> +            }

Space after if. No need for { }

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:273
> +        function propertiesMatch(cssProperty) {

Newline before {.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:274
> +            if(cssProperty.enabled && !cssProperty.overridden) {

Space before if.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:275
> +                if(cssProperty.canonicalName == property.canonicalName || hasMatchingLonghandProperty(cssProperty)) {

Ditto.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:280
> +            }
> +            return false;

Newline between.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:283
> +        function hasMatchingLonghandProperty(cssProperty) {

Newline before {.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:286
> +            if(cssProperties.length === 0)

Space after if. !cssProperties.length

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:289
> +            for(var i = 0; i < cssProperties.length; ++i) {

for (...of...)

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:290
> +                if(propertiesMatch(cssProperties[i])) {

No need for {}.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:294
> +            }
> +            return false;

Newline between.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:317
> +            if(section.needsToUpdate) {
> +                section.refreshPropertiesTextEditor();
> +                delete section.needsToUpdate;
> +            }

Might not need this if you move things into CSSStyleDeclarationTextEditor and don't do a full refresh.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150526/5731923a/attachment.html>