Bug 85451

Summary: Add from-image to css3-images image-resolution
Product: WebKit Reporter: David Barr <davidbarr>
Component: CSSAssignee: David Barr <davidbarr>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, eric, macpherson, menard, mikelawther, noel.gordon, ojan, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://www.w3.org/TR/2012/CR-css3-images-20120417/#image-resolution
Bug Depends on: 85332    
Bug Blocks: 85262, 90140    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description David Barr 2012-05-02 20:21:17 PDT
The css3-images module is at candidate recommendation.
http://www.w3.org/TR/2012/CR-css3-images-20120417/#image-resolution

I propose to introduce image-resolution, initially behind a runtime feature flag.

Advertised on webkit-dev:
http://thread.gmane.org/gmane.os.opendarwin.webkit.devel/20505

As a fourth step, add support for the from-image value.
However, defer extracting resolution data from images to another bug.
For now, behave as though all images lack resolution data.
Comment 1 David Barr 2012-05-02 21:02:40 PDT
Created attachment 139944 [details]
Patch
Comment 2 David Barr 2012-05-23 23:44:57 PDT
Created attachment 143744 [details]
Patch

Updated ref-test.
Comment 3 David Barr 2012-05-30 17:28:12 PDT
Created attachment 144953 [details]
Patch

Updated patch to use compile time flag.
Comment 4 David Barr 2012-06-12 22:23:34 PDT
Created attachment 147230 [details]
Patch

Rebased patch against r119984: <http://trac.webkit.org/changeset/119984>
Comment 5 David Barr 2012-06-13 18:10:48 PDT
Created attachment 147457 [details]
Patch

Updated ChangeLog and rebased against pending patch: https://bugs.webkit.org/attachment.cgi?id=147397
Comment 6 Tony Chang 2012-06-14 10:17:20 PDT
Comment on attachment 147457 [details]
Patch

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

I realize this is not for review yet, but trying to provide some feedback since you're in a different time zone.

> Source/WebCore/rendering/style/RenderStyleConstants.h:457
> +enum EImageResolution { ImageResolutionInitial = 0, ImageResolutionFromImage };

Initial doesn't really say what the value is.  Maybe something like ImageResolutionNoFlags.  Is Snap going to be in the same enum?  I would also drop the E from the enum name (the E is from older code).

> LayoutTests/fast/css/image-resolution/image-resolution-expected.txt:85
> +FAIL img.offsetWidth should be 21. Was 16.
> +FAIL img.offsetHeight should be 21. Was 16.

Are these expected to fail?

> LayoutTests/fast/css/image-resolution/image-resolution.html:60
> +    var tests = [];
> +    tests.push.apply(tests, resolutions);

Nit: Arv tells me to deep copy an array, you can use slice.  E.g., var tests = resolutions.slice();

> LayoutTests/fast/css/image-resolution/image-resolution.html:62
> +        tests.push.apply(tests, permute2(resolution + ' from-image'));

Nit: It seems like it would be simpler to just enumerate resolution + ' from-image' and 'from-image ' + resolution.  I guess in the empty string case it's a bit redundant, but that's not a big deal.
Comment 7 David Barr 2012-06-14 20:40:10 PDT
> (From update of attachment 147457 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147457&action=review
> 
> I realize this is not for review yet, but trying to provide some feedback since you're in a different time zone.

Thanks for this.

> > Source/WebCore/rendering/style/RenderStyleConstants.h:457
> > +enum EImageResolution { ImageResolutionInitial = 0, ImageResolutionFromImage };
> 
> Initial doesn't really say what the value is.  Maybe something like ImageResolutionNoFlags.  Is Snap going to be in the same enum?  I would also drop the E from the enum name (the E is from older code).

Will apply suggested changes. Yes, Snap will eventually be in the same enum.

> > LayoutTests/fast/css/image-resolution/image-resolution-expected.txt:85
> > +FAIL img.offsetWidth should be 21. Was 16.
> > +FAIL img.offsetHeight should be 21. Was 16.
> 
> Are these expected to fail?

I'll amend the test to expect that plumbing hasn't been completed.

> > LayoutTests/fast/css/image-resolution/image-resolution.html:60
> > +    var tests = [];
> > +    tests.push.apply(tests, resolutions);
> 
> Nit: Arv tells me to deep copy an array, you can use slice.  E.g., var tests = resolutions.slice();

Thanks, will do.

> > LayoutTests/fast/css/image-resolution/image-resolution.html:62
> > +        tests.push.apply(tests, permute2(resolution + ' from-image'));
> 
> Nit: It seems like it would be simpler to just enumerate resolution + ' from-image' and 'from-image ' + resolution.  I guess in the empty string case it's a bit redundant, but that's not a big deal.

I plan to follow and extend this pattern when I add snap.
Comment 8 David Barr 2012-06-14 21:59:42 PDT
Created attachment 147725 [details]
Patch

Addressed Tony's comments and clarified test.
Comment 9 Tony Chang 2012-06-15 10:55:57 PDT
Comment on attachment 147725 [details]
Patch

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

> Source/WebCore/rendering/style/RenderStyleConstants.h:457
> +enum ImageResolution { ImageResolutionNoFlags = 0, ImageResolutionFromImage };

I think it would be more clear to have 2 enums. The memory usage is the same and it will make the values more clear.  E.g., 
enum ImageResolution { ImageResolutionSpecified, ImageResolutionFromImage }
enum ImageResolutionSnap { ImageResolutionNoSnap, ImageResolutionSnapPixels }

> LayoutTests/fast/css/image-resolution/image-resolution.html:70
> -var imgResolutionDppx = 72 / 96;
> -var dimensions = imgWidthPx + 'x' + imgHeightPx + '@' + imgResolutionDppx + 'dppx';
> +var imgResolutionDppx = 0; /* Embedded image resolution data not plumbed yet. */
> +var dimensions = imgWidthPx + 'x' + imgHeightPx;

Sorry, I just didn't understand why the tests were failing. It's OK to check in the real test with FAIL in the results as long as it's documented somewhere with a FIXME.
Comment 10 David Barr 2012-06-17 18:13:44 PDT
(In reply to comment #9)
> (From update of attachment 147725 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147725&action=review
> 
> > Source/WebCore/rendering/style/RenderStyleConstants.h:457
> > +enum ImageResolution { ImageResolutionNoFlags = 0, ImageResolutionFromImage };
> 
> I think it would be more clear to have 2 enums. The memory usage is the same and it will make the values more clear.  E.g., 
> enum ImageResolution { ImageResolutionSpecified, ImageResolutionFromImage }
> enum ImageResolutionSnap { ImageResolutionNoSnap, ImageResolutionSnapPixels }

Ok, I've chosen ImageResolutionSource { ImageResolutionSpecified, ImageResolutionFromImage } for this patch.

> > LayoutTests/fast/css/image-resolution/image-resolution.html:70
> > -var imgResolutionDppx = 72 / 96;
> > -var dimensions = imgWidthPx + 'x' + imgHeightPx + '@' + imgResolutionDppx + 'dppx';
> > +var imgResolutionDppx = 0; /* Embedded image resolution data not plumbed yet. */
> > +var dimensions = imgWidthPx + 'x' + imgHeightPx;
> 
> Sorry, I just didn't understand why the tests were failing. It's OK to check in the real test with FAIL in the results as long as it's documented somewhere with a FIXME.

Right, I decided it was better to adapt those failing tests to instead positively test the fallback behavior.
Comment 11 David Barr 2012-06-17 18:16:47 PDT
Created attachment 148032 [details]
Patch

Renamed enum and attribute that holds the 'from-image' bit.
Comment 12 David Barr 2012-06-17 21:18:16 PDT
Created attachment 148045 [details]
Patch

Updated WebCore::RenderImage::styleDidChange and ChangeLog. Previous revisions of the patch omitted some dynamic layout logic.
Comment 13 Tony Chang 2012-06-18 09:50:59 PDT
(In reply to comment #12)
> Created an attachment (id=148045) [details]
> Patch
> 
> Updated WebCore::RenderImage::styleDidChange and ChangeLog. Previous revisions of the patch omitted some dynamic layout logic.

Does the current test cover these dynamic layout cases? If not, a follow up patch adding tests for it would be great.
Comment 14 David Barr 2012-06-18 15:31:35 PDT
Comment on attachment 148045 [details]
Patch

The current test clears style before each rule. The dynamic application can be tested by removing the clear step and changing the order. Will follow up with more tests once this lands.
Comment 15 WebKit Review Bot 2012-06-18 16:30:48 PDT
Comment on attachment 148045 [details]
Patch

Clearing flags on attachment: 148045

Committed r120641: <http://trac.webkit.org/changeset/120641>
Comment 16 WebKit Review Bot 2012-06-18 16:31:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 noel gordon 2012-06-27 23:05:20 PDT
(In reply to comment #0)
> However, defer extracting resolution data from images to another bug.

Do we have a bug for this?
Comment 18 David Barr 2012-06-27 23:54:25 PDT
> > However, defer extracting resolution data from images to another bug.
> 
> Do we have a bug for this?

Created bug 90140 - Plumb embedded image-resolution data for from-image.
https://bugs.webkit.org/show_bug.cgi?id=90140