1156019 - Several keywords are missing in auto-completion for 'background-size' and other properties

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, enhancement, P2)

Description

[Broda Noel]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150402191859

Steps to reproduce:

Open Webmaster tolls (Press 12)
Click on "Instepctor" tab.
Select any HTML element.
In right (by default) panel add a new CSS value (example: background-size)
Press ENTER (and then, you should be able to write the value of the backgorund size)
Press "UP" or "DOWN" keys


Actual results:

Nothing happend... This is the problem.


Expected results:

We should see differents approved values.
Example: cover.

I'll never use Webmaster tools until have IntelliSense in accepted CSS values and CSS Attributes!!!

Comment 1

[Tom Tromey :tromey]

The bug here is that inDOMUtils::GetCSSValuesForProperty doesn't report "cover" or "contain"
for "background-size".

In nsCSSPropList.h we see:

CSS_PROP_BACKGROUND(
    background-size,
    background_size,
    BackgroundSize,
    CSS_PROPERTY_PARSE_FUNCTION |
        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
        CSS_PROPERTY_APPLIES_TO_PLACEHOLDER |
        CSS_PROPERTY_VALUE_LIST_USES_COMMAS |
        CSS_PROPERTY_VALUE_NONNEGATIVE |
        CSS_PROPERTY_STORES_CALC,
    "",
    0,
    kImageLayerSizeKTable,
    CSS_PROP_NO_OFFSET,
    eStyleAnimType_Custom)

... and normally I'd expect that kImageLayerSizeKTable entry to cause this;
but I think the "0" (as opposed to VARIANT_KEYWORD) causes us to skip this.

This particular one is easy enough to fix, but it would be best to audit other
entries as well.  The .js property database will have to be updated after this
work is done.

It might suffice to remove the VARIANT_KEYWORD check from inDOMUtils.cpp, I'm not sure.

This is a good first bug for someone wanting to try a bit of C++.

Comment 2

[Mayank [:mayank]]

Hi, This is my first bug. I looked up the code and saw the VARIANT_KEYWORD macro, but I didn't understand what it is actually doing. Could you please help me on understanding this. 

I will try to submit a patch after I am able to reproduce the bug and fix it.
Thanks.

Comment 3

[Tom Tromey :tromey]

(In reply to mayanksri18 from comment #2)
> Hi, This is my first bug. I looked up the code and saw the VARIANT_KEYWORD
> macro, but I didn't understand what it is actually doing. Could you please
> help me on understanding this. 

This is testing a flag in the description of a particular CSS property.
If the flag is set, then it calls GetKeywordsForProperty to add the keywords
for that property to the result array.

There are two spots to be concerned with:

https://dxr.mozilla.org/mozilla-central/rev/845cc4dea57f6cc93f46810d24b1058b640c3b74/layout/inspector/inDOMUtils.cpp#942
https://dxr.mozilla.org/mozilla-central/rev/845cc4dea57f6cc93f46810d24b1058b640c3b74/layout/inspector/inDOMUtils.cpp#960

One theory is that this test isn't needed, because GetKeywordsForProperty already looks
for a non-null keyword table.

Once you make the patch and rebuild firefox, you can use "./mach devtools-css-db" to regenerate
the database.  Reading this diff carefully and double-checking the results would be a good way
to see if the change makes sense.

Comment 4

[Mayank [:mayank]]

I modified the code and built Firefox. But running "./mach devtools-css-db" throws error. I am not able to regenerate properties database. FYI I am doing this on a Ubuntu Linux environment.

Comment 5

[Tom Tromey :tromey]

What error did you get?

Comment 6

[Mayank [:mayank]]

(In reply to Tom Tromey :tromey from comment #5)
> What error did you get?

This is the error i am getting:

firefox-dev@firefox-dev:~/mozilla-central$ ./mach devtools-css-db
Re-generating the css properties database...
Error running mach:

    ['devtools-css-db']

The error occurred in the implementation of the invoked mach command.

This should never occur and is likely a bug in the implementation of that
command. Consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

KeyError: 'CPP'

  File "/home/firefox-dev/mozilla-central/devtools/shared/css/generated/mach_commands.py", line 43, in generate_css_db
    preferences = self.get_preferences()
  File "/home/firefox-dev/mozilla-central/devtools/shared/css/generated/mach_commands.py", line 56, in get_preferences
    cpp = self.substs['CPP']

Comment 7

[Julian Descottes [:jdescottes]]

Inspector bug triage, filter on CLIMBING SHOES.

Comment 8

[Tom Tromey :tromey]

(In reply to mayanksri18 from comment #6)

> This is the error i am getting:
> 
> firefox-dev@firefox-dev:~/mozilla-central$ ./mach devtools-css-db
> Re-generating the css properties database...
> Error running mach:
[...]
> The details of the failure are as follows:
> 
> KeyError: 'CPP'

I am not familiar with this error.  If I had to guess I'd wonder if you were doing
an artifact build?  But it's better to just ask the expert than rely on my guesses :)

Comment 9

[Greg Tatum [:gregtatum]]

Hopefully that is just an artifact build error. You need to do a full build to get it to work. If you are in fact already doing a full build then it is probably an error with the mach command... If that's the case needinfo me and I'll try and reproduce on my Ubuntu VM.

Comment 10

[Mayank [:mayank]]

Thanks :gregtatum. 
:tromey, After making the suggested changes, i ran the full build. But keywords "contain" & "cover" didn't show up in "background-size" property in the intellisense.

I checked the generated properties-db.js file, id doesn't have cover and contain as values for background-size. Not sure if am looking at the right place.

Comment 11

[Mayank [:mayank]]

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #9)
> Hopefully that is just an artifact build error. You need to do a full build
> to get it to work. If you are in fact already doing a full build then it is
> probably an error with the mach command... If that's the case needinfo me
> and I'll try and reproduce on my Ubuntu VM.

I did the full build. After that i again ran mach devtools-css-db (not sure if i have to do this after full build), and still throws the same error.

Comment 12

[Greg Tatum [:gregtatum]]

Ok, I'll try and take a look at it. The mach devtools-css-db is definitely OS specific. On a side note, the database only is used as a fallback. You should get a live list of values when you use the inspector with the above "steps to reproduce". The inspector queries the browser for its currently supported values, and only uses the static database as a fallback.

Comment 13

[Tom Tromey :tromey]

One possible next step would be to examine a test case in the debugger.
If you've got gdb I can help walk you through this.

Examining the property database was really just a proxy for a test anyway.

Comment 14

[Mayank [:mayank]]

(In reply to Tom Tromey :tromey from comment #13)
> One possible next step would be to examine a test case in the debugger.
> If you've got gdb I can help walk you through this.
> 
> Examining the property database was really just a proxy for a test anyway.

Ok. Yes, i have gdb.

Comment 15

[Tom Tromey :tromey]

Ok!

The first step is to write a test case.  Searching for "getCSSValuesForProperty" shows a few plausible
tests, but the main one is layout/inspector/tests/test_bug877690.html.

You're going to want a test case for the final bug anyway, so you might as well write it now.
Just add another test clause for background-size, matching the others.

Next, to make life simpler for yourself, I would suggest commenting out all the other tests in that
file.  Obviously this is just temporary, but the idea is that when you debug the test run, you
won't have to try to ignore a lot of extraneous calls to the function you're debugging.

Speaking of which, make sure you did a debug build.  If you don't have debug symbols you won't
be able to debug effectively.

You can try the test case (without debugging) using:

  ./mach mochitest layout/inspector/tests/test_bug877690.html

If it worked (and assuming you wrote the test using the new results you're expecting), then hurray,
no need to debug, we just need to figure out the css-db rebuilding.

If it failed, time to debug... try:

  ./mach mochitest --debugger gdb --disable-e10s layout/inspector/tests/test_bug877690.html

At the (gdb) prompt, set a breakpoint in the function to debug, like:

  (gdb) break inDOMUtils::GetCSSValuesForProperty

Or you can use the filename and line number

  (gdb) break inDOMUtils.cpp:926  # check your line number...

If it asks you if you want to set a pending breakpoint, say yes.

Now "run".  Your test case will grind away for a bit and then gdb should stop in the function.
Now you can "step" or "next", and inspect things with "print".  So I would step into GetKeywordsForProperty
and see what is happening there.

Comment 16

[Mayank [:mayank]]

(In reply to Tom Tromey :tromey from comment #15)
> Ok!
> 
> The first step is to write a test case.  Searching for
> "getCSSValuesForProperty" shows a few plausible
> tests, but the main one is layout/inspector/tests/test_bug877690.html.
> 
> You're going to want a test case for the final bug anyway, so you might as
> well write it now.
> Just add another test clause for background-size, matching the others.
> 
> Next, to make life simpler for yourself, I would suggest commenting out all
> the other tests in that
> file.  Obviously this is just temporary, but the idea is that when you debug
> the test run, you
> won't have to try to ignore a lot of extraneous calls to the function you're
> debugging.
> 
> Speaking of which, make sure you did a debug build.  If you don't have debug
> symbols you won't
> be able to debug effectively.
> 
> You can try the test case (without debugging) using:
> 
>   ./mach mochitest layout/inspector/tests/test_bug877690.html
> 
> If it worked (and assuming you wrote the test using the new results you're
> expecting), then hurray,
> no need to debug, we just need to figure out the css-db rebuilding.
> 
> If it failed, time to debug... try:
> 
>   ./mach mochitest --debugger gdb --disable-e10s
> layout/inspector/tests/test_bug877690.html
> 
> At the (gdb) prompt, set a breakpoint in the function to debug, like:
> 
>   (gdb) break inDOMUtils::GetCSSValuesForProperty
> 
> Or you can use the filename and line number
> 
>   (gdb) break inDOMUtils.cpp:926  # check your line number...
> 
> If it asks you if you want to set a pending breakpoint, say yes.
> 
> Now "run".  Your test case will grind away for a bit and then gdb should
> stop in the function.
> Now you can "step" or "next", and inspect things with "print".  So I would
> step into GetKeywordsForProperty
> and see what is happening there.

Thanks Tromey. I followed this and wrote the test for these values:
"initial", "inherit", "unset", "auto", "cover", "contain". 

I first removed the VARIANT_KEYWORD check from inDOMUtils.cpp and then ran the test.
The test is passing. Although I can't see these properties in Inspector intellisense until I can regenerate the css-db. So, I guess it all boils down to figuring out regenerating the css-db, right?

Actually I am still a little confused how is this all affecting the intellisense?

Thanks

Comment 17

[Tom Tromey :tromey]

(In reply to Mayank from comment #16)

> Thanks Tromey. I followed this and wrote the test for these values:
> "initial", "inherit", "unset", "auto", "cover", "contain". 
> 
> I first removed the VARIANT_KEYWORD check from inDOMUtils.cpp and then ran
> the test.
> The test is passing. 

That's great.  Feel free to attach your WIP patch if you want.

> Although I can't see these properties in Inspector
> intellisense until I can regenerate the css-db. So, I guess it all boils
> down to figuring out regenerating the css-db, right?
> 
> Actually I am still a little confused how is this all affecting the
> intellisense?

In the rule view, the text property editor uses an "inplace editor" object
to edit the property value.  This editor object can be configured to do css
completions.  In the end it queries the css database:

https://dxr.mozilla.org/mozilla-central/rev/80eac484366ad881c6a10bf81e8d9b8f7a676c75/devtools/client/shared/inplace-editor.js#1488

you can use the browser toolbox and set a breakpoint here to see it in action.

Normally, as Greg says, the generated css database shouldn't matter -- the static database
only exists for debugging older versions of firefox.  Instead, the database is recreated live
by the server (the thing being debugged or inspected) at startup.

So, the fact that this isn't working in manual tests is quite mysterious.
If you attach your patch I'll give it a try here and see if I can figure it out.

Comment 18

[Mayank [:mayank]]

Attachment: bug-1156019.patch WIP for background-size keywords issue in intellisense

Attaching a WIP patch. Please review.

Comment 19

[Tom Tromey :tromey]

Comment on attachment 8828073 [details] [diff] [review]
bug-1156019.patch WIP for background-size keywords issue in intellisense

Review of attachment 8828073 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good so far.
I am clearing the r? for a couple reasons:

1. As discussed, the patch isn't complete until the db is updated;
2. Someone else (:dholbert or :heycam) will have to review the final patch, given what it touches

::: layout/inspector/inDOMUtils.cpp
@@ -939,4 @@
>      uint32_t propertyParserVariant = nsCSSProps::ParserVariant(propertyID);
>      // Get colors first.
>      GetColorsForProperty(propertyParserVariant, array);
> -    //if (propertyParserVariant & VARIANT_KEYWORD) {

The context here indicates that there's an earlier commit by you, to comment these out.  Nothing wrong with that!  But the final patch will have to be based on m-c.

Comment 20

[Tom Tromey :tromey]

Attachment: css database patch

I applied the patch and rebuilt, then rebuilt the css database.
This worked fine for me -- still no idea why it's not working for you :(

This patch seems almost ok, however some of those "-moz-" additions
seem iffy to me.  Perhaps those should be filtered out in GetKeywordsForProperty?
But given the appearance of some in GetOtherValuesForProperty, maybe they
need to be hand audited.

Comment 21

[Saghan]

Hi,
I am new to contributing. I see that Mayank has worked on this bug but it is still not closed. Can someone else work on this bug too?

Comment 22

[Mayank [:mayank]]

(In reply to Tom Tromey :tromey from comment #20)
> Created attachment 8828093 [details] [diff] [review]
> css database patch
> 
> I applied the patch and rebuilt, then rebuilt the css database.
> This worked fine for me -- still no idea why it's not working for you :(
> 
> This patch seems almost ok, however some of those "-moz-" additions
> seem iffy to me.  Perhaps those should be filtered out in
> GetKeywordsForProperty?
> But given the appearance of some in GetOtherValuesForProperty, maybe they
> need to be hand audited.

Thanks Tromey. I guess I have to wait for gtatum's input for build error for rebuilding the css db. So, are you able to get the values in Intellisense now?
Also, not sure what 'm-c' is? Can you assign the bug to me?

Comment 23

[Tom Tromey :tromey]

(In reply to Saghan from comment #21)
> Hi,
> I am new to contributing. I see that Mayank has worked on this bug but it is
> still not closed. Can someone else work on this bug too?

Hi.  Sorry, no, I think this bug is in process and there isn't much a second person could do.

Comment 24

[Tom Tromey :tromey]

(In reply to Mayank from comment #22)
> So, are you able to get the values in Intellisense
> now?

Yes, it works for me.

> Also, not sure what 'm-c' is?

Mozilla Central - the main repository.

> Can you assign the bug to me?

Done!

Comment 25

[Mayank [:mayank]]

(In reply to Tom Tromey :tromey from comment #24)
> (In reply to Mayank from comment #22)
> > So, are you able to get the values in Intellisense
> > now?
> 
> Yes, it works for me.
> 
> > Also, not sure what 'm-c' is?
> 
> Mozilla Central - the main repository.
> 
> > Can you assign the bug to me?
> 
> Done!

Thanks, so now I am just looking forward for gtatum's input on rebuilding css-db, so that I can also see the keywords in intellisense. By the way, did you regenerated the db by running script or manually updated the css-db?

Comment 26

[Tom Tromey :tromey]

(In reply to Mayank from comment #25)

> Thanks, so now I am just looking forward for gtatum's input on rebuilding
> css-db, so that I can also see the keywords in intellisense. By the way, did
> you regenerated the db by running script or manually updated the css-db?

I just ran the mach command, and it worked for me.
I think the next step is filtering out those "-moz" values; at least the ones
I looked at didn't seem to be documented, so providing them as completions
doesn't make much sense.

Comment 27

[Greg Tatum [:gregtatum]]

I ran the mach devtools-css-db command on my ubuntu VM, and it worked fine. Seems like the script on your machine can't find CPP in your config, which is odd... I forget where the config file is located where it reads that information, but do you at least have cpp accessible from your path? Seems like you would need to in order to build...

Comment 28

[Mayank [:mayank]]

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #27)
> I ran the mach devtools-css-db command on my ubuntu VM, and it worked fine.
> Seems like the script on your machine can't find CPP in your config, which
> is odd... I forget where the config file is located where it reads that
> information, but do you at least have cpp accessible from your path? Seems
> like you would need to in order to build...

Ok, I am not sure what's wrong. Is it that it's an artifact build so i can't modify .cpp files? If so how do i do a non artifact build?

Thanks

Comment 29

[Mayank [:mayank]]

(In reply to Tom Tromey :tromey from comment #26)
> (In reply to Mayank from comment #25)
> 
> > Thanks, so now I am just looking forward for gtatum's input on rebuilding
> > css-db, so that I can also see the keywords in intellisense. By the way, did
> > you regenerated the db by running script or manually updated the css-db?
> 
> I just ran the mach command, and it worked for me.
Finally I got it working. I am able to see contain & cover in intellisense.

> I think the next step is filtering out those "-moz" values; at least the ones
> I looked at didn't seem to be documented, so providing them as completions
> doesn't make much sense.
 
As you said -moz additions may not be needed. I will check this in mdn documentation and filter accordingly.

Comment 30

[Mayank [:mayank]]

Attachment: bug-1156019.patch

Submitting a patch. Please review.

Thanks

Comment 31

[Tom Tromey :tromey]

Comment on attachment 8832580 [details] [diff] [review]
bug-1156019.patch

Review of attachment 8832580 [details] [diff] [review]:
-----------------------------------------------------------------

I had a couple of comments, so I'm clearing the review.

The final patch will have to be reviewed by someone else, since I don't have review powers for the code in question.

::: layout/inspector/inDOMUtils.cpp
@@ +590,5 @@
>    if (keywordTable) {
>      for (size_t i = 0; keywordTable[i].mKeyword != eCSSKeyword_UNKNOWN; ++i) {
> +      auto word = nsCSSKeywords::GetStringValue(keywordTable[i].mKeyword);
> +
> +      if (word.Compare("-moz-zoom-in") != 0 && word.Compare("-moz-zoom-out") != 0 &&

I think this should have a comment explaining how these particular values are chosen.

@@ +937,4 @@
>      uint32_t propertyParserVariant = nsCSSProps::ParserVariant(propertyID);
>      // Get colors first.
>      GetColorsForProperty(propertyParserVariant, array);
> +    GetKeywordsForProperty(propertyID, array);

There is another call to GetKeywordsForProperty.  One is one gated and the other not?

Comment 32

[Mayank [:mayank]]

Attachment: Bug-1156019-WIP.patch

This patch removes the VARIANT_KEYWORD check and the result is as expected. But the test for border-image property fails.

Comment 33

[Mayank [:mayank]]

(In reply to Tom Tromey :tromey from comment #31)
> Comment on attachment 8832580 [details] [diff] [review]
> bug-1156019.patch
> 
> Review of attachment 8832580 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I had a couple of comments, so I'm clearing the review.
> 
> The final patch will have to be reviewed by someone else, since I don't have
> review powers for the code in question.
> 
> ::: layout/inspector/inDOMUtils.cpp
> @@ +590,5 @@
> >    if (keywordTable) {
> >      for (size_t i = 0; keywordTable[i].mKeyword != eCSSKeyword_UNKNOWN; ++i) {
> > +      auto word = nsCSSKeywords::GetStringValue(keywordTable[i].mKeyword);
> > +
> > +      if (word.Compare("-moz-zoom-in") != 0 && word.Compare("-moz-zoom-out") != 0 &&
> 
> I think this should have a comment explaining how these particular values
> are chosen.
> 
> @@ +937,4 @@
> >      uint32_t propertyParserVariant = nsCSSProps::ParserVariant(propertyID);
> >      // Get colors first.
> >      GetColorsForProperty(propertyParserVariant, array);
> > +    GetKeywordsForProperty(propertyID, array);
> 
> There is another call to GetKeywordsForProperty.  One is one gated and the
> other not?

Yes, if the check is removed from the second call, the test for "border-image" property fails. If the check is removed only from the first call the result is as expected. 

I have attached another patch with this "border-image" issue. For expected, please check the earlier patch.
Thanks

Comment 34

[Mayank [:mayank]]

Attachment: bug-1156019.patch

This is the updated patch with fixed issues and updated tests.

Comment 35

[Tom Tromey :tromey]

Comment on attachment 8833343 [details] [diff] [review]
bug-1156019.patch

Review of attachment 8833343 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, thank you.  I still have questions, but this is looking good to me.
I am going to r+ it with the proviso that we need to get a review by someone else.

::: devtools/shared/css/generated/properties-db.js
@@ +3154,4 @@
>      "values": [
>        "COLOR",
>        "-moz-all",
> +      "-moz-alt-content",

I am curious about whether some of these additions are desirable.

::: layout/inspector/tests/test_bug877690.html
@@ +215,5 @@
>    // test border-image property, for bug 973345
>    var prop = "border-image";
>    var values = getCSSValuesForProperty(prop);
> +  var expected = [ "inherit", "initial", "unset", "repeat", "stretch", "-moz-element", "-moz-image-rect", "url", "linear-gradient",
> +  "radial-gradient", "repeating-linear-gradient", "repeating-radial-gradient", "-moz-linear-gradient", "-moz-radial-gradient", "-moz-repeating-linear-gradient", "-moz-repeating-radial-gradient", "fill", "none", "round", "space" ];

So, this change definitely seems correct based on 

https://developer.mozilla.org/en-US/docs/Web/CSS/border-image

I just wonder what in your patch might cause this.

Comment 36

[Tom Tromey :tromey]

> I just wonder what in your patch might cause this.

Logging the lengths of the two arrays in the border-image case shows that the
actual array has 15 elements and the expected array has 188.
So I think this test is actually written incorrect already - it isn't detecting
the failure mode it is intended to detect.
I think it is worth fixing this first.

Comment 37

[Tom Tromey :tromey]

Attachment: P

Here's my proposed fix to make the test check what it ought to be checking.
This correctly errors when applied on an unmodified m-c, because the
border-image test is already wrong.
I think you can just roll this into your patch and it'll be good.

Comment 38

[Mayank [:mayank]]

Attachment: bug-1156019.patch

This is the latest patch with suggested changes. I have fixed old tests also which were failing due to the changes. There were some duplicate values in expected array. Now all the tests pass.

Comment 39

[Tom Tromey :tromey]

Comment on attachment 8833637 [details] [diff] [review]
bug-1156019.patch

Review of attachment 8833637 [details] [diff] [review]:
-----------------------------------------------------------------

It looks good to me, thanks.
Let's forward to the real reviewer.

Comment 40

[Cameron McCormack (:heycam)]

Comment on attachment 8833637 [details] [diff] [review]
bug-1156019.patch

Sorry for not looking at this sooner.  I'm a bit busy so redirecting to Xidorn to look at this.

Comment 41

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8833637 [details] [diff] [review]
bug-1156019.patch

Review of attachment 8833637 [details] [diff] [review]:
-----------------------------------------------------------------

r- for the issue below. It otherwise looks fine to me.

::: layout/inspector/inDOMUtils.cpp
@@ +594,5 @@
> +      // These are extra -moz values which are added while rebuilding the properties
> +      // db. These values are not relevant and are not documented on MDN, so filter
> +      // these out.
> +      if (word.Compare("-moz-zoom-in") != 0 && word.Compare("-moz-zoom-out") != 0 &&
> +          word.Compare("-moz-grab") != 0 && word.Compare("-moz-grabbing") != 0) {

Please compare keywordTable[i].mKeyword with value of nsCSSKeyword (eCSSKeyword__moz_zoom_in, etc.) directly. That is faster and more maintainable.

Also given all of these values have unprefixed equivalents, could you file a bug for removing these prefixed values?

Comment 42

[Tom Tromey :tromey]

Mayank -- are you still working on this?  It seems very close at this point.

Comment 43

[Sebastian Zartner [:sebo]]

As this patch goes far beyond the keywords for background size, I am wondering whether it shouldn't be rather moved to bug 1337918.
Also, there are several properties missing like e.g. font-weight, and due to this patch I am wondering now whether that should be added here or I should create a new bug for all of them or I should create individual bugs for each one.

Sebastian

Comment 44

[Tom Tromey :tromey]

(In reply to Sebastian Zartner [:sebo] from comment #43)
> As this patch goes far beyond the keywords for background size, I am
> wondering whether it shouldn't be rather moved to bug 1337918.]

The bug history and reviews are all here, so if a change must be made, I'd
rather we just morph this bug and close 1337918 et al as dups.

> Also, there are several properties missing like e.g. font-weight, and due to
> this patch I am wondering now whether that should be added here or I should
> create a new bug for all of them or I should create individual bugs for each
> one.

I think you could either apply the patch and check them; or go ahead and file
bugs and then someone else can check them later

Comment 45

[Mayank [:mayank]]

(In reply to Tom Tromey :tromey from comment #42)
> Mayank -- are you still working on this?  It seems very close at this point.

Hi Tom, I am still working on this. Just got busy, I will try to push the updated patch in 2-3 days.

Comment 46

[Mayank [:mayank]]

Attachment: Bug-694162.patch

Hi, Attaching the updated patch. Please review.
Thanks

Comment 47

[Sebastian Zartner [:sebo]]

(In reply to Mayank from comment #46)
> Created attachment 8843675 [details] [diff] [review]
> Bug-694162.patch
> 
> Hi, Attaching the updated patch. Please review.
> Thanks

That's obviously the patch for another bug.

(In reply to Tom Tromey :tromey from comment #44)
> (In reply to Sebastian Zartner [:sebo] from comment #43)
> > As this patch goes far beyond the keywords for background size, I am
> > wondering whether it shouldn't be rather moved to bug 1337918.]
> 
> The bug history and reviews are all here, so if a change must be made, I'd
> rather we just morph this bug and close 1337918 et al as dups.

But if I'm not mistaked, the patch here is just part of bug 1337918, because it only fixes a few keywords (e.g. it doesn't add the keywords for the CSS Grid properties, which is really sad regarding the 52 release).
Therefore I think it's better to keep the relation as is, but only change the summary of this bug to reflect the more global change.
Please correct me if I am wrong regarding the statement above.

> > Also, there are several properties missing like e.g. font-weight, and due to
> > this patch I am wondering now whether that should be added here or I should
> > create a new bug for all of them or I should create individual bugs for each
> > one.
> 
> I think you could either apply the patch and check them; or go ahead and file
> bugs and then someone else can check them later

I don't have the time now to do that.
Mayank, can you add the missing pieces? Otherwise I'll file follow-up bugs after the patch is in Nightly.

Sebastian

Comment 48

[Sebastian Zartner [:sebo]]

(In reply to Sebastian Zartner [:sebo] from comment #47)
> (In reply to Tom Tromey :tromey from comment #44)
> > (In reply to Sebastian Zartner [:sebo] from comment #43)
> > > As this patch goes far beyond the keywords for background size, I am
> > > wondering whether it shouldn't be rather moved to bug 1337918.]
> > 
> > The bug history and reviews are all here, so if a change must be made, I'd
> > rather we just morph this bug and close 1337918 et al as dups.
> 
> But if I'm not mistaked

s/mistaked/mistaken

> Therefore I think it's better to keep the relation as is

Said it and changed it accidentally. :-/

Sebastian

Comment 49

[Mayank [:mayank]]

Attachment: bug-1156019.patch

Sorry about that, didn't realise it was the wrong file.
Attached the correct patch.

Comment 50

[Tom Tromey :tromey]

Comment on attachment 8843714 [details] [diff] [review]
bug-1156019.patch

It looks good to me but really Xidorn ought to look at it.

Comment 51

[Tom Tromey :tromey]

(In reply to Sebastian Zartner [:sebo] from comment #47)

> But if I'm not mistaked, the patch here is just part of bug 1337918, because
> it only fixes a few keywords (e.g. it doesn't add the keywords for the CSS
> Grid properties, which is really sad regarding the 52 release).
> Therefore I think it's better to keep the relation as is, but only change
> the summary of this bug to reflect the more global change.
> Please correct me if I am wrong regarding the statement above.

I applied the patch and tried it out, and the style editor and rule view now
do offer completions for grid properties.  I also tried font-weight (mentioned
in some earlier comment) and that works as well.
I think what might be going on is that these properties don't show up in the
static database, because they are preffed off.  That's ok, though, because the
static database is only used in a particular situation, where the inspector is
being used to inspect an older version of firefox.  In normal cases the properties
come from the "live" firefox and all will be ok.

Comment 52

[Xidorn Quan [:xidorn] UTC+11]

One suggestion for Mayank: if you're using Mercurial, please add the following to your ~/.hgrc
> [diff]
> git = 1
> showfunc = 1
> unified = 8
so that reviewers can have more context of the change you are making from the patch.

Comment 53

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8843714 [details] [diff] [review]
bug-1156019.patch

Review of attachment 8843714 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comment addressed.

::: layout/inspector/inDOMUtils.cpp
@@ +589,4 @@
>      nsCSSProps::kKeywordTableTable[aProperty];
>    if (keywordTable) {
>      for (size_t i = 0; keywordTable[i].mKeyword != eCSSKeyword_UNKNOWN; ++i) {
> +      auto word = keywordTable[i].mKeyword;

I'd prefer you keep nsCSSKeyword for the type here. You don't really need to change this line, and explicit type name is usually preferable than "auto" before it helps readilibty.

@@ +595,5 @@
> +      // db. These values are not relevant and are not documented on MDN, so filter
> +      // these out.
> +      if (word != eCSSKeyword__moz_zoom_in && word != eCSSKeyword__moz_zoom_out &&
> +          word != eCSSKeyword__moz_grab && word != eCSSKeyword__moz_grabbing) {
> +        InsertNoDuplicates(aArray, NS_ConvertASCIItoUTF16(nsCSSKeywords::GetStringValue(word)));

These lines are too long. Please keep lines under 80 characters including the comment.

For this line here, it seems to be pretty hard to keep under 80 characters... Probably something like
>       InsertNoDuplicates(aArray,
>         NS_ConvertASCIItoUTF16(nsCSSKeywords::GetStringValue(word)));
is acceptable.

Comment 54

[Mayank [:mayank]]

Attachment: bug-1156019.patch

Attaching updated patch.
Thanks.

Comment 55

[Tom Tromey :tromey]

Comment on attachment 8845045 [details] [diff] [review]
bug-1156019.patch

It's fine to carry over the r+ if you addressed the comments.
But, you should put "r=xidorn" in the commit message.
Also, it's time to push it through "try" - do you have access to do this?
If not, I can do it for you.

Comment 56

[Mayank [:mayank]]

(In reply to Tom Tromey :tromey from comment #55)
> Comment on attachment 8845045 [details] [diff] [review]
> bug-1156019.patch
> 
> It's fine to carry over the r+ if you addressed the comments.
> But, you should put "r=xidorn" in the commit message.
> Also, it's time to push it through "try" - do you have access to do this?
> If not, I can do it for you.

In commit it's xidorn only. I don't have access to try.
Thanks

Comment 57

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=320f23864202eb67e7f3e66b3d3965e367104e62

FWIW if you use MozReview rather than splinter, it's much simpler to push patches
to try, and it's also simpler for reviewers to see the commit message.
(I still don't know where to see it with splinter...)
So on the whole I'd recommend switching to MozReview.

Comment 58

[Tom Tromey :tromey]

Sorry about the delay on this.  I forgot about it yesterday.
The try run shows some relevant failures, see the "X2" jobs.

 TEST-UNEXPECTED-FAIL | devtools/shared/tests/unit/test_css-properties-db.js | xpcshell return code: 0 
 TEST-UNEXPECTED-FAIL | devtools/shared/tests/unit/test_css-properties-db.js | run_test - [run_test : 63] The static database and platform do not match for

Comment 59

[Tom Tromey :tromey]

Any word on this?

Comment 60

[Sebastian Zartner [:sebo]]

(In reply to Tom Tromey :tromey from comment #51)
> (In reply to Sebastian Zartner [:sebo] from comment #47)
> 
> > But if I'm not mistaken, the patch here is just part of bug 1337918, because
> > it only fixes a few keywords (e.g. it doesn't add the keywords for the CSS
> > Grid properties, which is really sad regarding the 52 release).
> > Therefore I think it's better to keep the relation as is, but only change
> > the summary of this bug to reflect the more global change.
> > Please correct me if I am wrong regarding the statement above.
> 
> I applied the patch and tried it out, and the style editor and rule view now
> do offer completions for grid properties.  I also tried font-weight
> (mentioned
> in some earlier comment) and that works as well.
> I think what might be going on is that these properties don't show up in the
> static database, because they are preffed off.  That's ok, though, because
> the
> static database is only used in a particular situation, where the inspector
> is
> being used to inspect an older version of firefox.  In normal cases the
> properties
> come from the "live" firefox and all will be ok.

That sounds promising. I'll have a look at it when it lands.
So, Mayank, I hope you can finish the last bits soon!

Sebastian

Comment 61

[Mayank [:mayank]]

(In reply to Tom Tromey :tromey from comment #59)
> Any word on this?

Sorry, I am keeping a little busy. May be by this weekend I will look into this and patch.
Although I am not sure what is platform mismatch here.

Comment 62

[Tom Tromey :tromey]

(In reply to Mayank from comment #61)

> Sorry, I am keeping a little busy. May be by this weekend I will look into
> this and patch.
> Although I am not sure what is platform mismatch here.

It seems that the logs have been removed already, so I think the only thing to do is
run the particular test locally and see what's going wrong.  Normally I think this can
only happen if the css database isn't updated by the patch.

Comment 63

[Mayank [:mayank]]

(In reply to Tom Tromey :tromey from comment #62)
> (In reply to Mayank from comment #61)
> 
> > Sorry, I am keeping a little busy. May be by this weekend I will look into
> > this and patch.
> > Although I am not sure what is platform mismatch here.
> 
> It seems that the logs have been removed already, so I think the only thing
> to do is
> run the particular test locally and see what's going wrong.  Normally I
> think this can
> only happen if the css database isn't updated by the patch.

I don't get any test errors while running the modified test. Is it some other test I need to see?

Comment 64

[Tom Tromey :tromey]

(In reply to Mayank from comment #63)

> I don't get any test errors while running the modified test. Is it some
> other test I need to see?

I don't see problems locally either.
(The patch didn't apply cleanly but regenerating the database fixed it up)
I pushed to try again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=69e78b3a8208eaba2ec7865ae1921af37a4d154b

Comment 65

[Tom Tromey :tromey]

Well, that try run looked a lot better :-)
Who knows what went wrong previously, could have been my error ... one of the reasons
to prefer MozReview is that it connects to try, avoiding any patch-application problems.
Anyway, could you rebase the patch (making sure to re-run the mach command) and then
reattach it?  I think it's ready to go.

Comment 66

[Mayank [:mayank]]

Attachment: bug-1156019.patch

Attached the rebuilt patch.

Comment 67

[Tom Tromey :tromey]

It seems to me that the rebase was on a quite old base -- a commit from March 6.
It doesn't apply on today's autoland, for example.
Could you rebase onto inbound or something like that instead?
Otherwise I think we won't be able to land this.

Comment 68

[Tom Tromey :tromey]

Attachment: Fixed the issue of background-size property, regenerated properties css database & updated tests. r=xidorn

MozReview-Commit-ID: ACRxUEWBbSM

Comment 69

[Tom Tromey :tromey]

Rebased.

Comment 70

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fd8811d4c02e30103002176413217a2096f531e

Comment 71

[Tom Tromey :tromey]

Comment on attachment 8855884 [details] [diff] [review]
Fixed the issue of background-size property, regenerated properties css database & updated tests. r=xidorn

Carrying over the r+

Comment 72

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f846a3a49b96
Fix the issue of background-size property, regenerated properties css database & updated tests. r=xidorn

Comment 73

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f846a3a49b96

Comment 74

[Mayank [:mayank]]

Thanks for mentoring Tom.