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-
[PATCH] Part 2 - Mostly mechanical (classes) (19.48 KB, patch)
2017-06-27 20:42 PDT, Joseph Pecoraro
bburg: review+
[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+
[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+
[PATCH] Combined - Proposed Fix (73.13 KB, patch)
2017-06-27 20:49 PDT, Joseph Pecoraro
no flags
[PATCH] Combined - Proposed Fix (73.25 KB, patch)
2017-06-27 21:24 PDT, Joseph Pecoraro
bburg: review+
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
Note You need to log in before you can comment on or make changes to this bug.