Closed Bug 1172012 Opened 4 years ago Closed 4 years ago

Potential copy / paste error in nsCSSValue.cpp

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: Sylvestre, Assigned: ma.steiman, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [good first bug][lang=C++][CID 1221017])

Attachments

(1 file)

https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.cpp?from=%20%20%20%20nsCSSValue.cpp&case=true#1475
coverity says:
copy_paste_error: mXValue in gradient->mBgPos.mXValue looks like a copy-paste error.
Should it say mYValue instead?

We should figure out if it is a false positive or not.
David, do you have to give it a try?
Flags: needinfo?(dbryant)
Component: Layout → CSS Parsing and Computation
Hi Sylvestre, to me it does indeed look like a copy/paste error however are we waiting for David before taking steps?
Assignee: nobody → ma.steiman
Status: NEW → ASSIGNED
Flags: needinfo?(sledru)
David was looking for a good first bug to understand the contribution workflow. This one was interesting. If you want to take it,  go ahead, I can find another for him.
Flags: needinfo?(sledru)
Attachment #8616120 - Flags: review?(cam)
Thanks for your contribution Muhsin!  The code change looks good.  I don't think we have good test coverage for property serialization, but let's add a test for this case.  Can you add something to layout/style/test/test_specified_value_serialization.html that checks we don't serialize the Y component of the background position in a gradient value if only the X component was specified?  So, something like the existing font-variant-alternates test (i.e., setting the style attribute, then looking up the value with getPropertyValue).
Flags: needinfo?(ma.steiman)
(In reply to Cameron McCormack (:heycam) from comment #5)
> Thanks for your contribution Muhsin!  The code change looks good.  I don't
> think we have good test coverage for property serialization, but let's add a
> test for this case.  Can you add something to
> layout/style/test/test_specified_value_serialization.html that checks we
> don't serialize the Y component of the background position in a gradient
> value if only the X component was specified?  So, something like the
> existing font-variant-alternates test (i.e., setting the style attribute,
> then looking up the value with getPropertyValue).

I can certainly take a look however I'm pretty beginner and setting serialization tests in JS is a little beyond my realm of experience. If someone would like to mentor me a bit I would be glad to do it.
Flags: needinfo?(ma.steiman) → needinfo?(cam)
Hey Sylvestre, I'd like to this bug closed out, can you give me some information on this test that was mentioned above?
Flags: needinfo?(sledru)
Hey Muhsin, sorry I didn't get back to you.  I'd be happy to help.  Are you familiar with JS and DOM calls at all?

test_specified_value_serialization.html is a Mochitest, with is basically a scripted HTML file with a few specific functions available for making test assertions.  You can read about Mochitests here:

  https://developer.mozilla.org/en-US/docs/Mochitest

You can run that test from your repository checkout by running:

  ./mach mochitest layout/style/test/test_specified_value_serialization.html

Getting the test running is the first step I'd try.

The one Mochitest function being used here in this test is the is() function:

  https://developer.mozilla.org/en-US/docs/Mochitest#Test_functions

We want to add something to the script -- just before the final elt.setAttribute("style", "") call -- that first sets, say, the background-image property to a radial-gradient() value that only uses an X position.  Then following that, have an is() call that checks that the result of elt.style.getPropertyValue("background-image") is the expected radial-gradient() value, i.e. not one that mistakenly includes a Y position value.

There's some info on the radial-gradient() syntax here, if you're unfamiliar with it:

  https://developer.mozilla.org/en-US/docs/Web/CSS/radial-gradient

We want a value that has, for example, "at 10px" in it (so that's just the X value), rather than "at 10px 20px".
Flags: needinfo?(sledru)
Flags: needinfo?(cam)
Hey Cameron,

When adding:

elt.setAttribute("style","background-image: radial-gradient(ellipse 10px");
is(elt.style.getPropertyValue("background-image","10px"));

It's currently passing 27 and failing 1 in Mochitest with the error " -failed | undefined -got, expected undefined "

I feel like I'm off the mark but I'm a little too new to get on track here. I scripted this is as close as I could to the radial-gradient() and is() documentation but I'm sure I'm not checking correctly here.

Any help would be appreciated, sorry if I'm making this harder than it needs to be!
Flags: needinfo?(cam)
(In reply to Muhsin A. Steiman from comment #9)
> When adding:
> 
> elt.setAttribute("style","background-image: radial-gradient(ellipse 10px");
> is(elt.style.getPropertyValue("background-image","10px"));
> 
> It's currently passing 27 and failing 1 in Mochitest with the error "
> -failed | undefined -got, expected undefined "

Looks like you're passing the "10px" to getPropertyValue, but it should be the second argument to is(), instead.

I think also you'll need to include the "at" in the middle of the radial-gradient() -- that's not optional, AFAIK.

You should also pass a description string as the third argument to is(), describing what it is you are testing.  That will make the test output more understandable.

> Any help would be appreciated, sorry if I'm making this harder than it needs
> to be!

No problem!
Flags: needinfo?(cam)
Cameron, 

After updating the code to: 

elt.setAttribute("style",
                 "background-image: radial-gradient(ellipse at 10px)");
is(elt.style.getPropertyValue("background-image"), "10px",
   "tests for proper background position serialization");

I'm still receiving an error of: 

" failed | tests for proper background position serialization - got , expected 10px "

I made the changes based on your last post yet still no luck ... any suggestions? Thanks!
Flags: needinfo?(cam)
(In reply to Muhsin A. Steiman from comment #11)
> After updating the code to: 
> 
> elt.setAttribute("style",
>                  "background-image: radial-gradient(ellipse at 10px)");
> is(elt.style.getPropertyValue("background-image"), "10px",
>    "tests for proper background position serialization");
> 
> I'm still receiving an error of: 
> 
> " failed | tests for proper background position serialization - got ,
> expected 10px "

I think there are two things.  First, you need to include some colour stops in the radial-gradient() value for it to be valid, for example radial-gradient(ellipse at 10px, red, blue).  Second, the value that you are comparing against, in the is() function call, should be the whole serialized radial-gradient() value.  Once you add the colour values and run the test you should see what value you're getting out (and with your patch applied, that will be the value you want to check for).
Flags: needinfo?(cam)
Cameron,

After making updates I'm receiving: 

failed | tests for proper background position serialization - got radial-gradient(ellipse at 10px 50% , red, blue), expected radial-gradient(ellipse at 10px, red, blue)

My code was updated to: 

elt.setAttribute("style",
                 "background-image: radial-gradient(ellipse at 20px, red, blue)");
is(elt.style.getPropertyValue("background-image"), "radial-gradient(ellipse at 20px, red, blue)",
   "tests for proper background position serialization");

I've also played around with some examples found in DXR with percentages and it keeps failing on me....
Flags: needinfo?(dbryant) → needinfo?(cam)
That test code looks correct to me.  But, I'm afraid I have led you astray!  Looking at the nsCSSValue serialization code, the part where your fix applies is only for when we have a legacy gradient syntax -- so that's something like linear-gradient(left, red, blue).  But also, the part of the parser where we parse the position values in the gradient, CSSParserImpl::ParseBoxPositionValues, appears to always set the y value if there was an x value.  I guess that's why we haven't noticed this issue before: gradient->mBgPos.mYValue will always be non-null if gradient->mBgPos.mXValue is non-null.

So I think after all we have nothing to test here.  Sorry for making you look into that, although hopefully now you know how to modify a mochitest for future patches. :-)  Let's get your patch landed.
Flags: needinfo?(cam)
Attachment #8616120 - Flags: review?(cam) → review+
Not a problem at all! I'm definitely here to learn and I definitely know about mochitests now which is always a plus, thanks!
https://hg.mozilla.org/mozilla-central/rev/6cd6a90ecb4d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.