Bug 162970 - [Modern Media Controls] Icon service and the IconButton class
Summary: [Modern Media Controls] Icon service and the IconButton class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari 10
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-10-05 08:23 PDT by Antoine Quint
Modified: 2016-10-06 06:33 PDT (History)
4 users (show)

See Also:


Attachments
Patch (33.49 KB, patch)
2016-10-05 08:34 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 (6.63 MB, application/zip)
2016-10-05 09:54 PDT, Build Bot
no flags Details
Patch for landing (41.28 KB, patch)
2016-10-06 05:55 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2016-10-05 08:23:01 PDT
The most frequent type of buttons that we will display in the modern media controls are buttons showing an image icon. We need a service to manage those icons and a Button subclass to show them.
Comment 1 Radar WebKit Bug Importer 2016-10-05 08:23:44 PDT
<rdar://problem/28631803>
Comment 2 Antoine Quint 2016-10-05 08:34:39 PDT
Created attachment 290707 [details]
Patch
Comment 3 Build Bot 2016-10-05 09:54:26 PDT
Comment on attachment 290707 [details]
Patch

Attachment 290707 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2224949

New failing tests:
media/modern-media-controls/icon-button/icon-button.html
Comment 4 Build Bot 2016-10-05 09:54:29 PDT
Created attachment 290717 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 5 Dean Jackson 2016-10-05 22:08:42 PDT
Comment on attachment 290707 [details]
Patch

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

> Source/WebCore/Modules/modern-media-controls/controls/icon-service.js:43
> +    set directoryPath(path)
> +    {
> +        this._directoryPath = path;
> +    }

What is the point of this if there is no getter and no custom code here? Just use .directoryPath directly externally.

> Source/WebCore/Modules/modern-media-controls/controls/icon-service.js:61
> +        if (layoutTraits & LayoutTraits.Mac)

I think layoutTraits should be a dictionary that takes strings. so layoutTraits["platform"] = "macOS".

Either way .Mac should be .macOS.

> Source/WebCore/Modules/modern-media-controls/controls/icon-service.js:68
> +        if (layoutTraits & LayoutTraits.Fullscreen && IconsWithFullScreenVariants.indexOf(iconName) !== -1)

I meant to mention this last time, but you can use !Array.includes(x) in place of the Array.indexOf(x) !== -1. Please change this in the other patches.

(Also, I don't think you would need the !== form here. != would be always fine)
Comment 6 Antoine Quint 2016-10-06 05:07:15 PDT
(In reply to comment #5)
> Comment on attachment 290707 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290707&action=review
> 
> > Source/WebCore/Modules/modern-media-controls/controls/icon-service.js:43
> > +    set directoryPath(path)
> > +    {
> > +        this._directoryPath = path;
> > +    }
> 
> What is the point of this if there is no getter and no custom code here?
> Just use .directoryPath directly externally.

Not sure what I was thinking :)

> > Source/WebCore/Modules/modern-media-controls/controls/icon-service.js:61
> > +        if (layoutTraits & LayoutTraits.Mac)
> 
> I think layoutTraits should be a dictionary that takes strings. so
> layoutTraits["platform"] = "macOS".
> 
> Either way .Mac should be .macOS.

I'll keep the bit-mask for now, but will rename to `macOS`.

> > Source/WebCore/Modules/modern-media-controls/controls/icon-service.js:68
> > +        if (layoutTraits & LayoutTraits.Fullscreen && IconsWithFullScreenVariants.indexOf(iconName) !== -1)
> 
> I meant to mention this last time, but you can use !Array.includes(x) in
> place of the Array.indexOf(x) !== -1. Please change this in the other
> patches.

Will change.

> (Also, I don't think you would need the !== form here. != would be always
> fine)

For numbers, that is true.
Comment 7 Antoine Quint 2016-10-06 05:48:58 PDT
The test failure on iOS is being tracked by https://bugs.webkit.org/show_bug.cgi?id=163009, I'm skipping the failing test on iOS for now.
Comment 8 Antoine Quint 2016-10-06 05:55:50 PDT
Created attachment 290816 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2016-10-06 06:33:21 PDT
Comment on attachment 290816 [details]
Patch for landing

Clearing flags on attachment: 290816

Committed r206864: <http://trac.webkit.org/changeset/206864>
Comment 10 WebKit Commit Bot 2016-10-06 06:33:25 PDT
All reviewed patches have been landed.  Closing bug.