rdar://problem/30154093
Created attachment 305774 [details] Patch
Comment on attachment 305774 [details] Patch Attachment 305774 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3435623 New failing tests: media/modern-media-controls/scrubber/scrubber-has-correct-ax-label.html
Created attachment 305795 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 305798 [details] Patch
Comment on attachment 305798 [details] Patch Attachment 305798 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3436201 New failing tests: media/modern-media-controls/scrubber/scrubber-has-correct-ax-label.html
Created attachment 305805 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 305798 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305798&action=review > Source/WebCore/Modules/modern-media-controls/controls/scrubber.js:61 > + setAccessibleLabelForInput(label) We typically use setters, so I think this would be `set inputAccessibleLabel(label)`. > Source/WebCore/Modules/modern-media-controls/controls/scrubber.js:73 > + _formatTime(timeInSeconds) We already have a _formattedTime() method in TimeLabel. Ideally, we could refactor this code to be useful to both TimeLabel and Scrubber. A global utility in main.js would be fine. > Source/WebCore/Modules/modern-media-controls/controls/time-label.js:63 > + this._timeControl.updateScrubberLabel(); Code in `commitProperty` should specifically reflect the value of a given property in the DOM. I think you want to change this when `value` has changed, correct? If so, we should move this under the `if (propertyName === "value")` clause. > LayoutTests/media/modern-media-controls/scrubber/scrubber-has-correct-ax-label.html:32 > +media.addEventListener("canplay", () => { I'm not entirely sure, but I think you won't be getting this event on iOS by default because the "preload" attribute is to "metadata", so no actual playable data is downloaded until there is user action. This is my guess as to why this test is timing out on the iOS bot.
Created attachment 307350 [details] Patch
Comment on attachment 307350 [details] Patch Attachment 307350 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3555535 New failing tests: media/modern-media-controls/time-label/time-label.html
Created attachment 307354 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 307350 [details] Patch Attachment 307350 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3555568 New failing tests: media/modern-media-controls/time-label/time-label.html
Created attachment 307355 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 307350 [details] Patch Attachment 307350 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3555640 New failing tests: http/tests/inspector/network/resource-sizes-network.html
Created attachment 307356 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 307350 [details] Patch Attachment 307350 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3555639 New failing tests: media/modern-media-controls/time-label/time-label.html
Created attachment 307359 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 307395 [details] Patch
Comment on attachment 307395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307395&action=review > Source/WebCore/ChangeLog:55 > + (formatTimeByUnit): Make sure this double ChangeLog entry gets fixed. > Source/WebCore/Modules/modern-media-controls/controls/scrubber.js:85 > + _unitizeTime(value, unit) While it's OK as-is, this method really needn't be on the Scrubber prototype, it could just be a global function. > Source/WebCore/Modules/modern-media-controls/controls/time-control.js:93 > + if (this.scrubber) Is this ever not true? > Source/WebCore/Modules/modern-media-controls/controls/time-label.js:59 > + if (this._timeControl) Is this ever not true? > Source/WebCore/Modules/modern-media-controls/main.js:57 > + let result = {}; > + const time = value || 0; > + const absTime = Math.abs(time); > + result.intSeconds = Math.floor(absTime % 60).toFixed(0); > + result.intMinutes = Math.floor((absTime / 60) % 60).toFixed(0); > + result.intHours = Math.floor(absTime / (60 * 60)).toFixed(0); > + return result; Probably more idiomatic to write this as: return { seconds: …, minutes: …, hours: … }; > LayoutTests/ChangeLog:20 > + Double ChangeLog entry here as well.
Comment on attachment 307395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307395&action=review >> Source/WebCore/Modules/modern-media-controls/controls/time-control.js:93 >> + if (this.scrubber) > > Is this ever not true? This is extra. >> Source/WebCore/Modules/modern-media-controls/controls/time-label.js:59 >> + if (this._timeControl) > > Is this ever not true? This one is needed to pass the time-label test. The test only instantiates a time-label. It does not have a time control.
Created attachment 307526 [details] Patch
Comment on attachment 307526 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307526&action=review > Source/WebCore/Modules/modern-media-controls/controls/scrubber.js:64 > + this._inputAccessibleLabel = this._formatTime(timeValue); Doesn't look like we need this ivar at all. > Source/WebCore/Modules/modern-media-controls/controls/slider.js:81 > this._handleInputEvent(); We should change the name of `_handleInputEvent()` now that it's called for "change" events as well. We probably want it to be called something like `_valueDidChange()`. > Source/WebCore/Modules/modern-media-controls/controls/time-label.js:65 > + Remove those extra lines.
Created attachment 307607 [details] Patch
Comment on attachment 307607 [details] Patch Clearing flags on attachment: 307607 Committed r215572: <http://trac.webkit.org/changeset/215572>
All reviewed patches have been landed. Closing bug.