[webkit-reviews] review granted: [Bug 166787] Web Inspector: Frontend should be made to expect and handle disabled properties : [Attachment 320585] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 166787: Web Inspector: Frontend should be made to expect and handle
disabled properties
https://bugs.webkit.org/show_bug.cgi?id=166787

Attachment 320585: Patch

https://bugs.webkit.org/attachment.cgi?id=320585&action=review




--- Comment #7 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 320585
  --> https://bugs.webkit.org/attachment.cgi?id=320585
Patch

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

> Source/WebCore/ChangeLog:11
> +	   Add new test cases to inspector/css/css-property.html.

I think the normal syntax prepare-ChangeLog generates is:

    Tests: inspector/css/css-property.html
	   inspector/css/matched-style-properties.html

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:222
> +    get allProperties() { return this._allProperties; }

I see that `visibleProperties` below filters on _properties and not
_allProperties. Do disabled properties have a styleDeclarationTextRange? If so
we might end up including a allVisibleProperties in the future.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorSection.js:311
> -	   let styleEnabled = event && event.data && event.data.enabled;
> +	   let styleEnabled = event && event.data && event.data.attached;

This doesn't make sense. `enabled` is the event data associated with the
VisualStyleSelectorTreeItem CheckboxChanged event:

       
this.dispatchEventToListeners(WI.VisualStyleSelectorTreeItem.Event.CheckboxChan
ged, {enabled: this._checkboxElement.checked});

This should not change to `attached`.