1118236 - Computed side panel isn't properly initialized

Status: RESOLVED WORKSFORME

(DevTools :: Inspector, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Cloned from https://github.com/firebug/firebug.next/issues/255

Firebug.next seems to produce an empty computed properties panel when run on Nightly and DevEd. This is caused by |getComputedStyle| method that returns null if the parent side bar is hidden at the moment. If this happens the CssHtmlTree.propertyNames isn't filled by supported css rules.

let styles = this.styleWindow.getComputedStyle(this.styleDocument.documentElement);

CssHtmlTree.propertyNames must be refreshed when the side bar is opened for the first time.

Honza

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1118236-1.patch

I am attaching a patch that solves the problem (making sure the CssHtmlTree.propertyNames is properly initialized).

Honza

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8544585 [details] [diff] [review]
bug1118236-1.patch

Review of attachment 8544585 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch with this. I hadn't thought of that because in the native tools, the sidebar can't be hidden (yet! we have a patch to make this happen, but it hasn't landed).
I just think the code should be refactored a bit for this particular issue to be solved in the internals of computed-view instead of being fixed from outside in style-inspector.

::: browser/devtools/styleinspector/computed-view.js
@@ +435,5 @@
>        onDone: () => {
>          // Completed callback.
>          this.element.appendChild(fragment);
>          this.noResults.hidden = this.numVisibleProperties > 0;
> +        this._createViewsPromise = null;

Why are you setting the promise to null here? If the view creation process was done successfully, it doesn't need to be redone later, so this._createViewsPromise should still exist in order for subsequent calls to _createPropertyViews to early return.

::: browser/devtools/styleinspector/style-inspector.js
@@ +215,5 @@
>      }
>    },
>  
>    onPanelSelected: function() {
> +    // Make sure that the cache with a list of css properties supported

This comment and the block of code below are very internal to how the computed-view works, so it feels strange for the style-inspector to do it.
This needs to move into computed-view.js instead.

In fact, I think we should remove everything that has to do with initializing markup in the computed-view constructor (in function CssHtmlTree) and instead lazily do it in CssHtmlTree.prototype.refreshPanel.

I don't have the whole flow of the computed-view in mind right now, so I might be forgetting something, but doing this would solve both your problem (because if the style views and property views would be created when needed thanks to the onSidebarShow listener you added) and would make the inspector initialize even faster.

Comment 3

[Julian Descottes [:jdescottes]]

(In reply to Patrick Brosset [:pbro] from comment #2)
> Comment on attachment 8544585 [details] [diff] [review]
> bug1118236-1.patch
>
> In fact, I think we should remove everything that has to do with
> initializing markup in the computed-view constructor (in function
> CssHtmlTree) and instead lazily do it in CssHtmlTree.prototype.refreshPanel.
> 

Looks like this was implemented in the meantime. The original firebug.next issue has been closed. Closing this one.

Inspector bug triage. Filter on CLIMBING SHOES.