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.
Created attachment 313979 [details] [PATCH] Part 1 - Mostly mechanical (var -> let)
Created attachment 313980 [details] [PATCH] Part 2 - Mostly mechanical (classes)
Created attachment 313981 [details] [PATCH] Part 3 - Move code around, eliminate some functions (isDefined, isPrimitiveValue)
Created attachment 313982 [details] [PATCH] Part 4 - Rename RemoteObject, move more code around, final tweaks
Created attachment 313983 [details] [PATCH] Combined - Proposed Fix
Created attachment 313985 [details] [PATCH] Combined - Proposed Fix
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 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 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 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?
Bump
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 on attachment 313982 [details] [PATCH] Part 4 - Rename RemoteObject, move more code around, final tweaks rs=me
Comment on attachment 313985 [details] [PATCH] Combined - Proposed Fix r=me, need to rebase it probably though.
<https://trac.webkit.org/r219639>