Bug 172393 – Web Inspector: Don't create DetailsSidebarPanel classes until they are needed by a Tab

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

[Devin Rousso]

Currently, all of these classes are constructed during load on the WebInspector object.  Instead, they should only be created once a tab that requires it is shown.

[Devin Rousso]

Created attachment 310716 [details]
Patch

[Build Bot]

Comment on attachment 310716 [details]
Patch

Attachment 310716 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3779638

New failing tests:
inspector/formatting/formatting-json.html

[Build Bot]

Created attachment 310725 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 310716 [details]
Patch

Attachment 310716 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3779747

New failing tests:
inspector/formatting/formatting-json.html

[Build Bot]

Created attachment 310730 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 310716 [details]
Patch

Attachment 310716 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3779805

New failing tests:
inspector/formatting/formatting-json.html

[Build Bot]

Created attachment 310731 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Devin Rousso]

Created attachment 310733 [details]
Patch

[Blaze Burg]

Comment on attachment 310733 [details]
Patch

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

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

This change seems unrelated to the goal stated in the change log. Is it related?

[Devin Rousso]

Comment on attachment 310733 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:103
>> +Object.defineProperty(Object, "singleton",
> 
> This change seems unrelated to the goal stated in the change log. Is it related?

This is used to create singleton instances of each DetailsSidebarPanel.  I figured that it would also be nice to replace the few existing `static instance()` methods with this for consistency.

[Joseph Pecoraro]

Comment on attachment 310733 [details]
Patch

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

The change seems fine, but I think it needs an iteration. I'm not sold on Object.singleton(...) so I want to push back on that.

>>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:103
>>> +Object.defineProperty(Object, "singleton",
>> 
>> This change seems unrelated to the goal stated in the change log. Is it related?
> 
> This is used to create singleton instances of each DetailsSidebarPanel.  I figured that it would also be nice to replace the few existing `static instance()` methods with this for consistency.

I prefer the readability of the existing static singleton static methods. Can you provide an argument for changing them?

> Source/WebInspectorUI/UserInterface/Views/ContentBrowserTabContentView.js:28
> +    constructor(identifier, styleClassNames, tabBarItem, navigationSidebarPanelClass, detailsSidebarPanelClasses, disableBackForward)

I think we normally call these Constructors not Classes. Classes made me think of classNames.

> Source/WebInspectorUI/UserInterface/Views/TabContentView.js:34
> +        // console.assert(!detailsSidebarPanelClasses || detailsSidebarPanelClasses.every((detailsSidebarPanel) => detailsSidebarPanel instanceof WebInspector.DetailsSidebarPanel));

I think you should be able to assert something here. Not instanceof, but probably typeof x === "function".

> Source/WebInspectorUI/UserInterface/Views/TabContentView.js:181
> +    get detailsSidebarPanels()
> +    {

The user of this getter should store the result into a variable instead of calling it many times in a loop.

[Devin Rousso]

Comment on attachment 310733 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:103
>>>> +Object.defineProperty(Object, "singleton",
>>> 
>>> This change seems unrelated to the goal stated in the change log. Is it related?
>> 
>> This is used to create singleton instances of each DetailsSidebarPanel.  I figured that it would also be nice to replace the few existing `static instance()` methods with this for consistency.
> 
> I prefer the readability of the existing static singleton static methods. Can you provide an argument for changing them?

I think that having a unified singleton approach is better mainly for consistency.  In addition, since we can't inherit static functions, it would require us to create a `static singleton()` for each DetailsSidebarPanel class, or do something potentially weird.

[Devin Rousso]

Created attachment 311045 [details]
Patch

[Build Bot]

Comment on attachment 311045 [details]
Patch

Attachment 311045 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3802467

New failing tests:
inspector/formatting/formatting-json.html

[Build Bot]

Created attachment 311054 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311045 [details]
Patch

Attachment 311045 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3802476

New failing tests:
inspector/formatting/formatting-json.html

[Build Bot]

Created attachment 311056 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Devin Rousso]

Created attachment 311057 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 311057 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:105
> +    value(constructor, ...arguments)

I don't think you should ever name a parameter `arguments` because it will shadow the real `arguments`. We normally use `args`.

[Joseph Pecoraro]

Comment on attachment 311057 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:105
>> +    value(constructor, ...arguments)
> 
> I don't think you should ever name a parameter `arguments` because it will shadow the real `arguments`. We normally use `args`.

Actually I don't think we would ever want arguments to this, it would mean potentially different construction in different places. So I'd drop this param.

[Devin Rousso]

Created attachment 311078 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 311078 [details]
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?".

[Devin Rousso]

Created attachment 311310 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 311310 [details]
Patch

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

This feels much better to me. It also has the added benefit where previously `WebInspector.resourceDetailsSidebarPanel` now becomes `WebInspector.__ResourceDetailsSidebarPanel` which seems like it might be useful for debugging to know what per-WebInspector instances have been created or not.

> Source/WebInspectorUI/UserInterface/Base/Main.js:519
> +// This function exists for classes that are "effectively" singletons in that they are only created
> +// once, such as SidebarPanel classes, but are not intended as such.  There is no reason why

Style: Lets not have air quotes in a common.
Style: Once space after a period.

> Source/WebInspectorUI/UserInterface/Base/Main.js:523
> +// multiple instances of these classes couldn't be created, other than that it is not necessary.
> +// As an example, for SidebarPanel classes, if a change to WebInspector was made such that two
> +// entire instances of the UI are rendered side by side, it should be possible for more than one
> +// instance of the sidebar to exist (one for each UI).

Lets tighten this comment up a bit. This function returns a lazily constructed instance of a class scoped to this WebInspector instance. In the unlikely event that we ever need to construct multiple WebInspector instances this allows us to scope objects within the WebInspector. Currently it is used only for sidebars.

> Source/WebInspectorUI/UserInterface/Base/Main.js:526
> +    let key = `__${constructor.name}`;

Lets assert: console.assert(typeof constructor === "function")

[Devin Rousso]

Created attachment 311317 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 311317 [details]
Patch

Clearing flags on attachment: 311317

Committed r217460: <http://trac.webkit.org/changeset/217460>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/32479823>