background-size has a new "contain" value: Scale the image, while preserving its intrinsic aspect ratio (if any), to the largest size such that both its width and its height can fit inside the background positioning area.
Created attachment 35132 [details] Patch for the CSS part of contain and cover Here is a patch for the CSS part of supporting contain and cover as values for background-size. There are a few things that I am unsure about: 1. Should the code that is share with WebKitMaskSize be broken out so that WebKitMaskSize does not support contain and cover? Right now, I implemented it to continue matching background-size. 2. In CSSPrimitiveValueMappings.h I was unsure what to return in the normal case when the value is a pair of numbers. Those are the main things, I think.
Comment on attachment 35132 [details] Patch for the CSS part of contain and cover Review comments. I has them. > + Return contain or cover when appropriate, and otherwise so what we > + used to do. Typo: “so”. > + backgroundSize() now returns an EBackgroundSize, and > + backgroundSizeLength() returns the numbers I think you meant “lengths” since they can still be 'auto' or a percentage. > # > +# css_prop_background_size > +# We no longer have these weird enum values for the properties, and I never understood why the comments were in term of those. You can just say “background-size”. > Index: WebCore/rendering/RenderBoxModelObject.cpp > =================================================================== > --- WebCore/rendering/RenderBoxModelObject.cpp (revision 47461) > +++ WebCore/rendering/RenderBoxModelObject.cpp (working copy) > @@ -497,8 +497,8 @@ IntSize RenderBoxModelObject::calculateB > if (bgLayer->isSizeSet()) { Not sure why you didn’t change this one to isSizeLengthSet()… > Index: WebCore/rendering/RenderObject.cpp > =================================================================== > --- WebCore/rendering/RenderObject.cpp (revision 47461) > +++ WebCore/rendering/RenderObject.cpp (working copy) > @@ -656,8 +656,8 @@ static bool mustRepaintFillLayers(const > if (!layer->xPosition().isZero() || !layer->yPosition().isZero()) > return true; > > - if (layer->isSizeSet()) { > - if (layer->size().width().isPercent() || layer->size().height().isPercent()) > + if (layer->isSizeLengthSet()) { > + if (layer->sizeLength().width().isPercent() || layer->sizeLength().height().isPercent()) …because you did change this one. I think instead of adding another “is set” bit, you can get rid of the existing one and just add a value to the EBackgroundSize enum representing the “not set” state. But now I am not sure I fully understand why we need this notion in the first place and what’s the difference between “not set” and 'background-size: auto, auto'.
(In reply to comment #2) > But now I am not sure I fully understand why we need this notion in the > first place and what’s the difference between “not set” and 'background-size: > auto, auto'. Oh, it’s about filling missing values.
Created attachment 35175 [details] Patch with Dan comments Here is a patch that addresses all of Dan's comments. Thanks Dan!
I had a third patch that Dan reviewed, and I committed it with r47630.
Comment on attachment 35175 [details] Patch with Dan comments R-minusing this one since I had a third patch that has already been reviewed and landed.
Created attachment 38406 [details] Rendering changes for "contain" and "cover"
Comment on attachment 38406 [details] Rendering changes for "contain" and "cover" Looks good!
Landed the rendering changes as <http://trac.webkit.org/projects/webkit/changeset/47650>.