Implement an API for getting CSS property values to be used in auto-completion

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: miker, Assigned: almasry.mina)

Tracking

(Blocks 1 bug)

Trunk
mozilla25
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 6 obsolete attachments)

bz asked me to log the things we need from bug 766032 in a new bug so here it is.

The API should be simple, something like getCSSValuesForProperty("display"). The resultant array should contain:

1. "initial"
2. "inherit"
3. For a longhand that takes keyword values, all the keyword values.
4. For a longhand that takes color values all named colors.
5. For a shorthand the union of the lists for all its subproperties.
Mina, do you want to take a shot at this?
Whiteboard: [mentor=bz][lang=c++]
I’m puzzled by item 5. Do you really want property names mixed in an array of property values?
Item 5 is the union of the lists of the subproperty _values_.
Note that some shorthands do not accept all keywords in the union of those accepted by their longhands. For example, css-fonts-3 adds a lot of values to 'font-variant', but only the 2.1 variant keywords are available in the 'font' shorthand:

http://www.w3.org/TR/css3-fonts/#ltfont-variant-css21gt

I think that the list you want is:

1. CSS-wide keywords. 'initial' and 'inherit' for now, css-cascade-3 adds 'default'. http://www.w3.org/TR/css3-values/#common-keywords
2. Keywords accepted by the property.

Named colors are keywords, and all propreties (longhand or shorthand) define their own syntax,the 'Value' line in specs.
That's a pain to implement, actually, since anything that has that behavior will have custom parsing and hence no easy way to extract that information...  :(
Maybe we can ignore the font/font-variant corner case (I’m not aware of another one) but don’t many if not all properties have custom parsing, shorthand or not?
The vast majority of non-shorthand properties in Gecko effectively use a table-driven parser, last I checked.  To quantify, 189 parseable longhands use the table-driven setup while 38 use custom functions.

All shorthands use custom functions, of course.
Posted patch proposed patch (obsolete) — Splinter Review
I'm on it! So far I think I've figured out the keywords, but I'm having trouble with colors. If I understand correctly, if a property takes colors, then we'd want to append the entire color table in [1]?

Also, I'm having trouble telling whether a property takes color values. In the parser the test is variantMask & VARIANT_COLOR, but VARIANT_COLOR is only defined in nsCSSParser.cpp. What should I do about that?

[1] http://dxr.mozilla.org/mozilla-central/source/gfx/src/nsColor.cpp#l24

[2] http://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp#5295
Attachment #758817 - Flags: review?(bzbarsky)
Comment on attachment 758817 [details] [diff] [review]
proposed patch

> +#include "nsCSSParser.h"

Shouldn't need this.

>+  if (!(keywordTable == nullptr)) {

  if (keywordTable)

>+    int i = 0;

size_t?  Or at least int32_t.

>+      aArray.AppendElement(
>+          NS_ConvertUTF8toUTF16(nsCSSKeywords::GetStringValue(word)));

Probably faster to do:

  CopyASCIItoUTF16(nsCSSKeywords::GetStringValue(word),
                   *aArray.AppendElement());

>+  array.AppendElement(NS_ConvertUTF8toUTF16("initial"));

  NS_LITERAL_STRING("initial")

and likewise for "inherit".  Also, do we support unprefixed "initial" now?  If not, "-moz-initial"...

>+  nsCSSProperty propertyID = nsCSSProps::LookupProperty(aProperty,
>+                                                        nsCSSProps::eEnabled);

If this returns eCSSProperty_UNKNOWN you probably want to throw or something... or at least return immediately with just the initial/inherit bit.

>+  if (!ret) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }

NS_Alloc is infallible, so no need.

>+    ret[i] = NS_strdup(array[i].get());

  ret[i] = ToNewUnicode(array[i]);

> VARIANT_COLOR is only defined in nsCSSParser.cpp. 

I think it should be fine to move all the VARIANT_* defines to nsCSSProps.h...
Attachment #758817 - Flags: review?(bzbarsky) → review+
David, any objection to moving the VARIANT_* bits?
Flags: needinfo?(dbaron)
I guess it's ok, though I'm somewhat hesitant to expose that stuff to this sort of API.  I guess if the API is only exposing a few of the bits (e.g., VARIANT_COLOR), I'm probably ok with it.
Flags: needinfo?(dbaron)
I don't think the API would expose the bits at all; it would just use the bits in the inspector utils code to decide what values the property supports.
Posted patch proposed patch v2 (obsolete) — Splinter Review
How is this?

I had to add an accessor for kColorNames in nsColor to access the color names, but beyond that I think everything was straightforward. 

I have a few whitespace errors in the diff, but they're because the code in the tree has trailing whitespace, and not because I'm adding any. Seems like getting rid of the whitespace errors will be a pain. Let me know if necessary.
Attachment #758817 - Attachment is obsolete: true
Attachment #766487 - Flags: review?(bzbarsky)
Attachment #766487 - Flags: review?(bzbarsky)
Posted patch proposed patch with mochitest (obsolete) — Splinter Review
Hi bz, I hope all is well. 

I've worked on this patch some more. I've added a mochitest, and fixed mistakes the test reveals, and removed all the whitespace diffs. Now the patch builds, and passes the attached test. This should be a much easier review for you.
Attachment #766487 - Attachment is obsolete: true
Attachment #770689 - Flags: review?(bzbarsky)
Posted patch proposed patch v2 with test (obsolete) — Splinter Review
Caught a couple mistakes in my last patch and fixed them.
Attachment #770689 - Attachment is obsolete: true
Attachment #770689 - Flags: review?(bzbarsky)
Attachment #771177 - Flags: review?(bzbarsky)
Posted patch patch (obsolete) — Splinter Review
I don't know if you tried looking at the patch I had yet, but it was edited in a way that breaks the review tool and makes it appear wrong. I've attached a correct patch now.
Attachment #771177 - Attachment is obsolete: true
Attachment #771177 - Flags: review?(bzbarsky)
Attachment #772413 - Flags: review?(bzbarsky)
Assignee: nobody → almasry.mina
Boris just a friendly reminder this patch is waiting review. I think you'll like this one, too. It has a mochitest written, it passes the test, and I think the behaviour is what you want.
Comment on attachment 772413 [details] [diff] [review]
patch

>+++ b/gfx/src/nsColor.cpp
>+  if (kColorNames) {

kColorNames can never be null, right?  So it should be reasonable to just return the const char** directly from this function, too.

>+++ b/gfx/src/nsColor.h
>+NS_GFX_(bool) NS_AllColorNames(const char * const ** aPColorNameArray,

Document that the caller is NOT supposed to free the array?

The rest of this looks great.  r=me with the signature of NS_AllColorNames fixed up.  And sorry for the lag...
Attachment #772413 - Flags: review?(bzbarsky) → review+
Posted patch (I think) final patch (obsolete) — Splinter Review
Wooo! First granted review from you!

Here is a patch with all the changes. What is there to do now? Mark checkin-needed?

Also, if there is anything else you're mentoring that you'd like me to work on, please point me to that.
Attachment #772413 - Attachment is obsolete: true
Attachment #775220 - Flags: review?(bzbarsky)
Comment on attachment 775220 [details] [diff] [review]
(I think) final patch

r=me

If you want to give bug 888864 or bug 734861 or bug 82711 a shot maybe?
Attachment #775220 - Flags: review?(bzbarsky) → review+
Here is my commiter's agreement, in case you didn't have one on file before.
FWIW, there was a green push prior to the backout, so this might simply be an IID rev or clobber issue.
We do need to rev the iid of inIDOMUtils in this bug.
Reved the IID
Attachment #775220 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #775790 - Attachment is patch: true
Attachment #775790 - Attachment mime type: text/x-patch → text/plain
https://hg.mozilla.org/mozilla-central/rev/0cedd434dddb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
https://hg.mozilla.org/mozilla-central/rev/0cedd434dddb#l6.22
> +  if (!array.indexOf) {
!Array.isArray(array)

https://hg.mozilla.org/mozilla-central/rev/0cedd434dddb#l6.99
> +  ok(testValues(values, expected), "Shorthand proprety values.");
https://hg.mozilla.org/mozilla-central/rev/0cedd434dddb#l6.134
> +  // test proprety
https://hg.mozilla.org/mozilla-central/rev/0cedd434dddb#l6.148
> +  ok(testValues(values, expected), "proprety float values");
s/proprety/property/
Depends on: 895076
Depends on: 899230
Depends on: 907816
You need to log in before you can comment on or make changes to this bug.