Add a layout test for APNG

Add a web platform test for APNG

While animated-png.html uses internals for selecting frames and
verifying that we decode APNGs properly, this test confirms that a
basic animation will work correctly, and can work in other browsers.

- apng.png
A simple APNG with two frames. The first frame is red, and displays
for as short a time as possible (11 ms; anything less than that and
Chromium increases the time), followed by a green frame. The image
does not loop. The green frame matches green.png, which already
exists

- animated-png-timeout-ref.html
Reference page showing the existing image (green.png).

BUG=437662
Review-Url: https://codereview.chromium.org/2814453004
Cr-Commit-Position: refs/heads/master@{#464038}
Committed: https://chromium.googlesource.com/chromium/src/+/08f24841310c677a67f79f89253d971820069da4

[foolip, 2017-04-11 04:19:02]

foolip@chromium.org changed reviewers:
+ foolip@chromium.org

[foolip, 2017-04-11 04:19:03]

https://codereview.chromium.org/2814453004/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/images/animated-png-timeout.html (right):

https://codereview.chromium.org/2814453004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/images/animated-png-timeout.html:5: img { margin:
1px; }
Is this needed, can't it be removed both here and in the reference? Ditto for
style="margin: 1px" below.

[scroggo_chromium, 2017-04-11 13:57:50]

Patch set 3 is my last one as a LayoutTest, and patch set 4 attempts to convert
to a web platform test. How do I run a web platform test? (I'm assuming I have
to move it into LayoutTests/external/wpt first? Which sub-directory? My first
guess was "images", but that appears to be a directory with image resources,
rather than tests of images. Maybe there should be an 'img' directory?)

I tried following the directions at
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/...,
but

  git submodule update --init --recursive

did not appear to do anything (I suppose that's from the directory
third_party/WebKit/LayoutTests/external/wpt/ ? I also tried from src/ just in
case), and neither did

  ./serve

(I don't see a file named "serve", but maybe that's because the submodule update
did not work?)

Once I get that working, what is the next step?

https://codereview.chromium.org/2814453004/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/images/animated-png-timeout.html (right):

https://codereview.chromium.org/2814453004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/images/animated-png-timeout.html:5: img { margin:
1px; }
On 2017/04/11 04:19:03, foolip_UTC7 wrote:
> Is this needed, can't it be removed both here and in the reference? Ditto for
> style="margin: 1px" below.

Not necessary. Carried over from another test.

[foolip, 2017-04-11 14:18:44]

On 2017/04/11 13:57:50, scroggo_chromium wrote:
> Patch set 3 is my last one as a LayoutTest, and patch set 4 attempts to
convert
> to a web platform test. How do I run a web platform test? (I'm assuming I have
> to move it into LayoutTests/external/wpt first? Which sub-directory? My first
> guess was "images", but that appears to be a directory with image resources,
> rather than tests of images. Maybe there should be an 'img' directory?)

I've asked in https://github.com/w3c/web-platform-tests/issues/5531 and think
that just apng/ would work, let's give people some time to react there.

> I tried following the directions at
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/...,
> but
> 
>   git submodule update --init --recursive
> 
> did not appear to do anything (I suppose that's from the directory
> third_party/WebKit/LayoutTests/external/wpt/ ? I also tried from src/ just in
> case), and neither did
> 
>   ./serve
> 
> (I don't see a file named "serve", but maybe that's because the submodule
update
> did not work?)
> 
> Once I get that working, what is the next step?

Things that are in LayoutTests/external/wpt mostly work like other layout tests,
i.e. you can run them using Tools/Scripts/run-webkit-tests and magic will take
care of starting wptserve for you and all that, just like for LayoutTests/http/,
where Apache is used.

The README.md is imported from upstream web-platform-tests and becomes a bit
confusing, because we flatten and copy web-platform-tests, there are no
submodules.

https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_plat...
is supposed to be our documentation for wpt in Chromium, so if there's anything
that should be added there, please let me know!

[scroggo_chromium, 2017-04-11 19:02:52]

On 2017/04/11 14:18:44, foolip_UTC7 wrote:
> On 2017/04/11 13:57:50, scroggo_chromium wrote:
> > Patch set 3 is my last one as a LayoutTest, and patch set 4 attempts to
> convert
> > to a web platform test. How do I run a web platform test? (I'm assuming I
have
> > to move it into LayoutTests/external/wpt first? Which sub-directory? My
first
> > guess was "images", but that appears to be a directory with image resources,
> > rather than tests of images. Maybe there should be an 'img' directory?)
> 
> I've asked in https://github.com/w3c/web-platform-tests/issues/5531 and think
> that just apng/ would work, let's give people some time to react there.

Thanks! I subscribed to that issue, so I will see the responses. I think I read
somewhere that new folders should be added upstream?

> 
> > I tried following the directions at
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/...,
> > but
> > 
> >   git submodule update --init --recursive
> > 
> > did not appear to do anything (I suppose that's from the directory
> > third_party/WebKit/LayoutTests/external/wpt/ ? I also tried from src/ just
in
> > case), and neither did
> > 
> >   ./serve
> > 
> > (I don't see a file named "serve", but maybe that's because the submodule
> update
> > did not work?)
> > 
> > Once I get that working, what is the next step?
> 
> Things that are in LayoutTests/external/wpt mostly work like other layout
tests,
> i.e. you can run them using Tools/Scripts/run-webkit-tests and magic will take
> care of starting wptserve for you and all that, just like for
LayoutTests/http/,
> where Apache is used.
> 
> The README.md is imported from upstream web-platform-tests and becomes a bit
> confusing, because we flatten and copy web-platform-tests, there are no
> submodules.
> 
>
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_plat...
> is supposed to be our documentation for wpt in Chromium, so if there's
anything
> that should be added there, please let me know!

Ah, thanks for the explanation. I guess the magic (e.g. reftest-wait) only works
when the test is inside external/wpt? I tried running with run-webkit-tests and
it failed, so I figured I must have been doing something wrong.

I've moved everything to the correct place (assuming people agree on apng/ in
https://github.com/w3c/web-platform-tests/issues/5531) and everything is working
now. (Thanks!)

PTAL

[foolip, 2017-04-12 04:44:12]

On 2017/04/11 19:02:52, scroggo_chromium wrote:
> On 2017/04/11 14:18:44, foolip_UTC7 wrote:
> > I've asked in https://github.com/w3c/web-platform-tests/issues/5531 and
think
> > that just apng/ would work, let's give people some time to react there.
> 
> Thanks! I subscribed to that issue, so I will see the responses. I think I
read
> somewhere that new folders should be added upstream?

That's
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_plat...

Either way is fine, the bit that has to be done upstream is adding the OWNERS
files, since those aren't synced.

> Ah, thanks for the explanation. I guess the magic (e.g. reftest-wait) only
works
> when the test is inside external/wpt? I tried running with run-webkit-tests
and
> it failed, so I figured I must have been doing something wrong.

Yep, reftest-wait only works for web-platform-tests, see
https://cs.chromium.org/chromium/src/content/shell/test_runner/test_runner.cc

[foolip, 2017-04-12 04:55:20]

Just some nits.

https://codereview.chromium.org/2814453004/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/apng/animated-png-timeout-expected.html
(right):

https://codereview.chromium.org/2814453004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/apng/animated-png-timeout-expected.html:3:
<title>APNG test: Second frame displays quickly, replacing red with
green.</title>
References can be minimal, and a title won't affect anything. Try leaving *just*
the <img> element.

https://codereview.chromium.org/2814453004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/apng/animated-png-timeout-expected.html:5:
<img src=../images/apng-test-ref.png>
I looked for other *-ref.png and only found a few in css/css-fonts-3/support/.

Since the ref image won't be generally useful in images, I suggest just putting
these two inside apng/. Alternatively, if you could make the second frame of the
APNG match images/green.png, that'd be nifty. Then putting the APNG file under
images/apng.png would make sense. (Having "test" in the filename isn't necessary
or common.)

https://codereview.chromium.org/2814453004/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/external/wpt/apng/animated-png-timeout.html
(right):

https://codereview.chromium.org/2814453004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/apng/animated-png-timeout.html:2:
<head>
It's OK, and some reviewers like me encourage, to remove bits of markup that
aren't needed. Removing <head>, </head>, </body> and </html> from this test
should be fine. (There is no <body>, it's now implied by the <img> element.)

https://codereview.chromium.org/2814453004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/apng/animated-png-timeout.html:3:
<title>APNG test: Second frame displays quickly, replacing red with
green.</title>
uNit: Having "test" as part of the test description almost never adds anything,
and here it can be dropped with no harm done.

https://codereview.chromium.org/2814453004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/apng/animated-png-timeout.html:5:
<link rel="match" href="animated-png-timeout-expected.html"/>
Because of this syntax the name can be anything, but *-expected.txt will not be
exported and if *-expected.html are, that's rather confusing I'd say. The naming
pattern in web-platform-tests is usually -ref.html, so
animated-png-timeout-ref.html

[scroggo_chromium, 2017-04-12 14:03:48]

Description was changed from

==========
Add a layout test for APNG

While animated-png.html uses internals for selecting frames and
verifying that we decode APNGs properly, this test confirms that a
basic animation will work correctly.

- apng-test.png
A simple APNG with two frames. The first frame is red, and displays
for as short a time as possible (11 ms; anything less than that and
Chromium increases the time), followed by a green frame. The image
does not loop.

- animated-png-timeout.html
Uses a timeout of a second, after which time the second frame should
be showing.

- animated-png-timeout-expected.html; apng-test-ref.png
Reference page and png.

BUG=437662
==========

to

==========
Add a web platform test for APNG

While animated-png.html uses internals for selecting frames and
verifying that we decode APNGs properly, this test confirms that a
basic animation will work correctly, and can work in other browsers.

- apng.png
A simple APNG with two frames. The first frame is red, and displays
for as short a time as possible (11 ms; anything less than that and
Chromium increases the time), followed by a green frame. The image
does not loop. The green frame matches green.png, which already
exists

- animated-png-timeout-ref.html
Reference page showing the existing image (green.png).

BUG=437662
==========

[scroggo_chromium, 2017-04-12 14:04:12]

On 2017/04/12 04:44:12, foolip_UTC7 wrote:
> On 2017/04/11 19:02:52, scroggo_chromium wrote:
> > On 2017/04/11 14:18:44, foolip_UTC7 wrote:
> > > I've asked in https://github.com/w3c/web-platform-tests/issues/5531 and
> think
> > > that just apng/ would work, let's give people some time to react there.
> > 
> > Thanks! I subscribed to that issue, so I will see the responses. I think I
> read
> > somewhere that new folders should be added upstream?
> 
> That's
>
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_plat...
> 
> Either way is fine, the bit that has to be done upstream is adding the OWNERS
> files, since those aren't synced.
> 
> > Ah, thanks for the explanation. I guess the magic (e.g. reftest-wait) only
> works
> > when the test is inside external/wpt? I tried running with run-webkit-tests
> and
> > it failed, so I figured I must have been doing something wrong.
> 
> Yep, reftest-wait only works for web-platform-tests, see
> https://cs.chromium.org/chromium/src/content/shell/test_runner/test_runner.cc

Thanks for the pointers :)

https://codereview.chromium.org/2814453004/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/external/wpt/apng/animated-png-timeout-expected.html
(right):

https://codereview.chromium.org/2814453004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/apng/animated-png-timeout-expected.html:3:
<title>APNG test: Second frame displays quickly, replacing red with
green.</title>
On 2017/04/12 04:55:20, foolip_UTC7 wrote:
> References can be minimal, and a title won't affect anything. Try leaving
*just*
> the <img> element.

Done.

https://codereview.chromium.org/2814453004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/apng/animated-png-timeout-expected.html:5:
<img src=../images/apng-test-ref.png>
On 2017/04/12 04:55:20, foolip_UTC7 wrote:
> I looked for other *-ref.png and only found a few in css/css-fonts-3/support/.
> 
> Since the ref image won't be generally useful in images, I suggest just
putting
> these two inside apng/. Alternatively, if you could make the second frame of
the
> APNG match images/green.png, that'd be nifty. Then putting the APNG file under
> images/apng.png would make sense. (Having "test" in the filename isn't
necessary
> or common.)

Done. Renamed apng-test.png to apng.png, and modified it so it will match
green.png on the second frame.

https://codereview.chromium.org/2814453004/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/external/wpt/apng/animated-png-timeout.html
(right):

https://codereview.chromium.org/2814453004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/apng/animated-png-timeout.html:2:
<head>
On 2017/04/12 04:55:20, foolip_UTC7 wrote:
> It's OK, and some reviewers like me encourage, to remove bits of markup that
> aren't needed. Removing <head>, </head>, </body> and </html> from this test
> should be fine. (There is no <body>, it's now implied by the <img> element.)

Done.

https://codereview.chromium.org/2814453004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/apng/animated-png-timeout.html:3:
<title>APNG test: Second frame displays quickly, replacing red with
green.</title>
On 2017/04/12 04:55:20, foolip_UTC7 wrote:
> uNit: Having "test" as part of the test description almost never adds
anything,
> and here it can be dropped with no harm done.

Done.

https://codereview.chromium.org/2814453004/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/external/wpt/apng/animated-png-timeout.html:5:
<link rel="match" href="animated-png-timeout-expected.html"/>
On 2017/04/12 04:55:20, foolip_UTC7 wrote:
> Because of this syntax the name can be anything, but *-expected.txt will not
be
> exported and if *-expected.html are, that's rather confusing I'd say. The
naming
> pattern in web-platform-tests is usually -ref.html, so
> animated-png-timeout-ref.html

Done.

[foolip, 2017-04-12 14:34:21]

The CQ bit was checked by foolip@chromium.org

[foolip, 2017-04-12 14:34:22]

lgtm

Yay, thanks!

[commit-bot: I haz the power, 2017-04-12 14:34:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-12 15:43:15]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492007661799870,
"parent_rev": "c1d98a208cadc9b2ee92a82f641f026f1df9490d", "commit_rev":
"08f24841310c677a67f79f89253d971820069da4"}

[commit-bot: I haz the power, 2017-04-12 15:44:08]

Description was changed from

==========
Add a web platform test for APNG

While animated-png.html uses internals for selecting frames and
verifying that we decode APNGs properly, this test confirms that a
basic animation will work correctly, and can work in other browsers.

- apng.png
A simple APNG with two frames. The first frame is red, and displays
for as short a time as possible (11 ms; anything less than that and
Chromium increases the time), followed by a green frame. The image
does not loop. The green frame matches green.png, which already
exists

- animated-png-timeout-ref.html
Reference page showing the existing image (green.png).

BUG=437662
==========

to

==========
Add a web platform test for APNG

While animated-png.html uses internals for selecting frames and
verifying that we decode APNGs properly, this test confirms that a
basic animation will work correctly, and can work in other browsers.

- apng.png
A simple APNG with two frames. The first frame is red, and displays
for as short a time as possible (11 ms; anything less than that and
Chromium increases the time), followed by a green frame. The image
does not loop. The green frame matches green.png, which already
exists

- animated-png-timeout-ref.html
Reference page showing the existing image (green.png).

BUG=437662

Review-Url: https://codereview.chromium.org/2814453004
Cr-Commit-Position: refs/heads/master@{#464038}
Committed:
https://chromium.googlesource.com/chromium/src/+/08f24841310c677a67f79f89253d...
==========

[commit-bot: I haz the power, 2017-04-12 15:44:09]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/08f24841310c677a67f79f89253d...