Summary: | [Modern Media Controls] Icon service and the IconButton class | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||||
Component: | Media | Assignee: | Antoine Quint <graouts> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, dino, graouts, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | Safari 10 | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Antoine Quint
2016-10-05 08:23:01 PDT
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. |