Bug 31629 – Web Inspector: Prevent styles information from being queries twice for each update, get rid of metrics and properties sidebars' flickering.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Feldman]

This makes styles and metrics panes aware of each other, but results in a solid performance improvement.

[Pavel Feldman]

Created attachment 43434 [details]
[PATCH] proposed fix

[Joseph Pecoraro]

Comment on attachment 43434 [details]
[PATCH] proposed fix

I have some small nits.

> +        Web Inspector: Prevent styles information from being queries twice
> +        for each update, get rid of metrics and properties sidebars' flickering.

typo: s/queries/queried/;

>              if (name === "content") {
> -                var width = style.width.replace(/px$/, "");
> +                var width = computedStyle.width.replace(/px$/, "");

This is my first time coming across this code. Is the width coming from computedStyle guaranteed to be "px"? That seems the case from the older code. This might be a good spot to add a comment. If it is not just "px" then using something like parseInt(..., 10) would strip any unit suffix (em, px, etc.). However, this isn't really a candidate for optimization so it could just be left alone.

> +
> +        // Update slave metrics pane too.
> +        this._metricsPane.update(node);
> +    },
> +
> +    set metricsPane(metricsPane)
> +    {
> +        this._metricsPane = metricsPane;
>      },

These are the only mentions of _metricsPane. If a metricsPane has not been set then the above line would fail (as there is no check if this._metricsPane exists). Obviously that will never be the case because there is only one StylesSidebarPane =/. Being the last line in the method a silent fail wouldn't be too harmful. Still, I think it would be safer to pass the metricsPane into the constructor, or do an if check before calling update on it.

[Pavel Feldman]

(In reply to comment #2)
> 
> typo: s/queries/queried/;
> 

Fixed.

> >              if (name === "content") {
> > -                var width = style.width.replace(/px$/, "");
> > +                var width = computedStyle.width.replace(/px$/, "");
> 
> This is my first time coming across this code. Is the width coming from
> computedStyle guaranteed to be "px"? That seems the case from the older code.
> This might be a good spot to add a comment. If it is not just "px" then using
> something like parseInt(..., 10) would strip any unit suffix (em, px, etc.).
> However, this isn't really a candidate for optimization so it could just be
> left alone.
> 

Yeah, it is px.

> > +
> > +        // Update slave metrics pane too.
> > +        this._metricsPane.update(node);
> > +    },
> > +
> > +    set metricsPane(metricsPane)
> > +    {
> > +        this._metricsPane = metricsPane;
> >      },
> 
> These are the only mentions of _metricsPane. If a metricsPane has not been set
> then the above line would fail (as there is no check if this._metricsPane
> exists). Obviously that will never be the case because there is only one
> StylesSidebarPane =/. Being the last line in the method a silent fail wouldn't
> be too harmful. Still, I think it would be safer to pass the metricsPane into
> the constructor, or do an if check before calling update on it.

I agree and that is what i did to MetricsPane. The thing is that dependency is bi-directional :( and I can't pass both into the constructors. I don't want a check here, since having no metricsPane means that we are in the illegal app state. I'd much more appreciate real time exception with console entry in this case...

[Timothy Hatcher]

Comment on attachment 43434 [details]
[PATCH] proposed fix


> +                 var height = computedStyle.height.replace(/px$/, "");

Joe might be on to something, we could use parseInt() here and it would be faster than the RegEx.


> -        body.removeChildren();
> -
> -        this.sections = [];
> -
> -        if (!node)
> +        if (!node) {
> +            body.removeChildren();
> +            this.sections = [];
>              return;
> +        }

Why do we not always need to remove the children and sections? Does later code handle them existing?


>          }
> -
>          InjectedScriptAccess.getStyles(node.id, !Preferences.showUserAgentStyles, callback);

I prefer blank lines in places like this. Can you keep it?

I don't clearly see where this prevents two style queries. Wouldn't it be better to have the ElementsPanel broker these updates so there isn't a bi-directional dependency?

But seems fine.

[Pavel Feldman]

(In reply to comment #4)
> (From update of attachment 43434 [details])
> 
> > +                 var height = computedStyle.height.replace(/px$/, "");
> 
> Joe might be on to something, we could use parseInt() here and it would be
> faster than the RegEx.
> 

Ok. let me try that.

> > -        body.removeChildren();
> > -
> > -        this.sections = [];
> > -
> > -        if (!node)
> > +        if (!node) {
> > +            body.removeChildren();
> > +            this.sections = [];
> >              return;
> > +        }
> 
> Why do we not always need to remove the children and sections? Does later code
> handle them existing?
> 

Yes, this is only for the case when we navigate off the node that has data (comments, etc.). Later code handles it Ok.

> 
> >          }
> > -
> >          InjectedScriptAccess.getStyles(node.id, !Preferences.showUserAgentStyles, callback);
> 
> I prefer blank lines in places like this. Can you keep it?
> 

Ok.

> I don't clearly see where this prevents two style queries. Wouldn't it be
> better to have the ElementsPanel broker these updates so there isn't a
> bi-directional dependency?
> 
> But seems fine.

Of course you are right and I should make ElementsPanel broker it. It was just me being lazy. This one prevents two queries since metrics panel does not do its own one anymore. Let me actually split this change into two: first would remove flickering and latter one will make elements panel manage the updates.

[Pavel Feldman]

Created attachment 43439 [details]
[PATCH] Flickering only fix.

[Timothy Hatcher]

Comment on attachment 43439 [details]
[PATCH] Flickering only fix.

Thanks for splitting this out.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/MetricsSidebarPane.js
	M	WebCore/inspector/front-end/PropertiesSidebarPane.js
Committed r51121