Bug 161274 – Web Inspector: Add View layout tests, make views more testable

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

[Matt Baker]

View should be made more testable by dispatching events for view tree traversal stages:

WebInspector.Notification.ViewTreeWillLayout
WebInspector.Notification.ViewTreeDidLayout

Since View underpins so much of the Inspector UI, any changes carry potentially high-risk so we need some tests!

Basic tests:
- A default constructed view should be backed by a <div>.
- View.rootView() should return a view backed by document.body.
- After construction, layoutPending should be false.
- After construction, isDescendentOf(View.rootView()) should be false.

Hierarchy:
- After adding(removing) a child to(from) a parent, child.isDescendentOf(parent) should be true(false).
    - Adding a child to a parent causes child.didMoveToParent to be called with parentView === parent.
    - Removing a child from a parent causes child.didMoveToParent to be called with parentView === null.
    - Removing a child from a parent causes child.didMoveToWindow to be called with isAttachedToRoot === false.
- When a view first becomes a descendent of the root, didMoveToWindow is called with isAttachedToRoot === true.
    - Should happen when view is added directly to root, when view's detached parent is added to root, and when view is added to parent already attached to root.

Layouts:
- initialLayout should happen  exactly once, the first time the view does a layout but before its layout method is called.
- Calling view.needsLayout should schedule an async layout.
- Calling view.cancelLayout should cancel an async layout.
- Calling view.updateLayout should call layout immediately.
- Calling view.updateLayout when an async layout has been scheduled for the view should cancel the async layout.
- Calling view.updateLayoutIfNeeded should do nothing is an async layout hasn't been schedulde.

[Radar WebKit Bug Importer]

<rdar://problem/28038615>

[Matt Baker]

Created attachment 322303 [details]
Patch

[Devin Rousso]

Comment on attachment 322303 [details]
Patch

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

It's awesome we can now test some of our UI management =D

As of now, the only caveat for a r+ is the failing assertion in inspector/view/basics.html.  Once that is all PASS, I'll review again.

> Source/WebInspectorUI/UserInterface/Test/FrontendTestHarness.js:149
> +        this._originalRequestAnimationFrame = window.requestAnimationFrame;

Since you override `cancelAnimationFrame` as well, is it also worth saving to a variable?

> Source/WebInspectorUI/UserInterface/Views/View.js:48
> +            let rootView = new WI.View(document.body);

You could just assign this directly:

    WI.View._rootView = new WI.View(document.body);
    WI.View._rootView._isAttachedToRoot = true;

> LayoutTests/inspector/view/asynchronous-layout.html:92
> +            let view = new WI.TestView;
> +            WI.View.rootView().addSubview(view);
> +            InspectorTest.log("Cancel automatic layout.");
> +            view.cancelLayout();
> +            InspectorTest.expectFalse(view.layoutPending, "View should not have a pending layout.");
> +            view.needsLayout();
> +            InspectorTest.log("Cancel scheduled layout.");
> +            view.cancelLayout();
> +            InspectorTest.expectFalse(view.layoutPending, "View should not have a pending layout.");

Style: I'd personally space these out more, so it's clearer what each "phase" of the test involves.

    let view = new WI.TestView;
    WI.View.rootView().addSubview(view);

    InspectorTest.log("Cancel automatic layout.");
    view.cancelLayout();
    InspectorTest.expectFalse(view.layoutPending, "View should not have a pending layout.");

    view.needsLayout();
    InspectorTest.log("Cancel scheduled layout.");

    view.cancelLayout();
    InspectorTest.expectFalse(view.layoutPending, "View should not have a pending layout.");

> LayoutTests/inspector/view/basics-expected.txt:28
> +FAIL: Grandchild should be a descendant of it's grandparent.
> +    Expected: truthy
> +    Actual: false

This should PASS.

> LayoutTests/inspector/view/basics.html:18
> +            InspectorTest.expectEqual(WI.View.rootView(), WI.View.rootView(), "View.rootView() should always return the same view.");

You could just compare one of these with `rootView`, since it already is a different invocation of `WI.View.rootView()`.

> LayoutTests/inspector/view/resources/test-view.js:6
> +            super();

Style: add newline.

> LayoutTests/inspector/view/synchronous-layout.html:14
> +        name: "View.updateLayout",

I think our style has been to prefix test names with the name of the suite.

    "View.SynchronousLayout.updateLayout"

> LayoutTests/inspector/view/synchronous-layout.html:36
> +        name: "View.updateLayout.propogateToSubviews",

Ditto (14).

> LayoutTests/inspector/view/synchronous-layout.html:54
> +        name: "View.updateLayoutIfNeeded",

Ditto (14).

[Matt Baker]

Comment on attachment 322303 [details]
Patch

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

>> LayoutTests/inspector/view/basics-expected.txt:28
>> +    Actual: false
> 
> This should PASS.

Ugh, caused by a typo: child.parent -> child.parentView

>> LayoutTests/inspector/view/basics.html:18
>> +            InspectorTest.expectEqual(WI.View.rootView(), WI.View.rootView(), "View.rootView() should always return the same view.");
> 
> You could just compare one of these with `rootView`, since it already is a different invocation of `WI.View.rootView()`.

That's actually how I'd written it initially, but I kind of like how this line stands all on its own.

>> LayoutTests/inspector/view/synchronous-layout.html:14
>> +        name: "View.updateLayout",
> 
> I think our style has been to prefix test names with the name of the suite.
> 
>     "View.SynchronousLayout.updateLayout"

We should create a style guide for Inspector layout tests at some point, and come up with some conventions for names, use of InspectorTest expect and assert, etc.

[Matt Baker]

Created attachment 322328 [details]
Patch

[Devin Rousso]

Comment on attachment 322328 [details]
Patch

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

r=me

> Source/WebInspectorUI/ChangeLog:20
> +        Fixed issues caught while writing tests for the expected View behavior.

Style: move this to the end of the function list.

[Matt Baker]

Created attachment 322480 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 322480 [details]
Patch

Clearing flags on attachment: 322480

Committed r222782: <http://trac.webkit.org/changeset/222782>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.