Closed Bug 973345 Opened 6 years ago Closed 6 years ago

domUtils.getCSSValuesForProperty is missing {repeating-,}{linear,radial}-gradient


(Core :: CSS Parsing and Computation, defect)

Not set





(Reporter: simon.lindholm10, Assigned: nbordenabe)


(Blocks 1 open bug)


(Whiteboard: [][lang=c++])


(1 file, 1 obsolete file)

domUtils = Cc[";1"].getService(Ci.inIDOMUtils);

should include linear-gradient, radial-gradient, repeating-linear-gradient and repeating-radial-gradient.
Although I'd love to I don't think I can work on this right now. CC'ing bz to get this some attention.
Someone would need to teach getCSSValuesForProperty about VARIANT_GRADIENT (and presumably other subcomponents of VARIANT_IMAGE as well).
Whiteboard: [][lang=c++]
Hi, I would like to start contributing, and I thought this could be a good bug to start. However, this is my first experience with the Firefox source code. Any pointers on where/how to start? 

Nicolás, assuming you've already downloaded the code and compiled it, you want to look at layout/inspector/inDOMUtils.cpp, which is where inDOMUtils::GetCSSValuesForProperty is implemented.  The static GetOtherValuesForProperty method there is what adds various values to the list based on how the propery is parsed.  

You'll want to add a new block to that function to handle the case aParserVariant & VARIANT_GRADIENT.  It looks like the other subtypes of VARIANT_IMAGE are all handled there already.

If you run into any trouble, feel free to mail me directly!
Attached patch bug973345.patch (obsolete) — Splinter Review
Ok, I think the issue is fixed. I added a test case for this within the tests for Bug 877690. It seems to be working.
If you have a patch that you think is ready, you should request review by setting the review flag on the attachment to ? and entering an email address of the reviewer (in this case, probably Boris's).
Attachment #8395961 - Flags: review?(bzbarsky)
You are right, thank you! Done.
Comment on attachment 8395961 [details] [diff] [review]

The code looks good, thank you!  You may want to add the -moz- prefixed versions of those 4 values in there, while we support them, though.

For the commit message, it might be better to have something like this:

Bug 973345 - Include gradient values in the return value of getCSSValuesForProperty() for properties that can have an <image> value specified.  r=bzbarsky

which describes the actual goal of the patch as opposed to the mechanics (which are pretty clear from the diff).

r=me with those two nits fixed.  Please let me know whether you'll be able to update the patch yourself or whether you'd like me to do it, ok?
Attachment #8395961 - Flags: review?(bzbarsky) → review+
I will do it, I'll try to have it ready later today, thanks! I should update the patch, right? (instead of ulpoading a new patch).
Upload a new patch with the changes, mark the existing one obsolete.
Attached patch bug973345.patchSplinter Review
Ok, I think I implemented all the requested changes. Let me know what you think.
Attachment #8395961 - Attachment is obsolete: true
Attachment #8396410 - Flags: review?(bzbarsky)
Comment on attachment 8396410 [details] [diff] [review]

Looks great!
Attachment #8396410 - Flags: review?(bzbarsky) → review+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.