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

RESOLVED FIXED in Firefox 55

Status

enhancement
P2
normal
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: brodanoel, Assigned: mayank, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

37 Branch
Firefox 55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 11 obsolete attachments)

Reporter

Description

4 years ago
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!!!
Reporter

Updated

4 years ago
Component: Untriaged → Developer Tools: Style Editor
Summary: IntelliSense in CSS values in Webmaster tools → IntelliSense in CSS values in Developer tools
Reporter

Updated

4 years ago
OS: Windows 7 → All
Hardware: x86_64 → All

Updated

4 years ago
Component: Developer Tools: Style Editor → Developer Tools: Object Inspector

Updated

4 years ago
Severity: normal → enhancement
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++.
Mentor: ttromey
Status: UNCONFIRMED → NEW
Component: Developer Tools: Object Inspector → Developer Tools: Inspector
Ever confirmed: true
Keywords: good-first-bug
Assignee

Comment 2

3 years ago
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.
Flags: needinfo?(ttromey)
(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.
Flags: needinfo?(ttromey)
Assignee

Comment 4

3 years ago
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.
What error did you get?
Assignee

Comment 6

3 years ago
(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']
Inspector bug triage, filter on CLIMBING SHOES.
Priority: -- → P2
(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 :)
Flags: needinfo?(gtatum)

Updated

3 years ago
Summary: IntelliSense in CSS values in Developer tools → background-size doesn't autocomplete with its keywords

Updated

3 years ago
Component: Developer Tools: Inspector → Developer Tools: CSS Rules Inspector
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.
Flags: needinfo?(gtatum)
Assignee

Comment 10

3 years ago
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.
Assignee

Comment 11

3 years ago
(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.
Flags: needinfo?(gtatum)
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.
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.
Assignee

Comment 14

3 years ago
(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.
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.
Assignee

Comment 16

3 years ago
(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
(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.
Assignee

Comment 18

3 years ago
Attaching a WIP patch. Please review.
Attachment #8828073 - Flags: review?(ttromey)
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.
Attachment #8828073 - Flags: review?(ttromey)
Posted patch css database patch (obsolete) — Splinter Review
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

3 years ago
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?
Flags: needinfo?(ttromey)
Assignee

Comment 22

3 years ago
(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?
(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.
Assignee: nobody → mayanksri18
Flags: needinfo?(ttromey)
(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!
Assignee

Comment 25

3 years ago
(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?
(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.
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...
Flags: needinfo?(gtatum)
Assignee

Comment 28

3 years ago
(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
Assignee

Comment 29

3 years ago
(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.
Assignee

Comment 30

2 years ago
Posted patch bug-1156019.patch (obsolete) — Splinter Review
Submitting a patch. Please review.

Thanks
Attachment #8828073 - Attachment is obsolete: true
Attachment #8832580 - Flags: review?(ttromey)
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?
Attachment #8832580 - Flags: review?(ttromey)
Assignee

Comment 32

2 years ago
Posted patch Bug-1156019-WIP.patch (obsolete) — Splinter Review
This patch removes the VARIANT_KEYWORD check and the result is as expected. But the test for border-image property fails.
Assignee

Comment 33

2 years ago
(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
Assignee

Comment 34

2 years ago
Posted patch bug-1156019.patch (obsolete) — Splinter Review
This is the updated patch with fixed issues and updated tests.
Attachment #8832580 - Attachment is obsolete: true
Attachment #8832981 - Attachment is obsolete: true
Attachment #8833343 - Flags: review?(ttromey)
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.
Attachment #8833343 - Flags: review?(ttromey) → review+
> 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.
Posted patch P (obsolete) — Splinter Review
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.
Assignee

Comment 38

2 years ago
Posted patch bug-1156019.patch (obsolete) — Splinter Review
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.
Attachment #8833637 - Flags: review?(ttromey)
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.
Attachment #8833637 - Flags: review?(ttromey)
Attachment #8833637 - Flags: review?(cam)
Attachment #8833637 - Flags: review+
Status: NEW → ASSIGNED
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.
Attachment #8833637 - Flags: review?(cam) → review?(xidorn+moz)
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?
Attachment #8833637 - Flags: review?(xidorn+moz) → review-
Mayank -- are you still working on this?  It seems very close at this point.
Flags: needinfo?(mayanksri18)
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
(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
Assignee

Comment 45

2 years ago
(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.
Flags: needinfo?(mayanksri18)
Assignee

Comment 46

2 years ago
Posted patch Bug-694162.patch (obsolete) — Splinter Review
Hi, Attaching the updated patch. Please review.
Thanks
Attachment #8833343 - Attachment is obsolete: true
Attachment #8833637 - Attachment is obsolete: true
Attachment #8843675 - Flags: review?(ttromey)
(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
No longer blocks: devtools/auto-completion
Summary: background-size doesn't autocomplete with its keywords → Auto-completion of values is missing for several properties
(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
Summary: Auto-completion of values is missing for several properties → Several keywords are missing in auto-completion for 'background-size' and other properties
Assignee

Comment 49

2 years ago
Posted patch bug-1156019.patch (obsolete) — Splinter Review
Sorry about that, didn't realise it was the wrong file.
Attached the correct patch.
Attachment #8843675 - Attachment is obsolete: true
Attachment #8843675 - Flags: review?(ttromey)
Attachment #8843714 - Flags: review?(ttromey)
Comment on attachment 8843714 [details] [diff] [review]
bug-1156019.patch

It looks good to me but really Xidorn ought to look at it.
Attachment #8843714 - Flags: review?(ttromey) → review?(xidorn+moz)

Updated

2 years ago
Attachment #8828093 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8833442 - Attachment is obsolete: true
(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.
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 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.
Attachment #8843714 - Flags: review?(xidorn+moz) → review+
Assignee

Comment 54

2 years ago
Posted patch bug-1156019.patch (obsolete) — Splinter Review
Attaching updated patch.
Thanks.
Attachment #8845045 - Flags: review?(ttromey)
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.
Flags: needinfo?(mayanksri18)
Attachment #8845045 - Flags: review?(ttromey) → review+
Assignee

Comment 56

2 years ago
(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
Flags: needinfo?(mayanksri18)

Updated

2 years ago
Attachment #8843714 - Attachment is obsolete: true
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.
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
Flags: needinfo?(mayanksri18)
Any word on this?
(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
Assignee

Comment 61

2 years ago
(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.
(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.
Assignee

Comment 63

2 years ago
(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?
Flags: needinfo?(mayanksri18) → needinfo?(ttromey)
(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
Flags: needinfo?(ttromey)
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.
Flags: needinfo?(mayanksri18)
Assignee

Comment 66

2 years ago
Posted patch bug-1156019.patch (obsolete) — Splinter Review
Attached the rebuilt patch.
Flags: needinfo?(mayanksri18)
Attachment #8854907 - Flags: review?(ttromey)
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.
Flags: needinfo?(mayanksri18)

Updated

2 years ago
Attachment #8854907 - Flags: review?(ttromey) → review+

Updated

2 years ago
Attachment #8845045 - Attachment is obsolete: true
Rebased.
Flags: needinfo?(mayanksri18)

Updated

2 years ago
Attachment #8854907 - Attachment is obsolete: true
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+
Attachment #8855884 - Flags: review+

Updated

2 years ago
Keywords: checkin-needed

Comment 72

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f846a3a49b96
Fix the issue of background-size property, regenerated properties css database & updated tests. r=xidorn
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f846a3a49b96
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Assignee

Comment 74

2 years ago
Thanks for mentoring Tom.

Updated

2 years ago
Duplicate of this bug: 1106336

Updated

2 years ago
Duplicate of this bug: 930665

Updated

2 years ago
Duplicate of this bug: 1254955

Updated

2 years ago
Duplicate of this bug: 1255403

Updated

2 years ago
Duplicate of this bug: 1255385

Updated

2 years ago
Duplicate of this bug: 1386050

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.