[webkit-reviews] review granted: [Bug 28635] [CSS3 Backgrounds and Borders] Add support for 2-keyword values for background-repeat : [Attachment 38762] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Aug 31 12:30:41 PDT 2009
Darin Adler <darin at apple.com> has granted Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 28635: [CSS3 Backgrounds and Borders] Add support for 2-keyword values for
background-repeat
https://bugs.webkit.org/show_bug.cgi?id=28635
Attachment 38762: Patch
https://bugs.webkit.org/attachment.cgi?id=38762&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
> + if (xRepeat == yRepeat)
> + return CSSPrimitiveValue::create(xRepeat);
> + else if (xRepeat == CSSValueRepeat && yRepeat ==
CSSValueNoRepeat)
> + return CSSPrimitiveValue::createIdentifier(CSSValueRepeatX);
> + else if (xRepeat == CSSValueNoRepeat && yRepeat ==
CSSValueRepeat)
> + return CSSPrimitiveValue::createIdentifier(CSSValueRepeatY);
No need for else after return, and normally we omit it.
> + case CSSPropertyWebkitMaskRepeat: {
> + EFillRepeat xRepeat = style->maskRepeatX();
> + EFillRepeat yRepeat = style->maskRepeatY();
> +
> + // For backwards compatibility, if both values are equal, just
return one of them. And
> + // if the two values are equivalent to repeat-x or repeat-y,
just return the shorthand.
> + if (xRepeat == yRepeat)
> + return CSSPrimitiveValue::create(xRepeat);
> + else if (xRepeat == CSSValueRepeat && yRepeat ==
CSSValueNoRepeat)
> + return CSSPrimitiveValue::createIdentifier(CSSValueRepeatX);
> + else if (xRepeat == CSSValueNoRepeat && yRepeat ==
CSSValueRepeat)
> + return CSSPrimitiveValue::createIdentifier(CSSValueRepeatY);
> +
> + RefPtr<CSSValueList> list =
CSSValueList::createSpaceSeparated();
> + list->append(CSSPrimitiveValue::create(xRepeat));
> + list->append(CSSPrimitiveValue::create(yRepeat));
> + return list.release();
> + }
This should use a function that takes xRepeat and yRepeat arguments so
mask-repeat can share almost all its code with background-repeat.
> + case CSSPropertyBackgroundRepeat: {
> + const int properties[2] = { CSSPropertyBackgroundRepeatX,
> + CSSPropertyBackgroundRepeatY };
> + return getLayeredShorthandValue(properties, 2);
> + }
I think the property names would look better on a single line here.
> + case CSSPropertyWebkitMaskRepeat: {
> + const int properties[2] = { CSSPropertyWebkitMaskRepeatX,
> + CSSPropertyWebkitMaskRepeatY };
> + return getLayeredShorthandValue(properties, 2);
> + }
Here too.
> + RefPtr<CSSValue> yValue;
> + if (values[j + 1]->isValueList())
> + yValue = static_cast<CSSValueList*>(values[j +
1].get())->itemWithoutBoundsCheck(i);
> + else
> + yValue = values[j + 1];
With values[j + 1] repeated three times I suspect it would read better if that
was in a local variable.
> + int xId =
static_cast<CSSPrimitiveValue*>(value.get())->getIdent();
> + int yId =
static_cast<CSSPrimitiveValue*>(yValue.get())->getIdent();
There’s an extra space here after the "=" sign.
All that code appending string is quite inefficient, but that's nothing new.
> case NoRepeatFill:
> m_value.ident = CSSValueNoRepeat;
> + break;
> case RoundFill:
> m_value.ident = CSSValueRound;
> + break;
> case SpaceFill:
> m_value.ident = CSSValueSpace;
> break;
Are there tests that cover these two missing break statements?
> + case CSSPropertyBackgroundRepeat:
> + HANDLE_BACKGROUND_INHERIT_AND_INITIAL(repeatX, RepeatX);
> + HANDLE_BACKGROUND_INHERIT_AND_INITIAL(repeatY, RepeatY);
> + return;
> + case CSSPropertyBackgroundRepeatX: {
> + HANDLE_BACKGROUND_VALUE(repeatX, RepeatX, value)
> + return;
> + }
> + case CSSPropertyBackgroundRepeatY: {
> + HANDLE_BACKGROUND_VALUE(repeatY, RepeatY, value)
> + return;
> + }
> + case CSSPropertyWebkitMaskRepeat:
> + HANDLE_MASK_INHERIT_AND_INITIAL(repeatX, RepeatX);
> + HANDLE_MASK_INHERIT_AND_INITIAL(repeatY, RepeatY);
> + return;
> + case CSSPropertyWebkitMaskRepeatX: {
> + HANDLE_MASK_VALUE(repeatX, RepeatX, value)
> + return;
> + }
> + case CSSPropertyWebkitMaskRepeatY: {
> + HANDLE_MASK_VALUE(repeatY, RepeatY, value)
> + return;
> + }
Do the HANDLE_BACKGROUND_VALUE and HANDLE_MASK_VALUE ones really need braces?
I'm surprised since the others don't have them.
> -background-image: url(about:blank); background-repeat: initial;
background-attachment: initial; background-origin: initial; background-clip:
initial; background-color: initial; background-position: 80% 50px;
> +background-image: url(about:blank); background-attachment: initial;
background-origin: initial; background-clip: initial; background-color:
initial; background-position: 80% 50px; background-repeat: initial initial;
The order here seems random. Should we define the order somehow? By changing
the code to do things in a canonical order?
> -Removing background removes background-image, background-repeat,
background-attachment, background-origin, background-clip, background-color,
background-position.
> +Removing background removes background-image, background-attachment,
background-origin, background-clip, background-color, background-position,
background-repeat.
Ditto.
> -Removing -webkit-mask removes -webkit-mask-image, -webkit-mask-repeat,
-webkit-mask-attachment, -webkit-mask-position-x, -webkit-mask-position-y,
-webkit-mask-origin, -webkit-mask-clip.
> +Removing -webkit-mask removes -webkit-mask-image, -webkit-mask-repeat-x,
-webkit-mask-repeat-y, -webkit-mask-attachment, -webkit-mask-position-x,
-webkit-mask-position-y, -webkit-mask-origin, -webkit-mask-clip.
Ditto.
> Index: LayoutTests/platform/mac/fast/inspector/style-expected.txt
> ===================================================================
> --- LayoutTests/platform/mac/fast/inspector/style-expected.txt
(revision 47879)
> +++ LayoutTests/platform/mac/fast/inspector/style-expected.txt
(working copy)
> @@ -9,46 +9,49 @@ layer at (0,0) size 800x600
> RenderBlock {DIV} at (24,42) size 736x28 [color=#FFFFFF]
[bgcolor=#800080]
> RenderText {#text} at (0,0) size 50x28
> text run at (0,0) width 50: "Test"
> - RenderBlock (anonymous) at (0,94) size 784x252
> + RenderBlock (anonymous) at (0,94) size 784x270
> RenderText {#text} at (0,0) size 388x18
> text run at (0,0) width 388: "background-image: initial (original
property was background)"
> RenderBR {BR} at (388,14) size 0x0
> - RenderText {#text} at (0,18) size 388x18
> - text run at (0,18) width 388: "background-repeat: initial
(original property was background)"
> - RenderBR {BR} at (388,32) size 0x0
> - RenderText {#text} at (0,36) size 418x18
> - text run at (0,36) width 418: "background-attachment: initial
(original property was background)"
> - RenderBR {BR} at (418,50) size 0x0
> - RenderText {#text} at (0,54) size 413x18
> - text run at (0,54) width 413: "background-position-x: initial
(original property was background)"
> - RenderBR {BR} at (413,68) size 0x0
> + RenderText {#text} at (0,18) size 401x18
> + text run at (0,18) width 401: "background-repeat-x: initial
(original property was background)"
> + RenderBR {BR} at (401,32) size 0x0
> + RenderText {#text} at (0,36) size 401x18
> + text run at (0,36) width 401: "background-repeat-y: initial
(original property was background)"
> + RenderBR {BR} at (401,50) size 0x0
> + RenderText {#text} at (0,54) size 418x18
> + text run at (0,54) width 418: "background-attachment: initial
(original property was background)"
> + RenderBR {BR} at (418,68) size 0x0
> RenderText {#text} at (0,72) size 413x18
> - text run at (0,72) width 413: "background-position-y: initial
(original property was background)"
> + text run at (0,72) width 413: "background-position-x: initial
(original property was background)"
> RenderBR {BR} at (413,86) size 0x0
> - RenderText {#text} at (0,90) size 387x18
> - text run at (0,90) width 387: "background-origin: initial
(original property was background)"
> - RenderBR {BR} at (387,104) size 0x0
> - RenderText {#text} at (0,108) size 373x18
> - text run at (0,108) width 373: "background-clip: initial (original
property was background)"
> - RenderBR {BR} at (373,122) size 0x0
> - RenderText {#text} at (0,126) size 387x18
> - text run at (0,126) width 387: "background-color: purple (original
property was background)"
> - RenderBR {BR} at (387,140) size 0x0
> - RenderText {#text} at (0,144) size 300x18
> - text run at (0,144) width 300: "margin-top: 1em (original property
was margin)"
> - RenderBR {BR} at (300,158) size 0x0
> - RenderText {#text} at (0,162) size 510x18
> - text run at (0,162) width 510: "margin-right: 1em (original
property was margin and property was implicitly set.)"
> - RenderBR {BR} at (510,176) size 0x0
> - RenderText {#text} at (0,180) size 525x18
> - text run at (0,180) width 525: "margin-bottom: 1em (original
property was margin and property was implicitly set.)"
> - RenderBR {BR} at (525,194) size 0x0
> - RenderText {#text} at (0,198) size 501x18
> - text run at (0,198) width 501: "margin-left: 1em (original
property was margin and property was implicitly set.)"
> - RenderBR {BR} at (501,212) size 0x0
> - RenderText {#text} at (0,216) size 75x18
> - text run at (0,216) width 75: "color: white"
> - RenderBR {BR} at (75,230) size 0x0
> - RenderText {#text} at (0,234) size 362x18
> - text run at (0,234) width 362: "font: normal normal normal
24px/normal 'Lucida Grande'"
> - RenderBR {BR} at (362,248) size 0x0
> + RenderText {#text} at (0,90) size 413x18
> + text run at (0,90) width 413: "background-position-y: initial
(original property was background)"
> + RenderBR {BR} at (413,104) size 0x0
> + RenderText {#text} at (0,108) size 387x18
> + text run at (0,108) width 387: "background-origin: initial
(original property was background)"
> + RenderBR {BR} at (387,122) size 0x0
> + RenderText {#text} at (0,126) size 373x18
> + text run at (0,126) width 373: "background-clip: initial (original
property was background)"
> + RenderBR {BR} at (373,140) size 0x0
> + RenderText {#text} at (0,144) size 387x18
> + text run at (0,144) width 387: "background-color: purple (original
property was background)"
> + RenderBR {BR} at (387,158) size 0x0
> + RenderText {#text} at (0,162) size 300x18
> + text run at (0,162) width 300: "margin-top: 1em (original property
was margin)"
> + RenderBR {BR} at (300,176) size 0x0
> + RenderText {#text} at (0,180) size 510x18
> + text run at (0,180) width 510: "margin-right: 1em (original
property was margin and property was implicitly set.)"
> + RenderBR {BR} at (510,194) size 0x0
> + RenderText {#text} at (0,198) size 525x18
> + text run at (0,198) width 525: "margin-bottom: 1em (original
property was margin and property was implicitly set.)"
> + RenderBR {BR} at (525,212) size 0x0
> + RenderText {#text} at (0,216) size 501x18
> + text run at (0,216) width 501: "margin-left: 1em (original
property was margin and property was implicitly set.)"
> + RenderBR {BR} at (501,230) size 0x0
> + RenderText {#text} at (0,234) size 75x18
> + text run at (0,234) width 75: "color: white"
> + RenderBR {BR} at (75,248) size 0x0
> + RenderText {#text} at (0,252) size 362x18
> + text run at (0,252) width 362: "font: normal normal normal
24px/normal 'Lucida Grande'"
> + RenderBR {BR} at (362,266) size 0x0
Can this be a dumpAsText() test? The output seems to indicate it should be.
I'd like to see even more tests of the parser. I'm not sure all the edge cases
are covered. Is ever branch of every if statement tested?
r=me as-is
More information about the webkit-reviews
mailing list