Avoid subclassing WebInspector.Object if we do not need to
Created attachment 316063 [details] [PATCH] Proposed Fix
Found files with: $ ack --files-without-matches 'Event = \{' | xargs ack 'extends WebInspector\.Object\b' Then had to add back just a couple (TestHarness and HeapSnapshotWorkerProxy). I didn't modify any Views / Controllers.
Comment on attachment 316063 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=316063&action=review Is there any particular motivation for this? Code cleanup, correctness, performance? > Source/WebInspectorUI/UserInterface/Models/ApplicationCacheFrame.js:26 > +WebInspector.ApplicationCacheFrame = class ApplicationCacheFrame I guess we don't type check anywhere that the target of saveIdentityToCookie is a subclass of object? > Source/WebInspectorUI/UserInterface/Models/DOMSearchMatchObject.js:27 > { Sidenote: this class needs to be cleaned up. It knows a lot of style classes for being in Models/ directory. > Source/WebInspectorUI/UserInterface/Models/Probe.js:27 > +WebInspector.ProbeSample = class ProbeSample r-, this class dispatches events.
Comment on attachment 316063 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=316063&action=review >> Source/WebInspectorUI/UserInterface/Models/ApplicationCacheFrame.js:26 >> +WebInspector.ApplicationCacheFrame = class ApplicationCacheFrame > > I guess we don't type check anywhere that the target of saveIdentityToCookie is a subclass of object? Correct, all cases of code using `instanceof WebInspector.Object\b` were in Event related code: > Base/EventListenerSet.js > 49: var emitterIsValid = emitter && (emitter instanceof WebInspector.Object || emitter instanceof Node || (typeof emitter.addEventListener === "function")); > > Base/EventListener.js > 44: var emitterIsValid = emitter && (emitter instanceof WebInspector.Object || emitter instanceof Node || (typeof emitter.addEventListener === "function")); >> Source/WebInspectorUI/UserInterface/Models/Probe.js:27 >> +WebInspector.ProbeSample = class ProbeSample > > r-, this class dispatches events. Where? Probe does, but I don't see where ProbeSample does. Can you point it out to me?
> Is there any particular motivation for this? Code cleanup, correctness, performance? Just cleanup and clarity. Not having a `this._listeners = null` property/value on all of these objects might be a tiny memory improvement but probably not. It will make printing these objects slightly nicer in our console not having that property around.
Comment on attachment 316063 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=316063&action=review >>> Source/WebInspectorUI/UserInterface/Models/Probe.js:27 >>> +WebInspector.ProbeSample = class ProbeSample >> >> r-, this class dispatches events. > > Where? Probe does, but I don't see where ProbeSample does. Can you point it out to me? Oops, didn't see the second class. I recant!
Comment on attachment 316063 [details] [PATCH] Proposed Fix Clearing flags on attachment: 316063 Committed r219734: <http://trac.webkit.org/changeset/219734>
All reviewed patches have been landed. Closing bug.