Bug 162868 – [Modern Media Controls] LayoutItem and Button classes

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

[Antoine Quint]

Now that we have the basic building blocks to render content asynchronously with LayoutNode, we need to add a class to create buttons, which is the bread and butter of the UI part of media controls.

[Radar WebKit Bug Importer]

<rdar://problem/28590166>

[Antoine Quint]

Created attachment 290490 [details]
Patch

[Dean Jackson]

Comment on attachment 290490 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        We introduce the new Button class and its parent class LayoutItem. A Button is a class
> +        that we will subclass a lot in coming patches, it provides a way to create a button

We will subclass Button a lot in coming patches, as it...

> Source/WebCore/Modules/modern-media-controls/controls/button.js:32
> +            element: `<button>`,

Why not use just a "" string here?

> Source/WebCore/Modules/modern-media-controls/controls/button.js:36
> +        this._enabled = true;

Would you ever want to call the delegate at this point?

> Source/WebCore/Modules/modern-media-controls/controls/layout-item.js:36
> +    constructor({ element = null, layoutDelegate = null } = {})

Nice. I'd forgotten about this syntax.

> LayoutTests/media/modern-media-controls/button/button.html:30
> +button.uiDelegate = {
> +
> +    buttonWasClicked(aButton)
> +    {
> +        clickedButton = aButton;
> +        shouldBeTrue("clickedButton === button");
> +    }
> +
> +}

This syntax is nuts. I really wanted to see buttonWasClicked: function (aButton)...

> LayoutTests/media/modern-media-controls/layout-item/layout-item.html:12
> +    
> +    get layoutTraits()

Nit: blank line isn't really needed

[Antoine Quint]

(In reply to comment #3)
> Comment on attachment 290490 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290490&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        We introduce the new Button class and its parent class LayoutItem. A Button is a class
> > +        that we will subclass a lot in coming patches, it provides a way to create a button
> 
> We will subclass Button a lot in coming patches, as it...
> 
> > Source/WebCore/Modules/modern-media-controls/controls/button.js:32
> > +            element: `<button>`,
> 
> Why not use just a "" string here?

Sure.

> > Source/WebCore/Modules/modern-media-controls/controls/button.js:36
> > +        this._enabled = true;
> 
> Would you ever want to call the delegate at this point?

The need for that hasn't arisen yet.

> > Source/WebCore/Modules/modern-media-controls/controls/layout-item.js:36
> > +    constructor({ element = null, layoutDelegate = null } = {})
> 
> Nice. I'd forgotten about this syntax.

Yes, default parameter values and object deconstruction is a fine pair.

> > LayoutTests/media/modern-media-controls/button/button.html:30
> > +button.uiDelegate = {
> > +
> > +    buttonWasClicked(aButton)
> > +    {
> > +        clickedButton = aButton;
> > +        shouldBeTrue("clickedButton === button");
> > +    }
> > +
> > +}
> 
> This syntax is nuts. I really wanted to see buttonWasClicked: function
> (aButton)...

Sure, we can write it this way.

> > LayoutTests/media/modern-media-controls/layout-item/layout-item.html:12
> > +    
> > +    get layoutTraits()
> 
> Nit: blank line isn't really needed

Will remove.

[Antoine Quint]

Created attachment 290492 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 290492 [details]
Patch for landing

Clearing flags on attachment: 290492

Committed r206745: <http://trac.webkit.org/changeset/206745>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.