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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Simon Lindholm]

domUtils = Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
domUtils.getCSSValuesForProperty('border-image')

should include linear-gradient, radial-gradient, repeating-linear-gradient and repeating-radial-gradient.

Comment 1

[Mina Almasry]

Although I'd love to I don't think I can work on this right now. CC'ing bz to get this some attention.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Someone would need to teach getCSSValuesForProperty about VARIANT_GRADIENT (and presumably other subcomponents of VARIANT_IMAGE as well).

Comment 3

[Nicolás Bordenabe]

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? 

Thanks.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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!

Comment 5

[Nicolás Bordenabe]

Attachment: bug973345.patch

Comment 6

[Nicolás Bordenabe]

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.

Comment 7

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

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).

Comment 8

[Nicolás Bordenabe]

You are right, thank you! Done.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8395961 [details] [diff] [review]
bug973345.patch

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?

Comment 10

[Nicolás Bordenabe]

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).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Upload a new patch with the changes, mark the existing one obsolete.

Comment 12

[Nicolás Bordenabe]

Attachment: bug973345.patch

Ok, I think I implemented all the requested changes. Let me know what you think.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8396410 [details] [diff] [review]
bug973345.patch

Looks great!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c7a936eb3d1

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0c7a936eb3d1