Bug 89745 - Add snap to css3-images image-resolution
Summary: Add snap to css3-images image-resolution
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Barr
URL: http://www.w3.org/TR/2012/CR-css3-ima...
Depends on:
Blocks: 85262 91467
  Show dependency treegraph
Reported: 2012-06-22 00:23 PDT by David Barr
Modified: 2012-07-16 20:31 PDT (History)
8 users (show)

See Also:

Patch (38.62 KB, patch)
2012-06-22 00:33 PDT, David Barr
no flags Details | Formatted Diff | Diff
Patch for landing (38.82 KB, patch)
2012-06-24 17:37 PDT, David Barr
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Barr 2012-06-22 00:23:33 PDT
The css3-images module is at candidate recommendation.

I propose to introduce image-resolution, initially behind a compile-time flag.

Advertised on webkit-dev:

As a fifth step, add support for the snap value.
Comment 1 David Barr 2012-06-22 00:33:30 PDT
Created attachment 148975 [details]
Comment 2 Tony Chang 2012-06-22 10:55:42 PDT
Comment on attachment 148975 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=148975&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests; extended fast/css/image-resolution/image-resolution.html

This is OK, but the test covers a lot of different things.  If it starts to fail, it'll probably be harder to figure out what exactly is going wrong (800 lines of test!) than if we had a few more focused tests.

> LayoutTests/fast/css/image-resolution/image-resolution.html:43
> +        else return;

Nit: return on next line.

> LayoutTests/fast/css/image-resolution/image-resolution.html:48
> -        dppx = Math.floor(dppx);
> +        dppx = Math.floor(dppx + 0.01);

Is this due to floating point imprecision? Maybe make a mention of this in the ChangeLog.
Comment 3 David Barr 2012-06-24 17:37:03 PDT
Created attachment 149216 [details]
Patch for landing

Addressed nit; updated ChangeLog. The single test is getting quite large but I prefer to err on the side of coverage.
Comment 4 WebKit Review Bot 2012-06-24 18:46:29 PDT
Comment on attachment 149216 [details]
Patch for landing

Clearing flags on attachment: 149216

Committed r121127: <http://trac.webkit.org/changeset/121127>
Comment 5 WebKit Review Bot 2012-06-24 18:46:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Simon Fraser (smfr) 2012-06-24 23:16:50 PDT
Comment on attachment 149216 [details]
Patch for landing

Why is there no pixel or ref test here? Seems like nothing is testing the actual rendering.
Comment 7 David Barr 2012-07-16 20:31:59 PDT
Created bug 91467 to address Simon's comment.