[webkit-reviews] review denied: [Bug 10794] [CSS1] Background-attachment: scroll should scroll within the containing block : [Attachment 20557] patch (updated 2)

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Anatoli Papirovski
<apapirovski at mac.com>'s request for review:
Bug 10794: [CSS1] Background-attachment: scroll should scroll within the
containing block
http://bugs.webkit.org/show_bug.cgi?id=10794

Attachment 20557: patch (updated 2)
http://bugs.webkit.org/attachment.cgi?id=20557&action=edit

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
- h = bg->imageSize(style()->effectiveZoom()).height() * scaledWidth /
bg->imageSize(style()->effectiveZoom()).width();
+ h = bg->imageSize(style()->effectiveZoom()).height() * w /
bg->imageSize(style()->effectiveZoom()).width();

This change fixes computation of height, but isn't the corresponding width case
also broken?  You could adjust the background-size computation so that it
computes values of w and h first (for fixed/percent).  Then handle the auto
cases for each after that.  (That enables you to fix the width case also.)

So instead of 

if (width is fixed)
else if (width is percent)
else if (width is auto)

if (height is fixed)
else if (height is percent)
else if (height is auto)

you could do:

if (width is fixed)
else if (width is percent)

if (height is fixed)
else if (height is percent)

if (width is auto && !height is auto)
else if (!width is auto && height is auto)
else if (width and height both auto)