Bug 85451 – Add from-image to css3-images image-resolution

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[David Barr]

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.

[David Barr]

Created attachment 139944 [details]
Patch

[David Barr]

Created attachment 143744 [details]
Patch

Updated ref-test.

[David Barr]

Created attachment 144953 [details]
Patch

Updated patch to use compile time flag.

[David Barr]

Created attachment 147230 [details]
Patch

Rebased patch against r119984: <http://trac.webkit.org/changeset/119984>

[David Barr]

Created attachment 147457 [details]
Patch

Updated ChangeLog and rebased against pending patch: https://bugs.webkit.org/attachment.cgi?id=147397

[Tony Chang]

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.

[David Barr]

> (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.

[David Barr]

Created attachment 147725 [details]
Patch

Addressed Tony's comments and clarified test.

[Tony Chang]

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.

[David Barr]

(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.

[David Barr]

Created attachment 148032 [details]
Patch

Renamed enum and attribute that holds the 'from-image' bit.

[David Barr]

Created attachment 148045 [details]
Patch

Updated WebCore::RenderImage::styleDidChange and ChangeLog. Previous revisions of the patch omitted some dynamic layout logic.

[Tony Chang]

(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.

[David Barr]

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.

[WebKit Review Bot]

Comment on attachment 148045 [details]
Patch

Clearing flags on attachment: 148045

Committed r120641: <http://trac.webkit.org/changeset/120641>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

(In reply to comment #0)
> However, defer extracting resolution data from images to another bug.

Do we have a bug for this?

[David Barr]

> > 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