Bug 31699 – Web Inspector: Should Cache Values of InspectorController.platform() and port

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

[Brian Weinstein]

These functions have to call into C++, they are being used more and more (keyboard shortcuts need to call it), and the values shouldn't change.

[Brian Weinstein]

Created attachment 43539 [details]
[PATCH] Fix

[Brian Weinstein]

Created attachment 43540 [details]
[PATCH] Use getters instead of WebInspector.function

[Timothy Hatcher]

Comment on attachment 43540 [details]
[PATCH] Use getters instead of WebInspector.function

> +        if (!this._platform)

A better idiom we use is:

          if (!("_platform" in this))

It is semi faster. It only checks if the property exists, not the value.

> +        if (!this._port)

Ditto.

[Pavel Feldman]

Given that we are 100% sure platform and port are needed for rendering, it does not really make sense to load them lazily. I'd rather push them into the frontend as soon as script object for it is available. Furthermore, I think we should do the same to inspector settings. Generic approach would be to create Preferences object (the one from inspector.js or its override) in InspectorController and push it into the frontend from within InspectorController::scriptObjectReady.

[Patrick Mueller]

(In reply to comment #4)
> I think we
> should do the same to inspector settings. Generic approach would be to create
> Preferences object (the one from inspector.js or its override) in
> InspectorController and push it into the frontend from within
> InspectorController::scriptObjectReady.

I didn't realize we already had a Preferences object; sounds like a good place to put the pre-load of the settings from InspectorController.  Do we create a method per setting, or two - a getter and setter - or a generic getter and setter?

Even though it gets busy, I like having method-per-setting, because then it's easy to figure out what all the possible settings actually are.

[Pavel Feldman]

(In reply to comment #5)
> (In reply to comment #4)
> > I think we
> > should do the same to inspector settings. Generic approach would be to create
> > Preferences object (the one from inspector.js or its override) in
> > InspectorController and push it into the frontend from within
> > InspectorController::scriptObjectReady.
> 
> I didn't realize we already had a Preferences object; sounds like a good place
> to put the pre-load of the settings from InspectorController.  Do we create a
> method per setting, or two - a getter and setter - or a generic getter and
> setter?
> 
> Even though it gets busy, I like having method-per-setting, because then it's
> easy to figure out what all the possible settings actually are.

I think we should have named settings with shared getter and single setter in order not to bloat cache-related code. We could have constants for names of the known settings to address your argument.

[Brian Weinstein]

(In reply to comment #4)
> Given that we are 100% sure platform and port are needed for rendering, it does
> not really make sense to load them lazily. I'd rather push them into the
> frontend as soon as script object for it is available. Furthermore, I think we
> should do the same to inspector settings. Generic approach would be to create
> Preferences object (the one from inspector.js or its override) in
> InspectorController and push it into the frontend from within
> InspectorController::scriptObjectReady.

I'm going to land this optimization in the patch, because it will clean up code a good amount, but if we want to figure out future optimizations (like not loading this lazily), we can do that in the future.

[Brian Weinstein]

Landed in r51242.