WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
173890
Web Inspector: Modernize InjectedScriptSource
https://bugs.webkit.org/show_bug.cgi?id=173890
Summary
Web Inspector: Modernize InjectedScriptSource
Joseph Pecoraro
Reported
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.
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Joseph Pecoraro
Comment 1
2017-06-27 20:42:00 PDT
Created
attachment 313979
[details]
[PATCH] Part 1 - Mostly mechanical (var -> let)
Joseph Pecoraro
Comment 2
2017-06-27 20:42:31 PDT
Created
attachment 313980
[details]
[PATCH] Part 2 - Mostly mechanical (classes)
Joseph Pecoraro
Comment 3
2017-06-27 20:43:05 PDT
Created
attachment 313981
[details]
[PATCH] Part 3 - Move code around, eliminate some functions (isDefined, isPrimitiveValue)
Joseph Pecoraro
Comment 4
2017-06-27 20:43:40 PDT
Created
attachment 313982
[details]
[PATCH] Part 4 - Rename RemoteObject, move more code around, final tweaks
Joseph Pecoraro
Comment 5
2017-06-27 20:49:24 PDT
Created
attachment 313983
[details]
[PATCH] Combined - Proposed Fix
Joseph Pecoraro
Comment 6
2017-06-27 21:24:46 PDT
Created
attachment 313985
[details]
[PATCH] Combined - Proposed Fix
Blaze Burg
Comment 7
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.
Blaze Burg
Comment 8
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.
Joseph Pecoraro
Comment 9
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.
Devin Rousso
Comment 10
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?
Joseph Pecoraro
Comment 11
2017-07-17 21:28:04 PDT
Bump
Blaze Burg
Comment 12
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! :)
Blaze Burg
Comment 13
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
Blaze Burg
Comment 14
2017-07-18 15:46:45 PDT
Comment on
attachment 313985
[details]
[PATCH] Combined - Proposed Fix r=me, need to rebase it probably though.
Joseph Pecoraro
Comment 15
2017-07-18 16:20:16 PDT
<
https://trac.webkit.org/r219639
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug