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.
Created attachment 139944 [details] Patch
Created attachment 143744 [details] Patch Updated ref-test.
Created attachment 144953 [details] Patch Updated patch to use compile time flag.
Created attachment 147230 [details] Patch Rebased patch against r119984: <http://trac.webkit.org/changeset/119984>
Created attachment 147457 [details] Patch Updated ChangeLog and rebased against pending patch: https://bugs.webkit.org/attachment.cgi?id=147397
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.
> (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.
Created attachment 147725 [details] Patch Addressed Tony's comments and clarified test.
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.
(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.
Created attachment 148032 [details] Patch Renamed enum and attribute that holds the 'from-image' bit.
Created attachment 148045 [details] Patch Updated WebCore::RenderImage::styleDidChange and ChangeLog. Previous revisions of the patch omitted some dynamic layout logic.
(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 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 on attachment 148045 [details] Patch Clearing flags on attachment: 148045 Committed r120641: <http://trac.webkit.org/changeset/120641>
All reviewed patches have been landed. Closing bug.
(In reply to comment #0) > However, defer extracting resolution data from images to another bug. Do we have a bug for this?
> > 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