Bug 173890 - Web Inspector: Modernize InjectedScriptSource
Summary: Web Inspector: Modernize InjectedScriptSource
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks: 174006
  Show dependency treegraph
 
Reported: 2017-06-27 14:32 PDT by Joseph Pecoraro
Modified: 2017-07-18 16:20 PDT (History)
9 users (show)

See Also:


Attachments
[PATCH] Part 1 - Mostly mechanical (var -> let) (44.97 KB, patch)
2017-06-27 20:42 PDT, Joseph Pecoraro
bburg: review+
bburg: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Part 2 - Mostly mechanical (classes) (19.48 KB, patch)
2017-06-27 20:42 PDT, Joseph Pecoraro
bburg: review+
Details | Formatted Diff | Diff
[PATCH] Part 3 - Move code around, eliminate some functions (isDefined, isPrimitiveValue) (31.89 KB, patch)
2017-06-27 20:43 PDT, Joseph Pecoraro
bburg: review+
Details | Formatted Diff | Diff
[PATCH] Part 4 - Rename RemoteObject, move more code around, final tweaks (22.82 KB, patch)
2017-06-27 20:43 PDT, Joseph Pecoraro
bburg: review+
Details | Formatted Diff | Diff
[PATCH] Combined - Proposed Fix (73.13 KB, patch)
2017-06-27 20:49 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Combined - Proposed Fix (73.25 KB, patch)
2017-06-27 21:24 PDT, Joseph Pecoraro
bburg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-06-27 14:32:12 PDT
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.
Comment 1 Joseph Pecoraro 2017-06-27 20:42:00 PDT
Created attachment 313979 [details]
[PATCH] Part 1 - Mostly mechanical (var -> let)
Comment 2 Joseph Pecoraro 2017-06-27 20:42:31 PDT
Created attachment 313980 [details]
[PATCH] Part 2 - Mostly mechanical (classes)
Comment 3 Joseph Pecoraro 2017-06-27 20:43:05 PDT
Created attachment 313981 [details]
[PATCH] Part 3 - Move code around, eliminate some functions (isDefined, isPrimitiveValue)
Comment 4 Joseph Pecoraro 2017-06-27 20:43:40 PDT
Created attachment 313982 [details]
[PATCH] Part 4 - Rename RemoteObject, move more code around, final tweaks
Comment 5 Joseph Pecoraro 2017-06-27 20:49:24 PDT
Created attachment 313983 [details]
[PATCH] Combined - Proposed Fix
Comment 6 Joseph Pecoraro 2017-06-27 21:24:46 PDT
Created attachment 313985 [details]
[PATCH] Combined - Proposed Fix
Comment 7 BJ Burg 2017-06-30 10:12:23 PDT
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.
Comment 8 BJ Burg 2017-06-30 15:13:29 PDT
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.
Comment 9 Joseph Pecoraro 2017-06-30 16:54:02 PDT
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.
Comment 10 Devin Rousso 2017-06-30 19:00:53 PDT
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?
Comment 11 Joseph Pecoraro 2017-07-17 21:28:04 PDT
Bump
Comment 12 BJ Burg 2017-07-18 15:42:04 PDT
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! :)
Comment 13 BJ Burg 2017-07-18 15:46:22 PDT
Comment on attachment 313982 [details]
[PATCH] Part 4 - Rename RemoteObject, move more code around, final tweaks

rs=me
Comment 14 BJ Burg 2017-07-18 15:46:45 PDT
Comment on attachment 313985 [details]
[PATCH] Combined - Proposed Fix

r=me, need to rebase it probably though.
Comment 15 Joseph Pecoraro 2017-07-18 16:20:16 PDT
<https://trac.webkit.org/r219639>