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.
<rdar://problem/28631803>
Created attachment 290707 [details] Patch
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
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 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)
(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.
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.
Created attachment 290816 [details] Patch for landing
Comment on attachment 290816 [details] Patch for landing Clearing flags on attachment: 290816 Committed r206864: <http://trac.webkit.org/changeset/206864>
All reviewed patches have been landed. Closing bug.