[webkit-reviews] review denied: [Bug 172393] Web Inspector: Don't create DetailsSidebarPanel classes until they are needed by a Tab : [Attachment 311078] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 172393: Web Inspector: Don't create DetailsSidebarPanel classes until they
are needed by a Tab
https://bugs.webkit.org/show_bug.cgi?id=172393

Attachment 311078: Patch

https://bugs.webkit.org/attachment.cgi?id=311078&action=review




--- Comment #22 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 311078
  --> https://bugs.webkit.org/attachment.cgi?id=311078
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311078&action=review

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:103
> +Object.defineProperty(Object, "singleton",

r-, I've now convinced myself that we shouldn't go down this road.

Although the sidebars are "effectively" singletons, we shouldn't treat them as
such. Before this patch instances are namespaced in WebInspector. Imagine if we
were to have 2 "WebInspector" instances some day we would want sidebar
instances bundled inside of WebInspector. So, I view these as lazily loaded
property namespaced in WebInspector. So something like:

    WebInspector.sidebarForClass = function(constructor)
    {
	let sidebar = this["__" + constructor.name];
	if (sidebar)
	    return sidebar;
	return this["__" + constructor.name] = new constructor();
    }

The current classes that have singleton static methods truly are singletons,
and should not be instantiated multiple time. The static methods serve as
documentation that "hey this is a singleton". Object.singleton muddles those
waters "what should and shouldn't be a singleton?".