Bug 70417 – Web Inspector: get rid of manual view hierarchy management.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

This change removes necessity to manage view hierarchy manually (add/removeChildView methods are going away).

[Pavel Feldman]

Created attachment 111622 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 111622 [details]
Patch

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

> Source/WebCore/inspector/front-end/NetworkPanel.js:-43
> -    // FIXME: some of the styles should be loaded on demand by components that need them.
> -    var styles = [
> -        "inspectorCommon.css",
> -        "dataGrid.css",
> -        "networkLogView.css"
> -    ];
> -    WebInspector.IFrameView.call(this, parentElement, styles);

This will probably regress performance. At least, this is worth reflecting in the ChangeLog.

> Source/WebCore/inspector/front-end/Panel.js:88
> +    detach: function()

how are extensions tests doing with this?

> Source/WebCore/inspector/front-end/View.js:139
> +        if (childIndex < 0) {
> +            console.error("Attempt to remove non-child view");

When are we supposed to hit the check?

> Source/WebCore/inspector/front-end/View.js:185
> +        while (parentElement && !parentElement.__view__)

Just __view seems more consistent with the rest of our code. __foo__ seems to be customary used for JS quirks.

> Source/WebCore/inspector/front-end/View.js:198
> +    printViewHierarchy: function()

Is this still necessary?

> Source/WebCore/inspector/front-end/View.js:205
> +    _collectViewHierarchy: function(prefix, lines)

ditto.

[Pavel Feldman]

Comment on attachment 111622 [details]
Patch

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

>> Source/WebCore/inspector/front-end/NetworkPanel.js:-43
>> -    WebInspector.IFrameView.call(this, parentElement, styles);
> 
> This will probably regress performance. At least, this is worth reflecting in the ChangeLog.

Done.

>> Source/WebCore/inspector/front-end/Panel.js:88
>> +    detach: function()
> 
> how are extensions tests doing with this?

Rolled it back for now.

>> Source/WebCore/inspector/front-end/View.js:139
>> +            console.error("Attempt to remove non-child view");
> 
> When are we supposed to hit the check?

We are not supposed to. I'll remove it after a number of builds.

>> Source/WebCore/inspector/front-end/View.js:185
>> +        while (parentElement && !parentElement.__view__)
> 
> Just __view seems more consistent with the rest of our code. __foo__ seems to be customary used for JS quirks.

done.

>> Source/WebCore/inspector/front-end/View.js:198
>> +    printViewHierarchy: function()
> 
> Is this still necessary?

Yes, I'd like to keep this debugging code at least for some time.

>> Source/WebCore/inspector/front-end/View.js:205
>> +    _collectViewHierarchy: function(prefix, lines)
> 
> ditto.

ditto

[Pavel Feldman]

Created attachment 111626 [details]
[Patch] Review comments addressed

[Pavel Feldman]

Created attachment 111627 [details]
[Patch] Review comments addressed

[Andrey Kosyakov]

Comment on attachment 111627 [details]
[Patch] Review comments addressed

LGTM

[Pavel Feldman]

Created attachment 111636 [details]
[Patch] with new iframe support

[Yury Semikhatsky]

Comment on attachment 111627 [details]
[Patch] Review comments addressed

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

> Source/WebCore/inspector/front-end/View.js:76
> +        if (parentElement && this.element.parentElement != parentElement)

!= -> !==

[Yury Semikhatsky]

Comment on attachment 111636 [details]
[Patch] with new iframe support

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

> Source/WebCore/inspector/front-end/ProfilesPanel.js:209
> +                if ("dispose" in view)

This can be implemented as detach override in corresponding view.

[Pavel Feldman]

Committed r97961: <http://trac.webkit.org/changeset/97961>