LayoutTests/fast/canvas/canvas-state-stack-simple.html - Issue 501353002: Transfer canvas state to the next frame with noticable restrictions.

Side by Side Diff: LayoutTests/fast/canvas/canvas-state-stack-simple.html

Issue 501353002: Transfer canvas state to the next frame with noticable restrictions. (Closed) Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Patch Set: Release candidate Created 6 years, 3 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <html>

	3 <head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
	zino 2014/09/13 16:00:28 These tags are normally omitted. <html>, <head>, < These tags are normally omitted. <html>, <head>, <body> http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h... I think your new test should be the following form. -------------------------------------------- <!DOCTYPE html> <canvas></canvas> <script> var context; if (window.testRunner) ... context... context... requestAnimationFrame(drawSecondFrame); function drawSecondFrame() { ... } </script> -------------------------------------------- Sergey 2014/09/15 02:45:35 This is probably a recommendation, when you are go Show quoted text On 2014/09/13 16:00:28, zino wrote: > These tags are normally omitted. > <html>, <head>, <body> > http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h... This is probably a recommendation, when you are going to test something with JS only and use 'shouldBe' checks e.t.c. In my case I would probably prefer to emphasize that resulted html is compared. And, actually, it seems to be the same case for many other layout tests in the current folder: LayoutTests/fast/canvas/canvas-skia-excessive-size.html LayoutTests/fast/canvas/canvas-shadow.html LayoutTests/fast/canvas/canvas-setTransform.html LayoutTests/fast/canvas/canvas-scale-shadowBlur.html LayoutTests/fast/canvas/canvas-partial-invalidation-zoomed.html zino 2014/09/15 09:51:03 I don't think the rule is related to "js-test" fra Show quoted text On 2014/09/15 02:45:35, Sergey wrote: > On 2014/09/13 16:00:28, zino wrote: > > These tags are normally omitted. > > <html>, <head>, <body> > > > http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h... > > This is probably a recommendation, when you are going to test something with JS > only and use 'shouldBe' checks e.t.c. > > In my case I would probably prefer to emphasize that resulted html is compared. > And, actually, it seems to be the same case for many other layout tests in the > current folder: > LayoutTests/fast/canvas/canvas-skia-excessive-size.html > LayoutTests/fast/canvas/canvas-shadow.html > LayoutTests/fast/canvas/canvas-setTransform.html > LayoutTests/fast/canvas/canvas-scale-shadowBlur.html > LayoutTests/fast/canvas/canvas-partial-invalidation-zoomed.html I don't think the rule is related to "js-test" framework. I'm not sure but I think the guide is always applied if your test isn't w3c platform test. http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-S... However, It is not important. (just nits) So, I'm fine anyway.
	4 <script>

	5 var context;

	6

	7 window.onload = function() {

	8 if (window.testRunner)

	9 testRunner.waitUntilDone();
	zino 2014/09/13 16:00:28 fix indentation? Either of 2 space and 4 space in fix indentation? Either of 2 space and 4 space indentation. The usage should be consistent within a test file. http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-I... Sergey 2014/09/15 02:45:36 Done. Show quoted text On 2014/09/13 16:00:28, zino wrote: > fix indentation? > > Either of 2 space and 4 space indentation. > The usage should be consistent within a test file. > > http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-I... Done.
	10 context = document.getElementById("c").getContext("2d");
	zino 2014/09/13 16:00:28 "c" -> 'c' "2d" -> '2d' Some tests use double quo "c" -> 'c' "2d" -> '2d' Some tests use double quote for HTML (e.g. <script src="some.js">) and single quote for JavaScript (e.g. var image = 'resources/image.jpg';), but it could be either. The usage should be consistent within a test file. http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-Q... Sergey 2014/09/15 02:45:36 Done. Show quoted text On 2014/09/13 16:00:28, zino wrote: > "c" -> 'c' > "2d" -> '2d' > > Some tests use double quote for HTML (e.g. <script src="some.js">) and single > quote for JavaScript (e.g. var image = 'resources/image.jpg';), but it could be > either. > The usage should be consistent within a test file. > > http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-Q... Done.
	11 context.scale(2,2);
	zino 2014/09/13 16:00:28 fix spacing? fix spacing? Sergey 2014/09/15 02:45:36 Here everything seems fine... Please, explain. Show quoted text On 2014/09/13 16:00:28, zino wrote: > fix spacing? Here everything seems fine... Please, explain. zino 2014/09/15 09:51:03 It is not important but it is better to have a con Show quoted text On 2014/09/15 02:45:36, Sergey wrote: > On 2014/09/13 16:00:28, zino wrote: > > fix spacing? > > Here everything seems fine... Please, explain. It is not important but it is better to have a consistency. The [space] character should be consistent within a test file. context.scale(2,2); -> context.scale(2,[space]2); context.rect(0,0,75,75); -> context.rect(0,[space]0,[space]75,[space]75); context.scale(0.5,0.5); -> .. context.rect(0,0,300,300); -> .. You already use [space] character here. context.fillRect(0, 0, 30, 30); context.fillRect(50, 50, 30, 30); context.fillRect(0, 0, 30, 30); context.fillRect(50, 50, 30, 30);
	12 context.beginPath();

	13 context.rect(0,0,75,75);

	14 context.clip();

	15 context.fillStyle = 'red';

	16 context.fillRect(0, 0, 30, 30);

	17 context.fillRect(50, 50, 30, 30);

	18 context.save();

	19 context.scale(0.5,0.5);

	20 context.beginPath();

	21 context.rect(0,0,300,300);
	Justin Novosad 2014/09/12 14:54:35 This test would be more interesting if you used a This test would be more interesting if you used a small rect here, or even an empty rect. zino 2014/09/13 16:00:28 good point. Show quoted text On 2014/09/12 14:54:35, junov wrote: > This test would be more interesting if you used a small rect here, or even an > empty rect. good point. Sergey 2014/09/15 02:45:36 Done. Show quoted text On 2014/09/12 14:54:35, junov wrote: > This test would be more interesting if you used a small rect here, or even an > empty rect. Done.
	22 context.clip();
	zino 2014/09/13 16:00:28 Also, I think it is better to add context.fillRect Also, I think it is better to add context.fillRect() here. Sergey 2014/09/15 02:45:36 May be that would be excessive, wouldn't it? Show quoted text On 2014/09/13 16:00:28, zino wrote: > Also, I think it is better to add context.fillRect() here. May be that would be excessive, wouldn't it?
	23 requestAnimationFrame(drawSecondFrame);

	24 }

	25

	26 function drawSecondFrame() {

	27 context.restore();

	28 context.fillStyle = 'green';

	29 context.fillRect(0, 0, 30, 30);

	30 context.fillRect(50, 50, 30, 30);

	31 if (window.testRunner)

	32 testRunner.notifyDone();

	33 }

	34 </script>

	35 </head>

	36 <body>

	37 <p>Two green squares of different sizes should appear below</p>

	38 <canvas id="c" width=300 height=300></canvas>
	zino 2014/09/13 16:00:28 width="300" height="300" width="300" height="300" Sergey 2014/09/15 02:45:36 Done. Show quoted text On 2014/09/13 16:00:28, zino wrote: > width="300" height="300" Done.
	39 </body>

	40 </html>

OLD	NEW

« no previous file with comments | « no previous file | LayoutTests/fast/canvas/canvas-state-stack-simple-expected.html » ('j') | Source/platform/graphics/RecordingImageBufferSurface.cpp » ('J')