Bug 167255

This is probably my fault. I think I added them to the ComputedStyles list as I was gather a list of all the CSSPropertyIDs that were missing.

Hmm, no I didn't add them and they don't show up in the list I was thinking of (computedProperties in CSSComputedStyleDeclarations.cpp).

<rdar://problem/30126160>

This is a hot topic in the css wg currently. Would be nice to fix this: https://github.com/w3c/csswg-drafts/issues/116

@Simon/Myles, did this progress with recent change to make this not web-visible?

background-repeat-x/y were historically not web-visible. We temporarily made them visible by mistake, which I reverted yesterday. But I think the inspector has treated them as actual properties for a long time.

Yes, Devin pointed out that they show up in the CSS autocompletion for example because it show sup in CSS.getSupportedCSSProperties. We should be excluding it from that list. I believe Devin started looking at this already.

These properties have "internal-only": true in the JSON now, so you should key off of that to decide what to hide in the UI.

Created attachment 299857 [details] Patch

Comment on attachment 299857 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299857&action=review On the face, this looks good to me! Update the test and I'll give it a solid review. > Source/WebCore/css/makeprop.pl:145 > + Style: remove this blank line. > Source/WebCore/css/makeprop.pl:402 > +bool internalCSSProperty(const CSSPropertyID&); I'd name this "isInternalCSSProperty". And have it take a CSSPropertyID, not a reference. Since its just a numeric value. > LayoutTests/inspector/css/property-validity.html:13 > +<script> > +function test() { I'd like to see this rewritten to use the TestSuite code. It wouldn't need to change much and I suspect it would be easier to read. > LayoutTests/inspector/css/property-validity.html:72 > + <p>Testing that the author rules returned by CSSStyleManager.stylesForNode have lowercase property names regardless of CSS source formatting.</p> This description does not match the test.

Created attachment 299869 [details] Patch

Comment on attachment 299869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299869&action=review r=me > Source/WebCore/css/makeprop.pl:260 > + print GPERF " case CSSPropertyID::CSSProperty" . $nameToId{$name} . ":\n"; Style: Perl indentation here should be 4 spaces. > LayoutTests/inspector/css/property-members.html:26 > + suite.addTestCase({ > + name: "CSSProperty.valid", This is beginning to look like an excellent unit test. Compare it to: LayoutTests/inspector/unit-tests/wrapped-promise.html We should follow that example and make this a unit-test for CSSProperty. Rename it to: (Keeping this in the css subdirectory because it depends on CSS domain behavior) LayoutTests/inspector/css/css-property.html Keep the suite name "CSSProperty" but name the individual test cases after the methods. So this one would be "CSSProperty.prototype.get valid". The `validateProperty` function can go inside of here, since if you were to test other CSSProperty.prototype methods. We can handle any other cleanup later when we start testing other CSSProperty methods. You may even want to test a slew of other CSSProperty.prototype methods while you are here! > LayoutTests/inspector/css/property-members.html:64 > + InspectorTest.log("DOM node not found."); Nit: While you're here this might as well be `InspectorTest.fail` instead of `InspectorTest.log`. > LayoutTests/inspector/css/property-members.html:73 > + <p>Testing that member values of CSSProperty are correct.</p> In the same spirit, just make this "Testing methods of CSSProperty."

Created attachment 299921 [details] Patch

Comment on attachment 299921 [details] Patch New tests look good for me!

Comment on attachment 299921 [details] Patch Clearing flags on attachment: 299921 Committed r211289: <http://trac.webkit.org/changeset/211289>

All reviewed patches have been landed. Closing bug.