Bug 66131 – Web Inspector: maintain visible view hierarchy and dispatch common view events automatically

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

[Andrey Kosyakov]

- added a notion of an optional parent to a View
- maintain a list of visible View children
- dispatch common view events, such as resize, activation (i.e. becoming visible as a result of some of the parent views becoming visible) and deactivation automatically through the hierarchy of visible views.
- changed store/restore of the view scroll position to use element's scrollTop/scrollLeft rather than view properties
- removed scrollTop/scrollLeft getters/setters for views that have an unusual scroll container, override storeScrollPositions()/restoreScrollPositions() instead.

This takes hare of several bugs where we were not restoring view positions properly or not re-sizing views. Also, this removes some custom/override logic from view and panel implementation in favor of common logic in the base View.

[Andrey Kosyakov]

Created attachment 103756 [details]
patch

[Pavel Feldman]

Comment on attachment 103756 [details]
patch

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

> Source/WebCore/inspector/front-end/ConsoleView.js:38
> +    drawer.setParent(this);

ConsoleView is not a parent of drawer. Is full screen mode a drawer?

> Source/WebCore/inspector/front-end/Drawer.js:183
> +            WebInspector.currentPanel.dispatchToVisibleViews("resize");

I like the way it was.

> Source/WebCore/inspector/front-end/Drawer.js:363
> + }

extra space

> Source/WebCore/inspector/front-end/NetworkPanel.js:643
> +    viewActivated: function()

What is view activated?

> Source/WebCore/inspector/front-end/NetworkPanel.js:1388
> +            this.visibleView.setParent(null);

You should do addChildView instead.

> Source/WebCore/inspector/front-end/View.js:137
> +        this._visibleChildren.push(view);

I don't think you should distinguish between visible views and non-visible views here.

[Andrey Kosyakov]

Created attachment 103917 [details]
patch

- viewActivated/viewDeactivated -> becameOnscreen/becameOffscreen
- setParent -> addChild/removeChild
- generalized saving/restoring scroll positions between panel and view
- only support inheriting scroll positions for TextView & SourceFrame

[Pavel Feldman]

Comment on attachment 103917 [details]
patch

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

We need to make sure that the view with the attached element is in the view hierarchy.

> Source/WebCore/ChangeLog:3
> +        Web Inspector: maintain visible view hierarchy and dispatch common view events automatically

Does this change fix any bugs?

> Source/WebCore/inspector/front-end/Drawer.js:65
> +            this.removeChild(this._visibleView);

Why do we do this? We should either view.hide or detach / removeChild.

> Source/WebCore/inspector/front-end/NetworkPanel.js:1257
> +    this.addChild(this._networkLogView);

addChildView

> Source/WebCore/inspector/front-end/NetworkPanel.js:1390
>              this.visibleView.detach();

detach should remove child from hierarchy.

> Source/WebCore/inspector/front-end/NetworkPanel.js:1407
> +            this.removeChild(this.visibleView);

ditto

> Source/WebCore/inspector/front-end/Panel.js:390
> +        this.dispatchToVisibleChildren("resize");

This should be on view, you should distinguish doResize and onResize.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:519
> +        this.addChild(view);

Again, hide and addChild should not be invoked next to each other.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:534
> +        this.removeChild(this.visibleView);

detach / removeChild together.

> Source/WebCore/inspector/front-end/ScriptsPanel.js:672
> +        sourceFrame.detach();

ditto

[Andrey Kosyakov]

Created attachment 104056 [details]
patch

Panel.resize() -> View.doResize() & View.onResize()
View.detach() -> View._detach(), clients should removeChildView() instead.
ebcameOnscreen()/becameOffscreen() -> didShow()/willHide()

[Andrey Kosyakov]

(In reply to comment #4)
> (From update of attachment 103917 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103917&action=review
> 
> Does this change fix any bugs?

yep -- at least bug 66064, plus a number of bugs with saving/restoring scroll positions (which I didn't file separately).

Also, forgetting to delegate resize() or show()/hide() properly used to be our favorite source of bugs in the past.
> 
> > Source/WebCore/inspector/front-end/Drawer.js:65
> > +            this.removeChild(this._visibleView);
> 
> Why do we do this? We should either view.hide or detach / removeChild.

We now just call removeChildView() in the client code, detach() is implicit.

> > Source/WebCore/inspector/front-end/NetworkPanel.js:1257
> > +    this.addChild(this._networkLogView);
> 
> addChildView

done.

> 
> > Source/WebCore/inspector/front-end/NetworkPanel.js:1390
> >              this.visibleView.detach();
> 
> detach should remove child from hierarchy.

I made it vice versa.

> > Source/WebCore/inspector/front-end/NetworkPanel.js:1407
> > +            this.removeChild(this.visibleView);
> 
> ditto
> 
> > Source/WebCore/inspector/front-end/Panel.js:390
> > +        this.dispatchToVisibleChildren("resize");
> 
> This should be on view, you should distinguish doResize and onResize.

done.

> 
> > Source/WebCore/inspector/front-end/ResourcesPanel.js:519
> > +        this.addChild(view);
> 
> Again, hide and addChild should not be invoked next to each other.
> 
> > Source/WebCore/inspector/front-end/ResourcesPanel.js:534
> > +        this.removeChild(this.visibleView);
> 
> detach / removeChild together.
> 
> > Source/WebCore/inspector/front-end/ScriptsPanel.js:672
> > +        sourceFrame.detach();
> 
> ditto

[Pavel Feldman]

Comment on attachment 104056 [details]
patch

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

Looks good. Please provide a couple of tests!

> Source/WebCore/inspector/front-end/ResourcesPanel.js:533
> +        this.removeChildView(this.visibleView);

Either leave view as is or detach it at all times.

> Source/WebCore/inspector/front-end/SourceFrame.js:81
> +    show: function(parent)

This change does not look consistent. Please leave parentElement.

> Source/WebCore/inspector/front-end/View.js:54
> +        if (typeof this.onResize === "function")

Just call onResize.

> Source/WebCore/inspector/front-end/View.js:131
> +    addChildView: function(view, parentElement)

parentElement is unused.

> Source/WebCore/inspector/front-end/View.js:133
> +        if (view._parent === this)

_parentView

[Andrey Kosyakov]

Manually committed r93196: http://trac.webkit.org/changeset/93196