Summary: | [MotionMark] Update Multiply test | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jon Lee <jonlee> | ||||
Component: | Animations | Assignee: | Jon Lee <jonlee> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cdumez, commit-queue, dino, ews-watchlist, rniwa, sabouhallawa, simon.fraser, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Jon Lee
2018-08-13 14:27:36 PDT
Created attachment 347044 [details]
Patch
Comment on attachment 347044 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347044&action=review > PerformanceTests/MotionMark/tests/dom/resources/multiply.js:33 > + options.visibleCSS = [["opacity", 0, 1]]; I would suggest writing this as an structures: options.visibleCSS = [{ property: "visibility", initialValue: "hidden", animateValue: "visible" }]; > PerformanceTests/MotionMark/tests/master/resources/multiply.js:39 > + visibleCSS: [ > + ["visibility", "hidden", "visible"], > + ["opacity", 0, 1], > + ["display", "none", "block"] > + ], Ditto: visibleCSS: [ { property: "visibility", initialValue: "hidden", animateValue: "visible" }, { property: "opacity", initialValue: 0, animateValue: 1 }, { property: "display", initialValue: "none", animateValue: "block" } ], > PerformanceTests/MotionMark/tests/master/resources/multiply.js:45 > - var tileSize = Math.round(this.size.height / 25); > + var tileSize = Math.round(this.size.height / this.totalRows); Shouldn't we use the screen.height instead of the stage height to control how many tiles can be shown in the page? > PerformanceTests/MotionMark/tests/master/resources/multiply.js:96 > + tile.style[visibleCSS[0]] = visibleCSS[1]; tile.style[visibleCSS.property] = visibleCSS.initialValue; > PerformanceTests/MotionMark/tests/master/resources/multiply.js:130 > + tile.element.style[tile.visibleCSS[0]] = tile.visibleCSS[2]; tile.style[tile.visibleCSS.property] = tile.visibleCSS.animateValue; > PerformanceTests/MotionMark/tests/master/resources/multiply.js:141 > + tile.element.style[tile.visibleCSS[0]] = tile.visibleCSS[1]; tile.style[tile.visibleCSS.property] = tile.visibleCSS.initialValue; Comment on attachment 347044 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347044&action=review >> PerformanceTests/MotionMark/tests/master/resources/multiply.js:39 >> + ], > > Ditto: > > visibleCSS: [ > { property: "visibility", initialValue: "hidden", animateValue: "visible" }, > { property: "opacity", initialValue: 0, animateValue: 1 }, > { property: "display", initialValue: "none", animateValue: "block" } > ], I'll do this update in a later patch. >> PerformanceTests/MotionMark/tests/master/resources/multiply.js:45 >> + var tileSize = Math.round(this.size.height / this.totalRows); > > Shouldn't we use the screen.height instead of the stage height to control how many tiles can be shown in the page? I don't think so. It does mean literally equal work in terms of point area among all device classes, but if we did this then desktop spaces would have high 10,000s max # of particles, which might be detrimental to getting high-enough scores given the # of renderers that need to be iterated. Comment on attachment 347044 [details] Patch Clearing flags on attachment: 347044 Committed r234832: <https://trac.webkit.org/changeset/234832> All reviewed patches have been landed. Closing bug. |