Bug 174703 – Web Inspector: Avoid subclassing WebInspector.Object if we do not need to

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

Avoid subclassing WebInspector.Object if we do not need to

[Joseph Pecoraro]

Created attachment 316063 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

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.

[Blaze Burg]

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.

[Joseph Pecoraro]

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?

[Joseph Pecoraro]

> 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.

[Blaze Burg]

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!

[WebKit Commit Bot]

Comment on attachment 316063 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 316063

Committed r219734: <http://trac.webkit.org/changeset/219734>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.