548375 - Implement background-repeat as a keyword pair as well as just a single keyword

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jeff Walden [:Waldo]]

CSS3 background-repeat has been decomposed into a pair of values, one for each axis (backward-compatible single-value versions can be mapped onto specific pairs).  Examples:

  repeat     ==   repeat repeat
  repeat-x   ==   repeat no-repeat
  repeat-y   ==   no-repeat repeat

The first keyword applies horizontally, the second vertically.

This is probably fairly simple to implement, although for backwards compatibility we probably don't want computed style to expose keyword pairs unless necessary, using the single-keyword values whenever possible.

Comment 1

[Swati Tiwari]

I would like to work on this one. I have been looking at the files in the layout/style directory and should have something substantial like a primary patch in a few days.

Comment 2

[Zack Weinberg (:zwol)]

(In reply to comment #1)
> I would like to work on this one. I have been looking at the files in the
> layout/style directory and should have something substantial like a primary
> patch in a few days.

Cool!  You can email me ([e-mail]) or find me on irc.mozilla.org (zwol, usually in #developers) if you need help.

Comment 3

[Swati Tiwari]

Attachment: nsCSSParser.cpp changes

In this patch, I have added a function called ParseBackgroundRepeat. This is because, background-repeat is no longer only a single valued property. 
The main idea is that if a single value is found by the scanner, then do nothing, send the value as it is. However, if two values are found, for example "repeat no-repeat", then compress them into a single CSS2 value. Is this the right way to go about it?

Comment 4

[Swati Tiwari]

So compress,
"repeat repeat" to "repeat"
"repeat no-repeat" to "repeat-x"
"no-repeat repeat" to "repeat-y"
"no-repeat no-repeat" to "no-repeat"

Comment 5

[Jeff Walden [:Waldo]]

This might be reasonable, except that part of the reason CSS3 expands background-repeat into a pair of keywords is that new keywords are added, and you can compose them in further ways -- 'space round', for example.

http://dev.w3.org/csswg/css3-background/#the-background-repeat

The property's value storage really should be converted into something pair-like (although it's possible the best way to do that is as the upper and lower bits of a single 32-bit integer).  Enumerating all possible pairs to assign to a single value isn't really the proper solution, going forward, and it seems reasonable to be forward-thinking when making changes here.

Comment 6

[Swati Tiwari]

(In reply to comment #5)
> This might be reasonable, except that part of the reason CSS3 expands
> background-repeat into a pair of keywords is that new keywords are added, and
> you can compose them in further ways -- 'space round', for example.
> 
> http://dev.w3.org/csswg/css3-background/#the-background-repeat
> 
> The property's value storage really should be converted into something
> pair-like (although it's possible the best way to do that is as the upper and
> lower bits of a single 32-bit integer).  Enumerating all possible pairs to
> assign to a single value isn't really the proper solution, going forward, and
> it seems reasonable to be forward-thinking when making changes here.

forward-thinking sounds reasonable - Out of town this week, can I take another shot next week? (May have some questions.)

Thanks,

Comment 7

[Zack Weinberg (:zwol)]

Comment on attachment 445192 [details] [diff] [review]
nsCSSParser.cpp changes

You get an r- because this is clearly not done and I don't want anyone
to think otherwise, but this is not a penalty or anything like that.
You're on the right track here; you made the proper modifications to
the high-level parser, and your low-level code is basically ok.  The
big things that appear to be missing are syntax error handling and
test cases.  Start with the test cases by adding stuff to the
background-repeat block in layout/style/test/property_database.js;
try to cover as many possibilities as you can.  You'll also need
to write reftests to cover the new behavior (basically, verify that
'no-repeat repeat' is the same as 'repeat-y', etc).

I'm now going to nitpick your coding style, because you should get in
the habit of coding to the local style as quickly as possible, and it
makes it easier to review a patch for content when it doesn't have a
bunch of style problems.

>+
>+//***************************************************************************
>+//***************************************************************************
>+//Creating a patch for the CSS background-repeat bug
>+  PRBool ParseBackgroundRepeat();
>+  PRBool ParseXYDirection(nsCSSValue &aOut, PRBool aAcceptsInherit);
>+
>+//***************************************************************************
>+//***************************************************************************

Don't introduce horizontal lines where there weren't any before, and
don't talk about the patch itself in comments.  Here you should just
add the new functions and that's it.

>@@ -4413,16 +4422,17 @@ CSSParserImpl::ParseVariant(nsCSSValue& 
> 
>   if (!GetToken(PR_TRUE)) {
>     return PR_FALSE;
>   }
>   nsCSSToken* tk = &mToken;
>   if (((aVariantMask & (VARIANT_AHK | VARIANT_NORMAL | VARIANT_NONE)) != 0) &&
>       (eCSSToken_Ident == tk->mType)) {
>     nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(tk->mIdent);
>+
>     if (eCSSKeyword_UNKNOWN < keyword) { // known keyword
>       if ((aVariantMask & VARIANT_AUTO) != 0) {
>         if (eCSSKeyword_auto == keyword) {
>           aValue.SetAutoValue();
>           return PR_TRUE;
>         }
>       }
>       if ((aVariantMask & VARIANT_INHERIT) != 0) {

Don't make whitespace changes when you're not changing anything else in
the same area -- it's more stuff to read past to get to the meat of the
change.


>@@ -5215,28 +5225,28 @@ static const nsCSSProperty kOutlineRadiu
>   eCSSProperty__moz_outline_radius_bottomRight,
>   eCSSProperty__moz_outline_radius_bottomLeft
> };
> 
> PRBool
> CSSParserImpl::ParseProperty(nsCSSProperty aPropID)
> {
>   NS_ASSERTION(aPropID < eCSSProperty_COUNT, "index out of range");
>-
>   switch (aPropID) {  // handle shorthand or multiple properties
>   case eCSSProperty_background:

Here you are making substantive changes in the same diff hunk, but I
would still have left this blank line alone.

>   case eCSSProperty__moz_background_origin:
>+    return ParseBackgroundList(aPropID);
>   case eCSSProperty_background_repeat:
>-    return ParseBackgroundList(aPropID);
>+    return ParseBackgroundRepeat();

This is as it should be.

>@@ -5473,16 +5483,22 @@ CSSParserImpl::ParseProperty(nsCSSProper
> #define BG_CENTER  NS_STYLE_BG_POSITION_CENTER
> #define BG_TOP     NS_STYLE_BG_POSITION_TOP
> #define BG_BOTTOM  NS_STYLE_BG_POSITION_BOTTOM
> #define BG_LEFT    NS_STYLE_BG_POSITION_LEFT
> #define BG_RIGHT   NS_STYLE_BG_POSITION_RIGHT
> #define BG_CTB    (BG_CENTER | BG_TOP | BG_BOTTOM)
> #define BG_CLR    (BG_CENTER | BG_LEFT | BG_RIGHT)
> 
>+// Bits used in determining which background repeat info we have
>+#define BG_REPEATOFF  NS_STYLE_BG_REPEAT_OFF
>+#define BG_REPEATX    NS_STYLE_BG_REPEAT_X
>+#define BG_REPEATY    NS_STYLE_BG_REPEAT_Y
>+#define BG_REPEAT     NS_STYLE_BG_REPEAT_XY
>+

And so is this.

>-  case eCSSProperty_background_repeat:
>+  //case eCSSProperty_background_repeat:
>     // Used only internally.
>-    return ParseVariant(aValue, VARIANT_HK,
>-                        nsCSSProps::kBackgroundRepeatKTable);
>+    //return ParseVariant(aValue, VARIANT_HK,
>+    //                    nsCSSProps::kBackgroundRepeatKTable);

Never comment out code in a patch; delete it instead.

>+static nsCSSValue
>+XYDirectionMaskToCSSValue(PRInt32 bitx, PRInt32 bity)
>+{
>+  // If bity is -1, then we just have to return the CSS2 val

This comment is not very helpful.  I would suggest changing it to

  // Only one keyword specified

and moving it to the top of the "if (bity == -1)" block.

>+  PRInt32 val;
>+  if(bity == -1){
>+    if(bitx == BG_REPEAT)
>+      val = NS_STYLE_BG_REPEAT_XY;
>+    else if(bitx == BG_REPEATX)
>+      val = NS_STYLE_BG_REPEAT_X;
>+    else if(bitx == BG_REPEATY)
>+      val = NS_STYLE_BG_REPEAT_Y;
>+    else if(bitx == BG_REPEATOFF)
>+      val = NS_STYLE_BG_REPEAT_OFF;

You should never just fall off the end of an if-else chain like this.
If bitx should always be one of the four values you consider, then put
this at the end:

  else
    NS_RUNTIMEABORT("impossible bitx value");

If it might have other values but those are parse errors, you need to
handle that.

[ You may see NS_NOTREACHED used to tie off if-else chains - that's
  less typing, but should not be used for new code because, like
  NS_ASSERTION, it isn't lethal. ]

Always put a space between a keyword and an open parenthesis or curly
brace, and between a close parenthesis and a curly brace, i.e.

  if (bity == -1) {
    if (bitx == BG_REPEAT)
      etc

Mozilla is not universally consistent about whether single-statement
blocks should have curly braces around them; however, *in this file*
we consistently do put braces.

I would normally advise that this if-else chain be converted to a
switch, because that way you don't have to repeat the 'bitx ==' part
over and over again, but in this case it's better to leave it as is
for consistency with the second if-else chain, which cannot be a switch.

>+  }
>+  else{

The close brace goes on the same line as the else, with a space in
between, i.e. "} else {".  You can leave a blank line immediately
before this if you

>+    // Else, there are two keywords for the x and y directions
>+    if((bitx == BG_REPEAT) && (bity == BG_REPEAT))
>+      val = NS_STYLE_BG_REPEAT_XY;
>+    else if ((bitx == BG_REPEAT) && (bity == BG_REPEATOFF))
>+      val = NS_STYLE_BG_REPEAT_X;
>+    else if ((bitx == BG_REPEATOFF) && (bity == BG_REPEAT))
>+      val = NS_STYLE_BG_REPEAT_Y;
>+    else if ((bitx == BG_REPEATOFF) && (bity == BG_REPEATOFF))
>+      val = NS_STYLE_BG_REPEAT_OFF;
>+  }

Again, don't fall off the end of the if-else chain.  Also, here you
have unnecessary extra parentheses; just write

  if (bitx == BG_REPEAT && bity == BG_REPEAT)

and so on.

>+  //Hopefully in this first iteration, there are no errors.
>+  return nsCSSValue(val, eCSSUnit_Enumerated);
>+}

You gotta handle errors.  This is a parser.  The Web can and will
throw complete nonsense at it.

If you're making a note to yourself that this function is incomplete,
you should write it like this:

 // XXX This function needs to handle errors. (swati 2010-05)

The name and date are important because 'this needs to be fixed'
comments can persist for years or even decades.  If there's a bug
number that you can put in such a comment, do that too.


>+PRBool
>+CSSParserImpl::ParseBackgroundRepeat()
>+{
>+  nsCSSValue value;
>+  nsCSSValueList *head = nsnull, **tail = &head;
>+  for (;;) {
>+    if (!ParseXYDirection(value, !head)) {
>+      break;
>+    }
>+    PRBool inheritOrInitial = value.GetUnit() == eCSSUnit_Inherit ||
>+                              value.GetUnit() == eCSSUnit_Initial;
>+    if (inheritOrInitial && head) {
>+      // inherit and initial are only allowed on their own
>+      break;
>+    }
>+    nsCSSValueList *item = new nsCSSValueList;
>+    if (!item) {
>+      mScanner.SetLowLevelError(NS_ERROR_OUT_OF_MEMORY);
>+      break;
>+    }
>+    item->mValue = value;
>+    *tail = item;
>+    tail = &item->mNext;
>+    if (!inheritOrInitial && ExpectSymbol(',', PR_TRUE)) {
>+      continue;
>+    }
>+    if (!ExpectEndProperty()) {
>+      break;
>+    }
>+    nsCSSValueList **source =
>+      static_cast<nsCSSValueList**>(mTempData.PropertyAt(eCSSProperty_background_repeat));
>+    *source = head;
>+    mTempData.SetPropertyBit(eCSSProperty_background_repeat);
>+    return PR_TRUE;
>+  }
>+  delete head;
>+  return PR_FALSE;
>+}

This looks fine (we need to refactor these loops to reduce code
duplication, but that's not your problem right now) except that the
'inheritOrInitial' checks should be unnecessary, because ParseXYDirection
should fail if it sees 'inherit'/'initial' when aAcceptsInherit is true.
(Which, I note, currently it does not, which is a bug.)

>+/**
>+ * This function is very similar to ParseBoxPositionValues, however, since 
>+ * background-repeat does not take percentage or length values, they
>+ * have been ignored.

Just "... however, background-repeat does not take percentage or
length values."

>+ *
>+ * Inherit is a legal keyword for CSS2

And delete this, any reader will already know that.

>+PRBool
>+CSSParserImpl::ParseXYDirection(nsCSSValue &aOut,
>+                                PRBool aAcceptsInherit)

You can fit both parameter declarations on one 80-column line,
therefore you should.

>+{$
>+  nsCSSValue xValue, yValue;$
>+  $

Here and elsewhere: Take care not to introduce new trailing white
space.  (Deleting existing trailing white space is encouraged, but
only if you can do so without expanding diff hunks too much.)

>+  //Check if inherited

Space between // and the first word of the comment.  Also, I would
word this comment '// Check for inherit or initial'.

>+  if(ParseVariant(aOut, VARIANT_INHERIT, nsnull)){
>+    if(eCSSUnit_Inherit == aOut.GetUnit() ||
>+       eCSSUnit_Initial == aOut.GetUnit()) {
>+        return PR_TRUE;
>+      }
>+  }

Space after 'if' and before '{' again; the close curly brace should
line up with the 'i' in 'if'; and above all, be consistent!

ParseVariant(..., VARIANT_INHERIT, ...) cannot return true without setting
the unit to _Inherit or _Initial, so the nested condition is unnecessary.
Also, this is where you need to be checking aAcceptsInherit.

Also also, I know lots of places in our code base do it, and I know
some people like it, but I really hate the 'constant == variable'
idiom.  Please use 'variable == constant'.

>+  // If it is a single value, then it can be any one of
>+  // "repeat", "no-repeat", "repeat-x" or "repeat-y"
>+  // However if there are two keywords as in CSS3
>+  // Then the legal keywords are "repeat" and "no-repeat"

So, uh, that's not what the current revision says.

http://dev.w3.org/csswg/css3-background/#the-background-repeat

# <repeat-style> =
#   repeat-x | repeat-y | [repeat | space | round | no-repeat]{1,2}

i.e. you need to handle 'space' and 'round' here as well.  Unless
*none* of the code handles 'space' and 'round' in this context - then
that should be addressed in a separate bug.

Also, the comment is not written very well: I'd suggest

 // css2.1 allows only a single keyword:
 //   repeat | no-repeat | repeat-x | repeat-y
 // css3-background expands this to
 //   repeat-x | repeat-y | [repeat | space | round | no-repeat]{1,2}
 // If there are two keywords, the first one gives the horizontal
 // behavior, the second the vertical.

>+    //Look if we have a second keyword according to CSS3

      // Look for a second keyword

>+      //Found a second keyword

Delete this comment, it's just repeating the code

>+      //No repeat-x or repeat-y

And this one

>+      if(bitx == BG_REPEATX || bitx == BG_REPEATY 
>+         || bity == BG_REPEATX || bity == BG_REPEATY ){

Absolutely do not ever put a space to the left of a close parenthesis,
or to the right of an open parenthesis.  Same for square brackets.

>+        //At least one of the two keywords is either repeat-x
>+        //or repeat-y, hence return an error
>+        return PR_FALSE;

          // repeat-x and repeat-y may not appear in a pair of keywords

And do you need to REPORT_UNEXPECTED() something?  Check what the
error console says and see if it's clear enough.  You may need to add
a new diagnostic to dom/locales/en-US/chrome/layout/css.properties
(yes, inconveniently far from the code).

>+      }
>+      
>+      //We cannot combine mask and the bit as both the x and the
>+      //y direction can have the same value 

Huh? And if so, what do you propose to do about it?

>+    }		
>+  }
>+
>+  //bitx cannot be -1, as that would mean, we did not find an
>+  //appropriate keyword
>+  if(bitx == -1){
>+    return PR_FALSE;
>+  }

You should have returned early if the first ParseEnum failed, instead.

>+
>+  //Prepare the out value
>+  aOut = XYDirectionMaskToCSSValue(bitx, bity);
>+  return PR_TRUE;
>+
>+}

Comment repeats the code, delete it; don't leave a blank line before
the end of the function.

Comment 8

[Zack Weinberg (:zwol)]

(In reply to comment #5)
> This might be reasonable, except that part of the reason CSS3 expands
> background-repeat into a pair of keywords is that new keywords are added, and
> you can compose them in further ways -- 'space round', for example.
> 
> http://dev.w3.org/csswg/css3-background/#the-background-repeat
> 
> The property's value storage really should be converted into something
> pair-like (although it's possible the best way to do that is as the upper and
> lower bits of a single 32-bit integer).  Enumerating all possible pairs to
> assign to a single value isn't really the proper solution, going forward, and
> it seems reasonable to be forward-thinking when making changes here.

I concur, and I think it would probably be easiest on the consumers of this data if we just switched it to a nsCSSValuePairList.  That does cascade through style/ and into nsCSSRendering, though.

Swati: If you're willing to try that, I recommend doing it in two pieces: change the specified-value storage to a nsCSSValuePairList, change the computed-value storage to a pair of PRUint8s (in nsStyleBackground::Layer), and fix up all the fallout; then change the parser to accept more than it currently does.

(In reply to comment #6)
> forward-thinking sounds reasonable - Out of town this week, can I take another
> shot next week? (May have some questions.)

There's no deadline here :)  Whenever you get around to it is fine.

Comment 9

[Zack Weinberg (:zwol)]

Adding dbaron to cc: in case he wants to kibitz on CSS parser changes.

Comment 10

[Swati Tiwari]

(In reply to comment #8)
> (In reply to comment #5)
> > This might be reasonable, except that part of the reason CSS3 expands
> > background-repeat into a pair of keywords is that new keywords are added, and
> > you can compose them in further ways -- 'space round', for example.
> > 
> > http://dev.w3.org/csswg/css3-background/#the-background-repeat
> > 
> > The property's value storage really should be converted into something
> > pair-like (although it's possible the best way to do that is as the upper and
> > lower bits of a single 32-bit integer).  Enumerating all possible pairs to
> > assign to a single value isn't really the proper solution, going forward, and
> > it seems reasonable to be forward-thinking when making changes here.
> 
> I concur, and I think it would probably be easiest on the consumers of this
> data if we just switched it to a nsCSSValuePairList.  That does cascade through
> style/ and into nsCSSRendering, though.
> 
> Swati: If you're willing to try that, I recommend doing it in two pieces:
> change the specified-value storage to a nsCSSValuePairList, change the
> computed-value storage to a pair of PRUint8s (in nsStyleBackground::Layer), and
> fix up all the fallout; then change the parser to accept more than it currently
> does.
> 
> (In reply to comment #6)
> > forward-thinking sounds reasonable - Out of town this week, can I take another
> > shot next week? (May have some questions.)
> 
> There's no deadline here :)  Whenever you get around to it is fine.

Thanks, those are great pointers! Will work on it :)

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

A few comments on Zack's comments, actually:

 * we don't want NS_RUNTIMEABORT (which does checks in release builds too); we'd want NS_ABORT_IF_FALSE

 * we don't necessarily need to worry about diagnostics inside the parsing code for property values.  We report at a single point that the property's value was not accepted, and I think that's generally sufficient.

And one other style comment on the patch:  local style uniformly places spaces:
  if (foo) {
rather than:
  if(foo){



We probably do want to switch to storing the property as an nsCSSValuePair in the structures used for specified style, and as two values in the structures used for computed style (nsStyleBackground::Layer), so that we can add support for space and round.

Comment 12

[Melissa Newman]

Attachment: Testcase: background-repeat: repeat norepeat;

Comment 13

[Melissa Newman]

Attachment: Fish image used in testcases

Comment 14

[Melissa Newman]

Attachment: Testcase: background-repeat: norepeat repeat;

Comment 15

[Melissa Newman]

Attachment: Testcase: background-repeat: repeat repeat;

Comment 16

[Melissa Newman]

Attachment: Testcase: background-repeat: space;

Should be converted to background-repeat: space space;

Comment 17

[Melissa Newman]

Attachment: Testcase: background-repeat: space space;

Comment 18

[Melissa Newman]

Attachment: Testcase: background-repeat: space repeat;

Comment 19

[Melissa Newman]

Attachment: Testcase: background-repeat: repeat space;

Comment 20

[Melissa Newman]

Attachment: Testcase: background-repeat: round;

Equal to background-repeat: round round;

Comment 21

[Melissa Newman]

Attachment: Testcase: background-repeat: round round;

Comment 22

[Melissa Newman]

Attachment: Testcase: background-repeat: round repeat;

Comment 23

[Melissa Newman]

Attachment: Testcase: background-repeat: repeat round;

Comment 24

[Melissa Newman]

Attachment: Testcase: background-repeat: no-repeat no-repeat;

Comment 25

[Melissa Newman]

All of the current single values should be converted to double values:

space = space space;
round = round round;
repeat-x = repeat no-repeat;
repeat-y = no-repeat repeat;
repeat = repeat repeat;
no-repeat = no-repeat no-repeat;
default value = repeat repeat;

Pseudo Code for main code:

hParameter = GetParameterBackgroundRepeat(HORIZONTAL);
vParameter = GetParameterBackgroundRepeat(VERTICAL);
ProcessBackgroundRepeat(hParameter, vParameter);

Pseudo Code for GetParamterBackgroundRepeat:

GetParameterBackgroundRepeat(BOOL parameter) {
   if (not exists(backgroundRepeat[1])
      return backgroundRepeat[0];
   else if (HORIZONTAL)
      return backgroundRepeat[0];
   else
      return backgroundRepeat[1];
}

Comment 26

[Melissa Newman]

Attachment: Testcase: background-repeat: round space;

Comment 27

[Melissa Newman]

Attachment: Testcase: background-repeat: space round;

Comment 28

[Melissa Newman]

List of all valid double cases for space, round, repeat, no-repeat:

no-repeat no-repeat
no-repeat repeat
no-repeat round
no-repeat space

repeat no-repeat
repeat repeat
repeat round
repeat space

round no-repeat
round repeat
round round
round space

space no-repeat
space repeat
space round
space space

Comment 31

[Swati Tiwari]

Back!

Comment 32

[Lazar Sumar [:nonsensickle]]

Swati, are you still working on this? If not, I would like to give it a try.

Comment 33

[Lazar Sumar [:nonsensickle]]

(In reply to Melissa Newman from comment #25)
> All of the current single values should be converted to double values:
> 
> space = space space;
...
> round = round round;

(In reply to Melissa Newman from comment #27)
(In reply to Melissa Newman from comment #26)
(In reply to Melissa Newman from comment #23)
(In reply to Melissa Newman from comment #22)
(In reply to Melissa Newman from comment #21)
(In reply to Melissa Newman from comment #20)
(In reply to Melissa Newman from comment #19)
(In reply to Melissa Newman from comment #18)
(In reply to Melissa Newman from comment #17)
(In reply to Melissa Newman from comment #16)

The round and space keywords are not yet implemented. See bug 548372.
Though, reftests should still be written for those cases as well.

Comment 34

[Lazar Sumar [:nonsensickle]]

There is a special case test for the round keyword that I found while working on Bug 548372 that should be included in this fix.

On http://dev.w3.org/csswg/css4-background/#background-size there is a special mention of the round keyword as shown below:

> If ‘background-repeat’ is ‘round’ for one dimension only and if ‘background-size’ 
> is ‘auto’ for the other dimension, then there is a third step: that other 
> dimension is scaled so that the original aspect ratio is restored.

Comment 35

[Lazar Sumar [:nonsensickle]]

Attachment: background-repeat patch 1 reftest v1

Added a simple reftest for the basic case, will add more later.

Invalidated testcases for 'space' and 'round' because of dependence on bug 548372. I'm thinking of making a superbug for tests that involve having pair values of 'space' or 'round' keywords.

What do you think roc?

The other simple case tests are covered by this patch.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Lazar Sumar [:lsumar] from comment #35)
> Invalidated testcases for 'space' and 'round' because of dependence on bug
> 548372. I'm thinking of making a superbug for tests that involve having pair
> values of 'space' or 'round' keywords.

If it simplifies things, you could do 'space' and 'round' in the same patch where you add support for two values. That would probably be a good thing.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 588797 [details] [diff] [review]
background-repeat patch 1 reftest v1

Review of attachment 588797 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/backgrounds/background-repeat-1-ref.html
@@ +38,5 @@
> +{
> +  width: 128px;
> +  height: 128px;
> +  background-image: url(aqua-yellow-32x32.png);
> +  background-repeat: repeat-y; /* no-repeat repeat */

Don't repeat the contents of these rules. Share the width/height/image properties in their own rule body > div > div or something like that. same for the other test.

Comment 38

[Lazar Sumar [:nonsensickle]]

Attachment: background-repeat patch 1 reftest v2

> Don't repeat the contents of these rules. Share the width/height/image 
> properties in their own rule body > div > div or something like that. same for 
> the other test.

Done. And were you referring to the tests for bug 548372 as well?

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Lazar Sumar [:lsumar] from comment #38)
> Done. And were you referring to the tests for bug 548372 as well?

Yes, actually.

Comment 40

[Lazar Sumar [:nonsensickle]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39)
> (In reply to Lazar Sumar [:lsumar] from comment #38)
> > Done. And were you referring to the tests for bug 548372 as well?
> 
> Yes, actually.

done. See bug 549372 comment #13.

Comment 41

[Lazar Sumar [:nonsensickle]]

(In reply to Lazar Sumar [:lsumar] from comment #40)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #39)
> > (In reply to Lazar Sumar [:lsumar] from comment #38)
> > > Done. And were you referring to the tests for bug 548372 as well?
> > 
> > Yes, actually.
> 
> done. See bug 549372 comment #13.

correction. bug 548372 comment #13.

Comment 42

[Lazar Sumar [:nonsensickle]]

Attachment: background-repeat patch 1 implementation v1

This works with the simple reftest on my machine. Will push to try when hg decides to unfreeze.

Comment 43

[Lazar Sumar [:nonsensickle]]

Comment on attachment 589096 [details] [diff] [review]
background-repeat patch 1 implementation v1

Hi Zack, would you be able to push this to Try for me if I don't manage to by the time you get around to the review?

Sorry for the hassle. Thanks.

Comment 44

[Lazar Sumar [:nonsensickle]]

(In reply to Lazar Sumar [:lsumar] from comment #43)
> Comment on attachment 589096 [details] [diff] [review]
> background-repeat patch 1 implementation v1
> 
> Hi Zack, would you be able to push this to Try for me if I don't manage to
> by the time you get around to the review?
> 
> Sorry for the hassle. Thanks.

hg unfroze. Try server link https://tbpl.mozilla.org/?tree=Try&rev=9d1088bfebc7.

Comment 45

[Lazar Sumar [:nonsensickle]]

Comment on attachment 589096 [details] [diff] [review]
background-repeat patch 1 implementation v1

Review of attachment 589096 [details] [diff] [review]:
-----------------------------------------------------------------

Fails tests for inherit keyword.

Comment 46

[Lazar Sumar [:nonsensickle]]

Attachment: background-repeat patch 1 implementation v2

Added inherit/initial keywords parsing to the code.

Try server link https://tbpl.mozilla.org/?tree=Try&rev=e95cf86701b7

Comment 47

[Lazar Sumar [:nonsensickle]]

Comment on attachment 589106 [details] [diff] [review]
background-repeat patch 1 implementation v2

Hi again Zack. Pending a successful try server push (link is in comment #46) can you please review?

Comment 48

[Lazar Sumar [:nonsensickle]]

Attachment: background-repeat patch 1 tests v3

Added change for the background-repeat entry in layout/style/test/property_database.js

This should fix the mochitest4 failure on https://tbpl.mozilla.org/?tree=Try&rev=e95cf86701b7

Comment 49

[Lazar Sumar [:nonsensickle]]

Attachment: background-repeat patch 1 implementation v3

It makes better sense to rename a few of these values to follow local convention.

Comment 50

[Lazar Sumar [:nonsensickle]]

Latest try server push https://tbpl.mozilla.org/?tree=Try&rev=fd0b90f3f4b7 for background-repeat patch 1 implementation v3

Comment 51

[Zack Weinberg (:zwol)]

FYI, I will review this but it may take me a few days to get to it, I've got some deadlines of my own to hit.

Comment 52

[Lazar Sumar [:nonsensickle]]

Comment on attachment 589366 [details] [diff] [review]
background-repeat patch 1 implementation v3

Thanks, no rush.

Comment 53

[Lazar Sumar [:nonsensickle]]

Attachment: background-repeat patch 1 implementation v3

Hmm, I fixed a typo here but it still appears to be failing due to my interpretation of the spec. It appears that it is not enough to leave the computed style in the "repeat repeat" form for its initial value. The fix for this oversight is on its way.

Comment 54

[Lazar Sumar [:nonsensickle]]

Attachment: background-repeat patch 1 implementation v4

Ok, this should fix the problem from comment #53.

Passes local reftests/mochitests, try server link https://tbpl.mozilla.org/?tree=Try&rev=ebac09a9d677

Given it passes the try server tests, Zack could you review this one?

Comment 55

[Lazar Sumar [:nonsensickle]]

Attachment: background-repeat patch 1 tests v4

Added a few tests, throwing the repeat-x and repeat-y keywords into keyword pairs. Now the implementation patch fails those...

The tests are good though.

Comment 56

[Lazar Sumar [:nonsensickle]]

Attachment: background-repeat patch 1 implementation v5

Fixes for the repeat repeat-x, and repeat repeat-y cases. It now fails in the parser as it should.

Comment 57

[Lazar Sumar [:nonsensickle]]

Comment on attachment 589753 [details] [diff] [review]
background-repeat patch 1 implementation v5

Try server link https://tbpl.mozilla.org/?tree=Try&rev=f04b09e1ae27

Comment 58

[Lazar Sumar [:nonsensickle]]

Attachment: background-repeat patch 1 implementation v6

That was silly of me. Here's a better looking try server push and a fixed patch.

https://tbpl.mozilla.org/?tree=Try&rev=34882f870007

Zack, sorry for spamming.

Comment 59

[Zack Weinberg (:zwol)]

Comment on attachment 589752 [details] [diff] [review]
background-repeat patch 1 tests v4

Review of attachment 589752 [details] [diff] [review]:
-----------------------------------------------------------------

> New Binary File: layout/reftests/backgrounds/aqua-yellow-32x32.png

This file is 6KB but can (and should) be recompressed down to 156 *bytes*.  I will attach a better version.  (The 'pngrewrite' and 'pngcrush' utilities are your friends.)  (Is the square in the upper left hand corner meant to be #212121 rather than black?)

::: layout/reftests/backgrounds/background-repeat-1-ref.html
@@ +5,5 @@
> +Original Author: Lazar Sumar <[e-mail]>, 13th Jan 2012, Bug 548372
> +-->
> +<head>
> +  <title>background-repeat single value mapping reference</title>
> +  <style type="text/css">

There is only one kind of <style>, so please remove the redundant 'type="text/css"'.  Also, I think it would be a little easier to read if you indented the style sheet four spaces, but that's not that important.

@@ +19,5 @@
> +#no_repeat
> +{
> +  background-image: url(aqua-yellow-32x32.png);
> +  background-repeat: no-repeat;  /* no-repeat no-repeat */
> +}

You could make this part less repetitive by giving all four inner DIVs a class, say "inner", and factoring out the background-image: declaration.

@@ +38,5 @@
> +}
> +  </style>
> +</head>
> +<body>
> +    <div id="outer">

Invalid HTML: you have four DIVs with the same ID. (It may work, but that's by accident.) Make it a class= instead.  Actually, why do you need the "outer" divs?  It looks to me like you could have 

<body>
  <div id="no_repeat"></div>
  <div id="repeat"></div>
  <div id="repeat_x"></div>
  <div id="repeat_y"></div>
</body>

with

<style>
  div { width: 128px; height: 128px; border: 1px solid black; background-image: url(aqua-yellow-32x32.png); }
  #no_repeat { background-repeat: no-repeat; }
  /* etc */
</style>

::: layout/reftests/backgrounds/background-repeat-1.html
@@ +2,5 @@
> +<html>
> +<!--
> +spec: dev.w3.org/csswg/css4-background/#the-background-repeat
> +Original Author: Lazar Sumar <[e-mail]>, 13th Jan 2012, Bug 548372
> +-->

All the above criticisms of background-repeat-1-ref.html also apply to this file.

Otherwise, it looks good to me; r+ with requested changes.  Over to dbaron for official review.

Comment 60

[Zack Weinberg (:zwol)]

Attachment: recompressed aqua-yellow-32x32.png

Please replace the aqua-yellow-32x32.png in attachment 589752 [details] [diff] [review] with this much smaller file.

Comment 61

[Zack Weinberg (:zwol)]

Comment on attachment 590140 [details] [diff] [review]
background-repeat patch 1 implementation v6

Review of attachment 590140 [details] [diff] [review]:
-----------------------------------------------------------------

FYI, I can't officially approve patches to the style system, I can only kibitz.  You need dbaron or bz for an official review.

Please make sure to replace the tryserver switches in the patch header with a real comment for the hg log, before committing or marking checkin-needed.

You have a whole bunch of machinery in here devoted to preserving the difference between 'background-repeat: repeat-x' and 'background-repeat: repeat no-repeat' on a roundtrip (the isShorthand stuff).  I don't think that's necessary.  It should be okay to just serialize to the compact form whenever possible, and to the two-keyword form when that's not possible (so 'repeat no-repeat' would become 'repeat-x' when serialized).  This is what we do for other properties whose value syntax has become more general.  (If dbaron contradicts me, though, do what he says.)

::: layout/style/nsCSSParser.cpp
@@ +5981,1 @@
>            NS_NOTREACHED("should be able to parse");

This NS_NOTREACHED is no longer correct -- make sure it shows up in the debugging spew for the property_database.js tests, and then remove it.

@@ +6138,5 @@
> +CSSParserImpl::ParseBackgroundRepeatValues(nsCSSValuePair& aValue) {
> +  nsCSSValue& xValue = aValue.mXValue;
> +  nsCSSValue& yValue = aValue.mYValue;
> +  bool isOk = ParseEnum(xValue, nsCSSProps::kBackgroundRepeatKTable);
> +  

Why isn't there a REPORT_UNEXPECTED in this function? Does the user actually get an error console message on a syntactically invalid background-repeat: value?  (It looks like you do *detect* errors fine, I'm just concerned that they may not be *reported*.)

I'm not a fan of the 'isOk' construction, I would prefer early 'return false'.

::: layout/style/nsComputedDOMStyle.cpp
@@ +1700,5 @@
>  
>  nsIDOMCSSValue*
>  nsComputedDOMStyle::DoGetBackgroundRepeat()
>  {
> +  // XXX Should probably modify XXX Lazar Sumar <[e-mail]>, 17th Jan 2012

Should probably modify how? And why didn't you?

::: layout/style/nsRuleNode.cpp
@@ +4819,5 @@
> +                  aSpecifiedValue->mYValue.GetUnit() == eCSSUnit_Null),
> +                 "Invalid unit");
> +    
> +    
> +    

Remove two of these blank lines, please.

Comment 62

[Lazar Sumar [:nonsensickle]]

Attachment: background-repeat patch 1 tests v4

(In reply to Zack Weinberg (:zwol) from comment #59)

> > New Binary File: layout/reftests/backgrounds/aqua-yellow-32x32.png
> 
> This file is 6KB but can (and should) be recompressed down to 156 *bytes*. 
> I will attach a better version.  (The 'pngrewrite' and 'pngcrush' utilities
> are your friends.)  (Is the square in the upper left hand corner meant to be
> #212121 rather than black?)

Done. Thanks for the tip.

> There is only one kind of <style>, so please remove the redundant
> 'type="text/css"'.  Also, I think it would be a little easier to read if you
> indented the style sheet four spaces, but that's not that important.

Both were done.

> Invalid HTML: you have four DIVs with the same ID. (It may work, but that's
> by accident.) Make it a class= instead.  Actually, why do you need the
> "outer" divs?  It looks to me like you could have 
> 
> <body>
>   <div id="no_repeat"></div>
>   ...

Good point. I did it your way.

> All the above criticisms of background-repeat-1-ref.html also apply to this
> file.

I will also propagate to bug 548372 tests.

> Otherwise, it looks good to me; r+ with requested changes.  Over to dbaron
> for official review.

Thanks.

Comment 63

[Lazar Sumar [:nonsensickle]]

Attachment: background-repeat patch 1 implementation v7

(In reply to Zack Weinberg (:zwol) from comment #61)
> FYI, I can't officially approve patches to the style system, I can only
> kibitz.  You need dbaron or bz for an official review.

I was aware of this. Thanks again.
 
> Please make sure to replace the tryserver switches in the patch header with
> a real comment for the hg log, before committing or marking checkin-needed.

At the moment I am still making changes. I will when the patch is reviewed and ready.

> You have a whole bunch of machinery in here devoted to preserving the
> difference between 'background-repeat: repeat-x' and 'background-repeat:
> repeat no-repeat' on a roundtrip (the isShorthand stuff).  I don't think
> that's necessary.  It should be okay to just serialize to the compact form
> whenever possible, and to the two-keyword form when that's not possible (so
> 'repeat no-repeat' would become 'repeat-x' when serialized).  This is what
> we do for other properties whose value syntax has become more general.  (If
> dbaron contradicts me, though, do what he says.)

No, I think both you and dbaron agree on this one. When we talked over irc he proposed the same. However, I was interpreting his remarks from bug 522607 comment #5 quite literally and have propagated them to this bug. I'm happy to change it though.

> ::: layout/style/nsCSSParser.cpp
> @@ +5981,1 @@
> >            NS_NOTREACHED("should be able to parse");
> 
> This NS_NOTREACHED is no longer correct -- make sure it shows up in the
> debugging spew for the property_database.js tests, and then remove it.

There wasn't any spew when I ran mochitests but I removed it anyway. I agree that the logic doesn't justify it any more.

> @@ +6138,5 @@
> > +CSSParserImpl::ParseBackgroundRepeatValues(nsCSSValuePair& aValue) {
> > +  nsCSSValue& xValue = aValue.mXValue;
> > +  nsCSSValue& yValue = aValue.mYValue;
> > +  bool isOk = ParseEnum(xValue, nsCSSProps::kBackgroundRepeatKTable);
> > +  
> 
> Why isn't there a REPORT_UNEXPECTED in this function? Does the user actually
> get an error console message on a syntactically invalid background-repeat:
> value?  (It looks like you do *detect* errors fine, I'm just concerned that
> they may not be *reported*.)

They are reported higher up in the caller. A warning that the background-repeat property couldn't be parsed is given but it doesn't say why. I didn't know REPORT_UNEXPECTED existed until just now and am happy to add it. However, it is not present in any of the other specialized background property parsing functions so I will wait for dbaron's verdict.
 
> I'm not a fan of the 'isOk' construction, I would prefer early 'return
> false'.

It also doesn't fit the local style so I've rewritten it in this patch.

> ::: layout/style/nsComputedDOMStyle.cpp
> @@ +1700,5 @@
> >  
> >  nsIDOMCSSValue*
> >  nsComputedDOMStyle::DoGetBackgroundRepeat()
> >  {
> > +  // XXX Should probably modify XXX Lazar Sumar <[e-mail]>, 17th Jan 2012
> 
> Should probably modify how? And why didn't you?

Whoops. That comment is a leftover from an earlier stage in writing. Removed.

> ::: layout/style/nsRuleNode.cpp
> @@ +4819,5 @@
> > +                  aSpecifiedValue->mYValue.GetUnit() == eCSSUnit_Null),
> > +                 "Invalid unit");
> > +    
> > +    
> > +    
> 
> Remove two of these blank lines, please.

Done.

Comment 64

[Zack Weinberg (:zwol)]

(In reply to Lazar Sumar [:lsumar] from comment #63)
> No, I think both you and dbaron agree on this one. When we talked over irc
> he proposed the same. However, I was interpreting his remarks from bug
> 522607 comment #5 quite literally and have propagated them to this bug. I'm
> happy to change it though.

I am not sure what he wants there, I confess.

> > This NS_NOTREACHED is no longer correct -- make sure it shows up in the
> > debugging spew for the property_database.js tests, and then remove it.
> 
> There wasn't any spew when I ran mochitests but I removed it anyway. I agree
> that the logic doesn't justify it any more.

If there was no spew, that means your tests are inadequate; you need to add tests that hit every syntax-error path in this code.

> They are reported higher up in the caller. A warning that the
> background-repeat property couldn't be parsed is given but it doesn't say
> why. I didn't know REPORT_UNEXPECTED existed until just now and am happy to
> add it. However, it is not present in any of the other specialized
> background property parsing functions so I will wait for dbaron's verdict.

I'll withdraw my objection then. At present it _is_ a general rule that we don't give terribly specific diagnostics for value parsing errors. I think that will need to change as values continue to get more complicated, but there's no reason to gate this on that.

Comment 65

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 591702 [details] [diff] [review]
background-repeat patch 1 tests v4

>diff --git a/layout/style/test/property_database.js b/layout/style/test/property_database.js

> 		initial_values: [ "repeat" ],
> 		other_values: [ "repeat-x", "repeat-y", "no-repeat",
> 			"repeat-x, repeat-x",
> 			"repeat, no-repeat",
> 			"repeat-y, no-repeat, repeat-y",
>-			"repeat, repeat, repeat"
>+			"repeat, repeat, repeat",
>+			"repeat repeat",
>+			"repeat no-repeat",
>+			"no-repeat repeat",
>+			"no-repeat no-repeat",
>+			"repeat repeat, repeat repeat",
> 		],
>-		invalid_values: [ "repeat repeat" ]
>+		invalid_values: [ "repeat repeat repeat", "repeat repeat-x",
>+		                  "repeat-x repeat", 
>+		                  "repeat-y repeat" ]

So this seems wrong.  In particular, "repeat repeat" means the same thing as "repeat", and it should compute to the same thing (probably "repeat" for compatibility -- always prefer the older syntax when serializing), which means it should be in the initial_values line rather than the other_values line.

At least, I think that's the case.  But you should probably follow any responses to http://lists.w3.org/Archives/Public/www-style/2012Feb/1094.html to see if others agree or disagree with me.

Comment 66

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Lazar Sumar [:lsumar] from comment #63)
> > You have a whole bunch of machinery in here devoted to preserving the
> > difference between 'background-repeat: repeat-x' and 'background-repeat:
> > repeat no-repeat' on a roundtrip (the isShorthand stuff).  I don't think
> > that's necessary.  It should be okay to just serialize to the compact form
> > whenever possible, and to the two-keyword form when that's not possible (so
> > 'repeat no-repeat' would become 'repeat-x' when serialized).  This is what
> > we do for other properties whose value syntax has become more general.  (If
> > dbaron contradicts me, though, do what he says.)
> 
> No, I think both you and dbaron agree on this one. When we talked over irc
> he proposed the same. However, I was interpreting his remarks from bug
> 522607 comment #5 quite literally and have propagated them to this bug. I'm
> happy to change it though.

Yeah, I agree that this should probably be changed.  I guess I think there's a difference between an expansion/contraction vs. a complete reformulation in terms of calc().

Comment 67

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 591702 [details] [diff] [review]
background-repeat patch 1 tests v4

>Bug 548375 - Added reftests

The commit message should be a little more descriptive:  something like "Add tests for background-repeat taking two values (css3-background).)

>+spec: dev.w3.org/csswg/css4-background/#the-background-repeat

Cite css3-background, not the totally-unreviewed css4-background draft.  And put http: at the start so people can follow the link.

>+Original Author: Lazar Sumar <[e-mail]>, 13th Jan 2012, Bug 548372

This is in the version control system; it shouldn't be in comments.


>+spec: dev.w3.org/csswg/css4-background/#the-background-repeat
>+Original Author: Lazar Sumar <[e-mail]>, 13th Jan 2012, Bug 548372

Same in the test.


r=dbaron with that fixed plus comment 65 / comment 66 (really the same thing) addressed

Comment 68

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 591867 [details] [diff] [review]
background-repeat patch 1 implementation v7

Don't change nsImageLoader.cpp -- it's a change inside commented out
code that's pretty far from compiling.  Just leave that as-is.

In CSSParserImpl::ParseBackgroundItem, call your "tempRep" variable
"scratch" instead to match the code just below it.

At the same point, you should leave the NS_NOTREACHED in.
ParseBackgroundRepeatValues should only consume what's legal.  (Need to
verify that that's actually true.)

>+CSSParserImpl::ParseBackgroundRepeatValues(nsCSSValuePair& aValue) {

{ on its own line

Also, I think this function would be better if you made a second
constant table (kBackgroundRepeatParkKTable) that didn't have repeat-x
and repeat-y in it.  You could then use that table for the second call
and avoid the conditions in the return statement.  (It would also
improve correctness if future extensions to the background shorthand
allowed a property with arbitrary identifiers.)

nsRuleNode.cpp:

>+      switch (value) {
>+        case NS_STYLE_BG_REPEAT_NO_REPEAT:
>+        case NS_STYLE_BG_REPEAT_REPEAT:
>+          aComputedValue.mYRepeat = value;
>+          break;
>+        default:
>+          NS_NOTREACHED("Unexpected value");
>+          break;
>+      }

This switch() could just be an NS_ASSERTION followed by an assignment.

nsStyleStruct.cpp:

>+nsStyleBackground::Repeat::SetInitialValues() {

{ on its own line



Per the previous comments, please remove
nsStyleBackground::Repeat::mIsShorthand.

I think the way you did the specified value storage (with the Y value
optionally null) is fine, though it would have been fine the other way
too, I think.  So I'd say leave it as-is.  (Changing would require
touching CSSParserImpl::ParseBackgroundRepeatValues and Decaration.cpp
and the nsRuleNode.cpp code.)


r=dbaron with those things fixed, though I'd like to look at it again
after the mIsShorthand removal, so marking review-

Comment 69

[Lazar Sumar [:nonsensickle]]

Attachment: background-repeat patch 1 tests v5

(In reply to David Baron [:dbaron] from comment #65)
> Comment on attachment 591702 [details] [diff] [review]
> background-repeat patch 1 tests v4
> 
> >diff --git a/layout/style/test/property_database.js b/layout/style/test/property_database.js
> 
> > 		initial_values: [ "repeat" ],
> > 		other_values: [ "repeat-x", "repeat-y", "no-repeat",
> > 			"repeat-x, repeat-x",
> > 			"repeat, no-repeat",
> > 			"repeat-y, no-repeat, repeat-y",
> >-			"repeat, repeat, repeat"
> >+			"repeat, repeat, repeat",
> >+			"repeat repeat",
> >+			"repeat no-repeat",
> >+			"no-repeat repeat",
> >+			"no-repeat no-repeat",
> >+			"repeat repeat, repeat repeat",
> > 		],
> >-		invalid_values: [ "repeat repeat" ]
> >+		invalid_values: [ "repeat repeat repeat", "repeat repeat-x",
> >+		                  "repeat-x repeat", 
> >+		                  "repeat-y repeat" ]
> 
> So this seems wrong.  In particular, "repeat repeat" means the same thing as
> "repeat", and it should compute to the same thing (probably "repeat" for
> compatibility -- always prefer the older syntax when serializing), which
> means it should be in the initial_values line rather than the other_values
> line.
> 
> At least, I think that's the case.  But you should probably follow any
> responses to http://lists.w3.org/Archives/Public/www-style/2012Feb/1094.html
> to see if others agree or disagree with me.

This test is tied to the interpretation of the expanded form vs the contraction as said in the discussion of the mIsShorthand property in comment #61, comment #63 and comment #66. 

I concede that dbaron has a point that "repeat repeat" should, when tested in JavaScript with a simple == against "repeat", probably be equal, and that the convention warrants this type of implementation. 

Fixed.

Comment 70

[Lazar Sumar [:nonsensickle]]

Attachment: background-repeat patch 1 implementation v8

(In reply to David Baron [:dbaron] from comment #67)
> Comment on attachment 591702 [details] [diff] [review]
> background-repeat patch 1 tests v4
> 
> >Bug 548375 - Added reftests
> 
> The commit message should be a little more descriptive:  something like "Add
> tests for background-repeat taking two values (css3-background).)
> 
> >+spec: dev.w3.org/csswg/css4-background/#the-background-repeat
> 
> Cite css3-background, not the totally-unreviewed css4-background draft.  And
> put http: at the start so people can follow the link.
> 
> >+Original Author: Lazar Sumar <[e-mail]>, 13th Jan 2012, Bug 548372
> 
> This is in the version control system; it shouldn't be in comments.
> 
> 
> >+spec: dev.w3.org/csswg/css4-background/#the-background-repeat
> >+Original Author: Lazar Sumar <[e-mail]>, 13th Jan 2012, Bug 548372
> 
> Same in the test.
> 
> 
> r=dbaron with that fixed plus comment 65 / comment 66 (really the same
> thing) addressed

(In reply to David Baron [:dbaron] from comment #66)
> (In reply to Lazar Sumar [:lsumar] from comment #63)
> > > You have a whole bunch of machinery in here devoted to preserving the
> > > difference between 'background-repeat: repeat-x' and 'background-repeat:
> > > repeat no-repeat' on a roundtrip (the isShorthand stuff).  I don't think
> > > that's necessary.  It should be okay to just serialize to the compact form
> > > whenever possible, and to the two-keyword form when that's not possible (so
> > > 'repeat no-repeat' would become 'repeat-x' when serialized).  This is what
> > > we do for other properties whose value syntax has become more general.  (If
> > > dbaron contradicts me, though, do what he says.)
> > 
> > No, I think both you and dbaron agree on this one. When we talked over irc
> > he proposed the same. However, I was interpreting his remarks from bug
> > 522607 comment #5 quite literally and have propagated them to this bug. I'm
> > happy to change it though.
> 
> Yeah, I agree that this should probably be changed.  I guess I think there's
> a difference between an expansion/contraction vs. a complete reformulation
> in terms of calc().

Noted. Reworked as explained below.

(In reply to David Baron [:dbaron] from comment #67)
> Comment on attachment 591702 [details] [diff] [review]
> background-repeat patch 1 tests v4
> 
> >Bug 548375 - Added reftests
> The commit message should be a little more descriptive:  something like "Add
> tests for background-repeat taking two values (css3-background).)

I missed this when responding in comment #69. Done.
 
> >+spec: dev.w3.org/csswg/css4-background/#the-background-repeat
> 
> Cite css3-background, not the totally-unreviewed css4-background draft.  And
> put http: at the start so people can follow the link.

Done.

> >+Original Author: Lazar Sumar <[e-mail]>, 13th Jan 2012, Bug 548372
> This is in the version control system; it shouldn't be in comments.

Apologies. This is the 3rd time you've mentioned this on my patches but all were written before the first review. Done and noted.
 
> >+spec: dev.w3.org/csswg/css4-background/#the-background-repeat
> >+Original Author: Lazar Sumar <[e-mail]>, 13th Jan 2012, Bug 548372
> 
> Same in the test.

Done.

> r=dbaron with that fixed plus comment 65 / comment 66 (really the same
> thing) addressed

Done.

(In reply to David Baron [:dbaron] from comment #66)
> (In reply to Lazar Sumar [:lsumar] from comment #63)
> > > You have a whole bunch of machinery in here devoted to preserving the
> > > difference between 'background-repeat: repeat-x' and 'background-repeat:
> > > repeat no-repeat' on a roundtrip (the isShorthand stuff).  I don't think
> > > that's necessary.  It should be okay to just serialize to the compact form
> > > whenever possible, and to the two-keyword form when that's not possible (so
> > > 'repeat no-repeat' would become 'repeat-x' when serialized).  This is what
> > > we do for other properties whose value syntax has become more general.  (If
> > > dbaron contradicts me, though, do what he says.)
> > 
> > No, I think both you and dbaron agree on this one. When we talked over irc
> > he proposed the same. However, I was interpreting his remarks from bug
> > 522607 comment #5 quite literally and have propagated them to this bug. I'm
> > happy to change it though.
> 
> Yeah, I agree that this should probably be changed.  I guess I think there's
> a difference between an expansion/contraction vs. a complete reformulation
> in terms of calc().

Noted. Done.

(In reply to David Baron [:dbaron] from comment #68)
> Comment on attachment 591867 [details] [diff] [review]
> background-repeat patch 1 implementation v7
> 
> Don't change nsImageLoader.cpp -- it's a change inside commented out
> code that's pretty far from compiling.  Just leave that as-is.

I didn't know this. Won't it get outdated? Regardless, done.

> In CSSParserImpl::ParseBackgroundItem, call your "tempRep" variable
> "scratch" instead to match the code just below it.

Done.

> At the same point, you should leave the NS_NOTREACHED in.
> ParseBackgroundRepeatValues should only consume what's legal.  (Need to
> verify that that's actually true.)

Done.

> >+CSSParserImpl::ParseBackgroundRepeatValues(nsCSSValuePair& aValue) {
> 
> { on its own line

Done.

> Also, I think this function would be better if you made a second
> constant table (kBackgroundRepeatParkKTable) that didn't have repeat-x
> and repeat-y in it.  You could then use that table for the second call
> and avoid the conditions in the return statement.  (It would also
> improve correctness if future extensions to the background shorthand
> allowed a property with arbitrary identifiers.)

I assume that was a typo and you meant "kBackgroundRepeatPartKTable".
Though I dislike copy/paste I can see your point. Done.

> nsRuleNode.cpp:
> 
> >+      switch (value) {
> >+        case NS_STYLE_BG_REPEAT_NO_REPEAT:
> >+        case NS_STYLE_BG_REPEAT_REPEAT:
> >+          aComputedValue.mYRepeat = value;
> >+          break;
> >+        default:
> >+          NS_NOTREACHED("Unexpected value");
> >+          break;
> >+      }
> 
> This switch() could just be an NS_ASSERTION followed by an assignment.

Done.

> nsStyleStruct.cpp:
> 
> >+nsStyleBackground::Repeat::SetInitialValues() {
> 
> { on its own line

This is turning to be a chronic mistake with me... Done.

> Per the previous comments, please remove
> nsStyleBackground::Repeat::mIsShorthand.

Removed.

> I think the way you did the specified value storage (with the Y value
> optionally null) is fine, though it would have been fine the other way
> too, I think.  So I'd say leave it as-is.  (Changing would require
> touching CSSParserImpl::ParseBackgroundRepeatValues and Decaration.cpp
> and the nsRuleNode.cpp code.)

As is.

> r=dbaron with those things fixed, though I'd like to look at it again
> after the mIsShorthand removal, so marking review-

r- in the comment until you r+ what's there now and it passes tryserver tests on https://tbpl.mozilla.org/?tree=Try&rev=4a2bf0ac3b91

Comment 71

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 599894 [details] [diff] [review]
background-repeat patch 1 implementation v8

nsCSSParser.cpp:

>+        nsCSSValuePair scratch;
>+        if (!ParseBackgroundRepeatValues(scratch)) {
>+           NS_NOTREACHED("should be able to parse");
>+           return false;
>+         }
>+        aState.mRepeat->mXValue = scratch.mXValue;
>+        aState.mRepeat->mYValue = scratch.mYValue;

You've got an extra space of indentation on the 2 lines inside the {}
and the line with the }.  (With that fixed, they should show up
as not modified at all in the diff.)

nsRuleNode.cpp:

>+    case NS_STYLE_BG_REPEAT_NO_REPEAT:
>+    case NS_STYLE_BG_REPEAT_REPEAT:
>+      aComputedValue.mXRepeat = value;
>+      hasContraction = false;
>+      break;
>+    default:
>+      NS_NOTREACHED("Unexpected value");
>+      break;

I'd just simplify this to
    default:
      aComputedValue.mXRepeat = value;
      hasContraction = false;
      break;

>+    if (hasContraction) {
>+      return;
>+    }

You should still asert that aSpecifiedValue->mYValue.GetUnit() is
eCSSUnit_Null, like you did in the last version of the patch.

r=dbaron with that

Comment 72

[Lazar Sumar [:nonsensickle]]

Attachment: background-repeat patch 1 implementation v9

(In reply to David Baron [:dbaron] from comment #71)
> Comment on attachment 599894 [details] [diff] [review]
> background-repeat patch 1 implementation v8
> 
> nsCSSParser.cpp:
> 
> >+        nsCSSValuePair scratch;
> >+        if (!ParseBackgroundRepeatValues(scratch)) {
> >+           NS_NOTREACHED("should be able to parse");
> >+           return false;
> >+         }
> >+        aState.mRepeat->mXValue = scratch.mXValue;
> >+        aState.mRepeat->mYValue = scratch.mYValue;
> 
> You've got an extra space of indentation on the 2 lines inside the {}
> and the line with the }.  (With that fixed, they should show up
> as not modified at all in the diff.)

Done.

> nsRuleNode.cpp:
> 
> >+    case NS_STYLE_BG_REPEAT_NO_REPEAT:
> >+    case NS_STYLE_BG_REPEAT_REPEAT:
> >+      aComputedValue.mXRepeat = value;
> >+      hasContraction = false;
> >+      break;
> >+    default:
> >+      NS_NOTREACHED("Unexpected value");
> >+      break;
> 
> I'd just simplify this to
>     default:
>       aComputedValue.mXRepeat = value;
>       hasContraction = false;
>       break;

I've done it for both switch statements. However I've added an NS_ASSERTION that the 'value' is correct to your example.

> >+    if (hasContraction) {
> >+      return;
> >+    }
> 
> You should still asert that aSpecifiedValue->mYValue.GetUnit() is
> eCSSUnit_Null, like you did in the last version of the patch.

Done.

> r=dbaron with that

Thanks.

Tryserver link https://tbpl.mozilla.org/?tree=Try&rev=3c87d4ff2e6e

Comment 73

[Lazar Sumar [:nonsensickle]]

Attachment: background-repeat patch 1 implementation v9.1

(In reply to David Baron [:dbaron] from comment #71)
> Comment on attachment 599894 [details] [diff] [review]
> background-repeat patch 1 implementation v8
> > nsRuleNode.cpp:
> > 
> > >+    case NS_STYLE_BG_REPEAT_NO_REPEAT:
> > >+    case NS_STYLE_BG_REPEAT_REPEAT:
> > >+      aComputedValue.mXRepeat = value;
> > >+      hasContraction = false;
> > >+      break;
> > >+    default:
> > >+      NS_NOTREACHED("Unexpected value");
> > >+      break;
> > 
> > I'd just simplify this to
> >     default:
> >       aComputedValue.mXRepeat = value;
> >       hasContraction = false;
> >       break;
> 
> I've done it for both switch statements. However I've added an NS_ASSERTION
> that the 'value' is correct to your example.

Never mind. I'll adhere to your recommendation a little more strictly or it won't look right.

New tryserver link https://tbpl.mozilla.org/?tree=Try&rev=15990a2d6baa, full mozilla-central run.

Comment 74

[Lazar Sumar [:nonsensickle]]

Attachment: background-repeat patch 1 implementation v9.1

Fixing indentation.

Comment 75

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 600254 [details] [diff] [review]
background-repeat patch 1 implementation v9.1

>+  static void ComputeValue(nsStyleContext* aStyleContext,
>+                           const nsCSSValuePairList* aSpecifiedValue,
>+                           nsStyleBackground::Repeat& aComputedValue,
>+                           bool& aCanStoreInRuleTree)
>+  {
>+    NS_ASSERTION(aSpecifiedValue->mXValue.GetUnit() == eCSSUnit_Enumerated ||
>+                 (aSpecifiedValue->mYValue.GetUnit() == eCSSUnit_Enumerated ||
>+                  aSpecifiedValue->mYValue.GetUnit() == eCSSUnit_Null),
>+                 "Invalid unit");

Not sure why you changed this from && to ||.  I'll change it back before landing.

Comment 76

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db6ca99007f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1c6cd6c52b7

Comment 77

[Lazar Sumar [:nonsensickle]]

(In reply to David Baron [:dbaron] from comment #75)
> Comment on attachment 600254 [details] [diff] [review]
> background-repeat patch 1 implementation v9.1
> 
> >+  static void ComputeValue(nsStyleContext* aStyleContext,
> >+                           const nsCSSValuePairList* aSpecifiedValue,
> >+                           nsStyleBackground::Repeat& aComputedValue,
> >+                           bool& aCanStoreInRuleTree)
> >+  {
> >+    NS_ASSERTION(aSpecifiedValue->mXValue.GetUnit() == eCSSUnit_Enumerated ||
> >+                 (aSpecifiedValue->mYValue.GetUnit() == eCSSUnit_Enumerated ||
> >+                  aSpecifiedValue->mYValue.GetUnit() == eCSSUnit_Null),
> >+                 "Invalid unit");
> 
> Not sure why you changed this from && to ||.  I'll change it back before
> landing.

That was a mistake. Thanks.

Comment 78

[Jean-Yves Perrier [:teoli]]

I've updated https://developer.mozilla.org/en/Firefox_13_for_developers#CSS
and 
https://developer.mozilla.org/en/CSS/background-repeat

Comment 79

[Jean-Yves Perrier [:teoli]]

Oups, I somewhat jumped the gun. I'll watch if it is committed to m-c for Fx 13 and sticks. If not, I'll reverse the doc.

Comment 80

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/db6ca99007f5
https://hg.mozilla.org/mozilla-central/rev/c1c6cd6c52b7