[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 #253824|review?                     |review-
              Flags|                            |

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

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

Looking a lot better. Some more work and this will be nice and clean.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:488
> +                delegate.switchToPropertyInRulesPanel(property);

This should be something more generic than mentioning "rules panel". The delegate could be/do anything. The point of a delegate is to be generic and not know about the other parts of the code.

Something like: delegate.cssStyleDeclarationTextEditorShowProperty

You will need change the typeof check above too.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:146
> +            this._navigationBar.selectedNavigationItem = this._lastSelectedSectionSetting.value;

I would move this to _switchPanels.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:147
> +            this._navigationBar.updateLayout();

Not sure this is needed.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:156
> +        if (this._rulesStyleDetailsPanel.contentsHaveChanged(this.domNode)) {
> +            this._rulesStyleDetailsPanel.savedProperty = property;
> +            switchToRulesStyleDetailsPanel.call(this);
> +        } else {
> +            switchToRulesStyleDetailsPanel.call(this);
> +            this._rulesStyleDetailsPanel.scrollToSectionAndHighlightProperty(property);
> +        }

This should just be:

this._switchPanels(this._rulesStyleDetailsPanel);
this._rulesStyleDetailsPanel.scrollToSectionAndHighlightProperty(property);

Then scrollToSectionAndHighlightProperty should have the check for contentsHaveChanged, and internally save the property. The property saving should not be the job of the caller, it should be transparent to the caller. You wouldn't want to have to repeat this code if two or more classes needed to do it.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:-204
> -        this._lastSelectedSectionSetting.value = selectedNavigationItem.identifier;

This could stay, then all the callers to _switchPanels won't need to do it.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:149
> +        if (typeof this._delegate.switchViewToPropertyInRulesStyleDetailsPanel === "function" )
> +            this._delegate.switchViewToPropertyInRulesStyleDetailsPanel(property);

This should be generic, like "computedStyleDetailsPanelShowProperty".

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:34
> +        this._savedProperty = null;

_propertyToSelectAndHighlight would be a better name.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:250
> +    scrollToSectionAndHighlightProperty(property)
> +    {
> +        for (var section of this._sections) {

I think this should save to _propertyToSelectAndHighlight and return early if (!this.visible). Then nodeStylesRefreshed will be called when it becomes visible, which will call scrollToSectionAndHighlightProperty again. Should eliminate the need for contentsHaveChanged and _currentDOMNode.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:266
> +    get sections()
> +    {
> +        return this._sections;
> +    }
> +
> +    set savedProperty(cssProperty)
> +    {
> +        this._savedProperty = cssProperty;
>      }

Shouldn't need these.

> Source/WebInspectorUI/UserInterface/Views/StyleDetailsPanel.js:117
> +    contentsHaveChanged(domNode)
> +    {
> +        if (domNode && this._currentDOMNode && this._currentDOMNode === domNode)
> +            return false;
> +
> +        this._currentDOMNode = domNode;
> +
> +        return true;
> +    }

I don't think you need this. See scrollToSectionAndHighlightProperty.

-- 
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/20150528/d74ed4cc/attachment.html>