Bug 173890 – Web Inspector: Modernize InjectedScriptSource

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

It is rather frustrating that InjectedScriptSource uses old style JavaScript.

  • It uses ES5 semantics when it could use ES2015+ for many things
    - var => let, template strings, and other syntactic + safer niceties

  • Object + Prototype could be using Class
    - classes provide a simpler syntax to do the same thing

  • Order of methods is mostly arbitrary
    - currently its a bit of a mess

I want the JavaScript InjectedScript class to mirror Inspector::InjectedScript's C++ interface.

[Joseph Pecoraro]

Created attachment 313979 [details]
[PATCH] Part 1 - Mostly mechanical (var -> let)

[Joseph Pecoraro]

Created attachment 313980 [details]
[PATCH] Part 2 - Mostly mechanical (classes)

[Joseph Pecoraro]

Created attachment 313981 [details]
[PATCH] Part 3 - Move code around, eliminate some functions (isDefined, isPrimitiveValue)

[Joseph Pecoraro]

Created attachment 313982 [details]
[PATCH] Part 4 - Rename RemoteObject, move more code around, final tweaks

[Joseph Pecoraro]

Created attachment 313983 [details]
[PATCH] Combined - Proposed Fix

[Joseph Pecoraro]

Created attachment 313985 [details]
[PATCH] Combined - Proposed Fix

[Blaze Burg]

Comment on attachment 313979 [details]
[PATCH] Part 1 - Mostly mechanical (var -> let)

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

r=me on part 1

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:824
> +        // FIXME: This is observable if the page overrides Set.prototype[Symbol.iterator].

Please file a bug about this, if there is not one already.

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:844
> +        // FIXME: This is observable if the page overrides Map.prototype[Symbol.iterator].

Ditto.

[Blaze Burg]

Comment on attachment 313980 [details]
[PATCH] Part 2 - Mostly mechanical (classes)

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

r=me on part 2

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:951
> +        this.className = InjectedScriptHost.internalConstructorName(object);

Not something to address in a class refactoring, but is there a reason that we need to compute these eagerly instead of doing it in getters? I guess if we immediately ship all of this to inspector it's OK to precompute it.

[Joseph Pecoraro]

Comment on attachment 313980 [details]
[PATCH] Part 2 - Mostly mechanical (classes)

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

>> Source/JavaScriptCore/inspector/InjectedScriptSource.js:951
>> +        this.className = InjectedScriptHost.internalConstructorName(object);
> 
> Not something to address in a class refactoring, but is there a reason that we need to compute these eagerly instead of doing it in getters? I guess if we immediately ship all of this to inspector it's OK to precompute it.

Correct `this` is going to be JSON serialized and sent to the frontend. That is why these properties are not _className, etc.

[Devin Rousso]

Comment on attachment 313980 [details]
[PATCH] Part 2 - Mostly mechanical (classes)

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

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:942
> +            return;

Do we ever return inside constructors?  I realize that this is fine, but I'm just not used to seeing it.  We might want to refactor this a bit for clarity if it is such that we don't have cases of returning in constructors.

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:965
> +        if (subtype === "array")
> +            this.size = typeof object.length === "number" ? object.length : 0;
> +        else if (subtype === "set" || subtype === "map")
> +            this.size = object.size;
> +        else if (subtype === "weakmap")
> +            this.size = InjectedScriptHost.weakMapSize(object);
> +        else if (subtype === "weakset")
> +            this.size = InjectedScriptHost.weakSetSize(object);
> +        else if (subtype === "class") {
> +            this.classPrototype = injectedScript._wrapObject(object.prototype, objectGroupName);
> +            this.className = object.name;
> +        }

NIT: This isn't needed, but you could rework this as a switch.

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:1308
> +InjectedScript.RemoteObject.createObjectPreviewForValue = function(value, generatePreview, columnNames)
> +{
> +    let remoteObject = new InjectedScript.RemoteObject(value, undefined, false, generatePreview, columnNames);
> +    if (remoteObject.objectId)
> +        injectedScript.releaseObject(remoteObject.objectId);
> +    if (remoteObject.classPrototype && remoteObject.classPrototype.objectId)
> +        injectedScript.releaseObject(remoteObject.classPrototype.objectId);
> +    return remoteObject.preview || remoteObject._emptyPreview();
> +};

Is there a reason to not make this static on InjectedScript.RemoteObject?

[Joseph Pecoraro]

Bump

[Blaze Burg]

Comment on attachment 313981 [details]
[PATCH] Part 3 - Move code around, eliminate some functions (isDefined, isPrimitiveValue)

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

rs=me

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:83
> +        return !InjectedScriptHost.isHTMLAllCollection(value);

So gross! :)

[Blaze Burg]

Comment on attachment 313982 [details]
[PATCH] Part 4 - Rename RemoteObject, move more code around, final tweaks

rs=me

[Blaze Burg]

Comment on attachment 313985 [details]
[PATCH] Combined - Proposed Fix

r=me, need to rebase it probably though.

[Joseph Pecoraro]

<https://trac.webkit.org/r219639>