Bug 162799 – [Modern Media Controls] layout nodes

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Antoine Quint]

For the work on the modern media controls we will be using a tree of LayoutNode objects that will commit to the DOM in coordinated `requestAnimationFrame()` calls to ensure all layouts are done in an efficient and coordinated manner. As a preamble, we introduced the `scheduler` singleton in https://bugs.webkit.org/show_bug.cgi?id=162726 which is in charge of scheduling callbacks. This task covers introducing the base LayoutNode class.

[Radar WebKit Bug Importer]

<rdar://problem/28569301>

[Antoine Quint]

Created attachment 290360 [details]
Patch

[Dean Jackson]

Comment on attachment 290360 [details]
Patch

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

I feel like all the property tests could be in a single file, but then you'd have more complicated logic in frameDidFire

> Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:41
> +        this.markDirtyProperty("x");

Seeing as all the properties have accessors, why is markDirtyProperty "public"?

> Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:116
> +        const previousChildren = this._children;
> +        while (previousChildren.length)
> +            this.removeChild(previousChildren[0]);

Do you really need the local variable here? Feels like just using this._children would be fine.

> Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:119
> +        for (let child of children)
> +            this.addChild(child);

children.forEach(child => this.addChild(child))

> Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:145
> +        return this.addChild(newSibling, index !== - 1 ? index + 1 : 0);

"index !== - 1 ? index + 1 : 0" can be just "index + 1"
(unless index is undefined, but that shouldn't happen)

> Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:211
> +        for (let propertyName of this._dirtyProperties)
> +            this.commitProperty(propertyName);

this._dirtyProperties.forEach(propertyName => this.commitProperty(propertyName));

> Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:238
> +        for (let i = this.children.length - 1; i >= 0; --i) {

Since you have insertAfter, why do you do this backwards?

> Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:261
> +    for (let dirtyNode of dirtyNodes)
> +        dirtyNode.layout();

dirtyNodes.forEach(n => n.layout());

> Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:265
> +    for (let nodeRequiringChildrenUpdate of nodesRequiringChildrenUpdate)
> +        nodeRequiringChildrenUpdate._updateChildren();

Same here.

> LayoutTests/media/modern-media-controls/layout-node/addChild.html:21
> +debug("\nnode.addChild(b, 0)");

I think it is nicer to do debug("") for the blank line, but whatever.

[Antoine Quint]

(In reply to comment #3)
> Comment on attachment 290360 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290360&action=review
> 
> I feel like all the property tests could be in a single file, but then you'd
> have more complicated logic in frameDidFire

I prefer smaller tests.

> > Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:41
> > +        this.markDirtyProperty("x");
> 
> Seeing as all the properties have accessors, why is markDirtyProperty
> "public"?

This method needs to be public since LayoutNode subclasses would need to call it to mark a new property as dirty. See the subclassing test and its OpacityNode class.

> > Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:116
> > +        const previousChildren = this._children;
> > +        while (previousChildren.length)
> > +            this.removeChild(previousChildren[0]);
> 
> Do you really need the local variable here? Feels like just using
> this._children would be fine.

Right, I'll fix that when landing.

> > Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:119
> > +        for (let child of children)
> > +            this.addChild(child);
> 
> children.forEach(child => this.addChild(child))

I thought for…of was faster, but it doesn't appear to be the case right now. I'll switch to forEach().

> > Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:145
> > +        return this.addChild(newSibling, index !== - 1 ? index + 1 : 0);
> 
> "index !== - 1 ? index + 1 : 0" can be just "index + 1"
> (unless index is undefined, but that shouldn't happen)

Good point. Not sure why I special-cased this.

> > Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:238
> > +        for (let i = this.children.length - 1; i >= 0; --i) {
> 
> Since you have insertAfter, why do you do this backwards?

This is the DOM API, there is no insertAfter.

> > LayoutTests/media/modern-media-controls/layout-node/addChild.html:21
> > +debug("\nnode.addChild(b, 0)");
> 
> I think it is nicer to do debug("") for the blank line, but whatever.

Cool, I'll do that.

[Antoine Quint]

(In reply to comment #4)
> (In reply to comment #3)
> > > Source/WebCore/Modules/modern-media-controls/controls/layout-node.js:119
> > > +        for (let child of children)
> > > +            this.addChild(child);
> > 
> > children.forEach(child => this.addChild(child))
> 
> I thought for…of was faster, but it doesn't appear to be the case right now.
> I'll switch to forEach().

Actually, it gets less elegant when you need to pass `this` explicitly:

children.forEach(child => this.addChild(child), this)

[Antoine Quint]

Created attachment 290394 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 290394 [details]
Patch for landing

Clearing flags on attachment: 290394

Committed r206686: <http://trac.webkit.org/changeset/206686>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.