Bug 163328

<rdar://problem/28733838>

Created attachment 291350 [details] Patch

Comment on attachment 291350 [details] Patch Attachment 291350 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2270310 New failing tests: media/modern-media-controls/volume-slider/volume-slider-value.html

Created attachment 291354 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 291350 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291350&action=review > Source/WebCore/Modules/modern-media-controls/controls/slider.js:35 > + this._input = new LayoutNode(`<input type="range" min="0" max="1" step="${1 / 1000}">`); Why not just 0.001? > Source/WebCore/Modules/modern-media-controls/controls/slider.js:37 > + this._input.element.addEventListener("input", this); I wonder if you're often doing someLayoutNode.element.addEventListenter(whatever, this) you should add a listenFor method to LayoutNode listenFor(type, target) { this.element.addEventListener(type, target); } > Source/WebCore/Modules/modern-media-controls/controls/slider.js:101 > + if (!this._valueIsChanging && this.uiDelegate && typeof this.uiDelegate.controlValueWillStartChanging === "function") isFunction() helper? > LayoutTests/media/modern-media-controls/resources/media-controls-utils.js:33 > + if (rgba.length == 4) { > + [r, g, b] = rgba; Wow. Didn't know that worked. > LayoutTests/media/modern-media-controls/resources/media-controls-utils.js:46 > + const expectedRGBA = rgba(expectedColor); > + shouldBe(`rgba(${expr}).r`, `${expectedRGBA.r}`); > + shouldBe(`rgba(${expr}).g`, `${expectedRGBA.g}`); > + shouldBe(`rgba(${expr}).b`, `${expectedRGBA.b}`); > + shouldBeCloseTo(`rgba(${expr}).a`, expectedRGBA.a, 0.001); Add a local variable. let rgbaValue = rgba(expr); Also, this function is for testing, and won't need to be included in the running production code. I suggest a separate media-control-test-utils.js file. > LayoutTests/media/modern-media-controls/volume-slider/volume-slider-value.html:42 > +function dragSlider(fromPx, toPx) { Isn't there a drag event sender? Or am I thinking of the iOS-only touch UIScripts?

(In reply to comment #5) > Comment on attachment 291350 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291350&action=review > > > Source/WebCore/Modules/modern-media-controls/controls/slider.js:35 > > + this._input = new LayoutNode(`<input type="range" min="0" max="1" step="${1 / 1000}">`); > > Why not just 0.001? No good reason. I'll change it. > > Source/WebCore/Modules/modern-media-controls/controls/slider.js:37 > > + this._input.element.addEventListener("input", this); > > I wonder if you're often doing > someLayoutNode.element.addEventListenter(whatever, this) you should add a > listenFor method to LayoutNode > > listenFor(type, target) { > this.element.addEventListener(type, target); > } I thought about that, but discarded it since it was not essential to the LayoutNode feature of grouping DOM updates. > > Source/WebCore/Modules/modern-media-controls/controls/slider.js:101 > > + if (!this._valueIsChanging && this.uiDelegate && typeof this.uiDelegate.controlValueWillStartChanging === "function") > > isFunction() helper? This is how we've written those things in the Web Inspector code. It's verbose but it's straightforward. > > LayoutTests/media/modern-media-controls/resources/media-controls-utils.js:33 > > + if (rgba.length == 4) { > > + [r, g, b] = rgba; > > Wow. Didn't know that worked. Yup, ES6 restructuring is the bomb. > > LayoutTests/media/modern-media-controls/resources/media-controls-utils.js:46 > > + const expectedRGBA = rgba(expectedColor); > > + shouldBe(`rgba(${expr}).r`, `${expectedRGBA.r}`); > > + shouldBe(`rgba(${expr}).g`, `${expectedRGBA.g}`); > > + shouldBe(`rgba(${expr}).b`, `${expectedRGBA.b}`); > > + shouldBeCloseTo(`rgba(${expr}).a`, expectedRGBA.a, 0.001); > > Add a local variable. > > let rgbaValue = rgba(expr); I don't think this can work, I think the `shouldBe()` family of functions only work when variables you refer to are in the global scope since eval() is used. > Also, this function is for testing, and won't need to be included in the > running production code. I suggest a separate media-control-test-utils.js > file. This file is in the LayoutTests directory. > > LayoutTests/media/modern-media-controls/volume-slider/volume-slider-value.html:42 > > +function dragSlider(fromPx, toPx) { > > Isn't there a drag event sender? > > Or am I thinking of the iOS-only touch UIScripts? Not that I'm aware. This was inspired by fast/form/range/range-slow-drag-to-edge.html.

I've filed https://bugs.webkit.org/show_bug.cgi?id=163357 to skip the failing test on iOS.

Created attachment 291395 [details] Patch for landing

Comment on attachment 291395 [details] Patch for landing Clearing flags on attachment: 291395 Committed r207245: <http://trac.webkit.org/changeset/207245>

All reviewed patches have been landed. Closing bug.

(In reply to comment #6) > > > LayoutTests/media/modern-media-controls/resources/media-controls-utils.js:46 > > > + const expectedRGBA = rgba(expectedColor); > > > + shouldBe(`rgba(${expr}).r`, `${expectedRGBA.r}`); > > > + shouldBe(`rgba(${expr}).g`, `${expectedRGBA.g}`); > > > + shouldBe(`rgba(${expr}).b`, `${expectedRGBA.b}`); > > > + shouldBeCloseTo(`rgba(${expr}).a`, expectedRGBA.a, 0.001); > > > > Add a local variable. > > > > let rgbaValue = rgba(expr); > > I don't think this can work, I think the `shouldBe()` family of functions > only work when variables you refer to are in the global scope since eval() > is used. Except I think you show how to do it in the second parameter :) shouldBe(`${rgbaValue.r}`, `${expectedRGBA.r}`) Now you're passing a string that has a single value, but the eval should work.

(In reply to comment #11) > (In reply to comment #6) > > > > > LayoutTests/media/modern-media-controls/resources/media-controls-utils.js:46 > > > > + const expectedRGBA = rgba(expectedColor); > > > > + shouldBe(`rgba(${expr}).r`, `${expectedRGBA.r}`); > > > > + shouldBe(`rgba(${expr}).g`, `${expectedRGBA.g}`); > > > > + shouldBe(`rgba(${expr}).b`, `${expectedRGBA.b}`); > > > > + shouldBeCloseTo(`rgba(${expr}).a`, expectedRGBA.a, 0.001); > > > > > > Add a local variable. > > > > > > let rgbaValue = rgba(expr); > > > > I don't think this can work, I think the `shouldBe()` family of functions > > only work when variables you refer to are in the global scope since eval() > > is used. > > Except I think you show how to do it in the second parameter :) > > shouldBe(`${rgbaValue.r}`, `${expectedRGBA.r}`) > > Now you're passing a string that has a single value, but the eval should > work. Right, except the point of those `shouldBe()` functions is that they show the eval'd string in the test results, and in your example I would see the actual r value, not the expression that would generate it.