Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2278723005:
Convert LayoutTests/fast/canvas/2d tests to testharness (Closed)
DescriptionConvert LayoutTests/fast/canvas/2d tests to testharness
This patch uses testharness.js asserts to test
2d.composite.globalAlpha.fillPath.html, 2d.fillText.gradient.html.
Remove the expectation files as they are not needed any more.
BUG=639732
Committed: https://crrev.com/ed0a7ec9b9a1b54d12cbc0e50b9f87599e62e629
Cr-Commit-Position: refs/heads/master@{#414508}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Changed test as per the comments. #
Total comments: 8
Patch Set 3 : Changed as per the comments #
Total comments: 8
Patch Set 4 : Changed test description. #Patch Set 5 : Test changed as per the comments. #
Messages
Total messages: 23 (9 generated)
Description was changed from ========== Convert LayoutTests/fast/canvas/2d tests to testharness This patch uses testharness.js asserts to test LayoutTests/fast/canvas/2d.composite.globalAlpha.fillPath.html LayoutTests/fast/canvas/2d.fillText.gradient.html. Remove the expectation files as they are not needed any more. BUG=639732 ========== to ========== Convert LayoutTests/fast/canvas/2d tests to testharness This patch uses testharness.js asserts to test 2d.composite.globalAlpha.fillPath.html, 2d.fillText.gradient.html. Remove the expectation files as they are not needed any more. BUG=639732 ==========
siva.gunturi@samsung.com changed reviewers: + fs@opera.com, junov@chromium.org, srirama.m@samsung.com
ptal..
https://codereview.chromium.org/2278723005/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/canvas/2d.composite.globalAlpha.fillPath.html (right): https://codereview.chromium.org/2278723005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/canvas/2d.composite.globalAlpha.fillPath.html:3: http://http://philip.html5.org/tests/canvas/suite/tests/index.2d.composite.gl... Nit: Maybe drop the initial http:// here (since it's duplicated.) https://codereview.chromium.org/2278723005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/canvas/2d.composite.globalAlpha.fillPath.html:14: assert_false(diff > tolerance); assert_greater_than https://codereview.chromium.org/2278723005/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html (right): https://codereview.chromium.org/2278723005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html:23: assert_false(imageData.data[0] != 0); Use assert_equals instead. (Maybe with an additional comment with the component checked: "red", "green" ...)
ptal.. https://codereview.chromium.org/2278723005/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/canvas/2d.composite.globalAlpha.fillPath.html (right): https://codereview.chromium.org/2278723005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/canvas/2d.composite.globalAlpha.fillPath.html:3: http://http://philip.html5.org/tests/canvas/suite/tests/index.2d.composite.gl... On 2016/08/25 10:50:37, fs wrote: > Nit: Maybe drop the initial http:// here (since it's duplicated.) Done. https://codereview.chromium.org/2278723005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/canvas/2d.composite.globalAlpha.fillPath.html:14: assert_false(diff > tolerance); On 2016/08/25 10:50:36, fs wrote: > assert_greater_than Done. https://codereview.chromium.org/2278723005/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html (right): https://codereview.chromium.org/2278723005/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html:23: assert_false(imageData.data[0] != 0); On 2016/08/25 10:50:37, fs wrote: > Use assert_equals instead. (Maybe with an additional comment with the component > checked: "red", "green" ...) Done.
https://codereview.chromium.org/2278723005/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/2d.composite.globalAlpha.fillPath.html (right): https://codereview.chromium.org/2278723005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/2d.composite.globalAlpha.fillPath.html:5: <title>The test to ensure correct sync behaviour with globalAlpha and fillPath() in accelerated-2d-canvas mode.</title> Nit: Drop "The test to ensure" (and capitalize "correct".) https://codereview.chromium.org/2278723005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/2d.composite.globalAlpha.fillPath.html:14: assert_greater_than(diff, tolerance); Actually, the assertion should be the other way around (less_than_equal), sorry I didn't notice this before. https://codereview.chromium.org/2278723005/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html (right): https://codereview.chromium.org/2278723005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html:2: <title>On success, the square should have the bottom left portion of the base of the green I and red otherwise.</title> Let's make this something akin to "fillText with gradient fillStyle", and make this a comment down in the script. https://codereview.chromium.org/2278723005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html:28: // Check the red pixel outside of text wan't touched. Nit: wasn't; the text
ptal.. https://codereview.chromium.org/2278723005/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/2d.composite.globalAlpha.fillPath.html (right): https://codereview.chromium.org/2278723005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/2d.composite.globalAlpha.fillPath.html:5: <title>The test to ensure correct sync behaviour with globalAlpha and fillPath() in accelerated-2d-canvas mode.</title> On 2016/08/25 11:16:48, fs wrote: > Nit: Drop "The test to ensure" (and capitalize "correct".) Done. https://codereview.chromium.org/2278723005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/2d.composite.globalAlpha.fillPath.html:14: assert_greater_than(diff, tolerance); On 2016/08/25 11:16:48, fs wrote: > Actually, the assertion should be the other way around (less_than_equal), sorry > I didn't notice this before. Done. https://codereview.chromium.org/2278723005/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html (right): https://codereview.chromium.org/2278723005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html:2: <title>On success, the square should have the bottom left portion of the base of the green I and red otherwise.</title> On 2016/08/25 11:16:48, fs wrote: > Let's make this something akin to "fillText with gradient fillStyle", and make > this a comment down in the script. Done. https://codereview.chromium.org/2278723005/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html:28: // Check the red pixel outside of text wan't touched. On 2016/08/25 11:16:48, fs wrote: > Nit: wasn't; the text Done.
LGTM w/ nit https://codereview.chromium.org/2278723005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html (right): https://codereview.chromium.org/2278723005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html:2: <title>Test the color gradient is applied properly for the rect in 2d canvas.</title> s/the rect/fillText()/
https://codereview.chromium.org/2278723005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html (right): https://codereview.chromium.org/2278723005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html:2: <title>Test the color gradient is applied properly for the rect in 2d canvas.</title> On 2016/08/25 13:57:36, fs wrote: > s/the rect/fillText()/ Done.
The CQ bit was checked by siva.gunturi@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com Link to the patchset: https://codereview.chromium.org/2278723005/#ps60001 (title: "Changed test description.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2278723005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/2d.composite.globalAlpha.fillPath.html (right): https://codereview.chromium.org/2278723005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/2d.composite.globalAlpha.fillPath.html:5: <title>Correct sync behaviour with globalAlpha and fillPath() in accelerated-2d-canvas mode.</title> It is not clear what is meant by "sync behavior". Also, there is no "fillPath()", it is just "fill()". https://codereview.chromium.org/2278723005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html (right): https://codereview.chromium.org/2278723005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html:2: <title>Test the color gradient is applied properly for the rect in 2d canvas.</title> the color gradient -> that color gradient the rect -> rect https://codereview.chromium.org/2278723005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html:8: function drawCanvas(ctx) { I know it was like this before, but this function adds unnecessary complexity. The drawing code could just be in-lined below.
The CQ bit was unchecked by siva.gunturi@samsung.com
@junov, ptal.. https://codereview.chromium.org/2278723005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/2d.composite.globalAlpha.fillPath.html (right): https://codereview.chromium.org/2278723005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/2d.composite.globalAlpha.fillPath.html:5: <title>Correct sync behaviour with globalAlpha and fillPath() in accelerated-2d-canvas mode.</title> On 2016/08/25 14:05:30, Justin Novosad wrote: > It is not clear what is meant by "sync behavior". Also, there is no > "fillPath()", it is just "fill()". Done. https://codereview.chromium.org/2278723005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html (right): https://codereview.chromium.org/2278723005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html:2: <title>Test the color gradient is applied properly for the rect in 2d canvas.</title> On 2016/08/25 14:05:30, Justin Novosad wrote: > the color gradient -> that color gradient > the rect -> rect Done. https://codereview.chromium.org/2278723005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/canvas/2d.fillText.gradient.html:8: function drawCanvas(ctx) { On 2016/08/25 14:05:30, Justin Novosad wrote: > I know it was like this before, but this function adds unnecessary complexity. > The drawing code could just be in-lined below. Done.
lgtm
The CQ bit was checked by siva.gunturi@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com Link to the patchset: https://codereview.chromium.org/2278723005/#ps80001 (title: "Test changed as per the comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Convert LayoutTests/fast/canvas/2d tests to testharness This patch uses testharness.js asserts to test 2d.composite.globalAlpha.fillPath.html, 2d.fillText.gradient.html. Remove the expectation files as they are not needed any more. BUG=639732 ========== to ========== Convert LayoutTests/fast/canvas/2d tests to testharness This patch uses testharness.js asserts to test 2d.composite.globalAlpha.fillPath.html, 2d.fillText.gradient.html. Remove the expectation files as they are not needed any more. BUG=639732 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Convert LayoutTests/fast/canvas/2d tests to testharness This patch uses testharness.js asserts to test 2d.composite.globalAlpha.fillPath.html, 2d.fillText.gradient.html. Remove the expectation files as they are not needed any more. BUG=639732 ========== to ========== Convert LayoutTests/fast/canvas/2d tests to testharness This patch uses testharness.js asserts to test 2d.composite.globalAlpha.fillPath.html, 2d.fillText.gradient.html. Remove the expectation files as they are not needed any more. BUG=639732 Committed: https://crrev.com/ed0a7ec9b9a1b54d12cbc0e50b9f87599e62e629 Cr-Commit-Position: refs/heads/master@{#414508} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ed0a7ec9b9a1b54d12cbc0e50b9f87599e62e629 Cr-Commit-Position: refs/heads/master@{#414508} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
