Closed
Bug 1156019
Opened 10 years ago
Closed 8 years ago
Several keywords are missing in auto-completion for 'background-size' and other properties
Categories
(DevTools :: Inspector: Rules, enhancement, P2)
Tracking
(firefox55 fixed)
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: brodanoel, Assigned: mayank, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: good-first-bug)
Attachments
(1 file, 11 obsolete files)
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•10 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•10 years ago
|
OS: Windows 7 → All
Hardware: x86_64 → All
Component: Developer Tools: Style Editor → Developer Tools: Object Inspector
Comment 1•8 years ago
|
||
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•8 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)
Comment 3•8 years ago
|
||
(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•8 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.
Comment 5•8 years ago
|
||
What error did you get?
Assignee | ||
Comment 6•8 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']
Comment 8•8 years ago
|
||
(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•8 years ago
|
Summary: IntelliSense in CSS values in Developer tools → background-size doesn't autocomplete with its keywords
Updated•8 years ago
|
Component: Developer Tools: Inspector → Developer Tools: CSS Rules Inspector
Comment 9•8 years ago
|
||
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•8 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•8 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)
Comment 12•8 years ago
|
||
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•8 years ago
|
||
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•8 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.
Comment 15•8 years ago
|
||
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•8 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
Comment 17•8 years ago
|
||
(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•8 years ago
|
||
Attaching a WIP patch. Please review.
Attachment #8828073 -
Flags: review?(ttromey)
Comment 19•8 years ago
|
||
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)
Comment 20•8 years ago
|
||
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•8 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•8 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?
Comment 23•8 years ago
|
||
(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)
Comment 24•8 years ago
|
||
(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•8 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?
Comment 26•8 years ago
|
||
(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•8 years ago
|
||
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•8 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•8 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•8 years ago
|
||
Submitting a patch. Please review.
Thanks
Attachment #8828073 -
Attachment is obsolete: true
Attachment #8832580 -
Flags: review?(ttromey)
Comment 31•8 years ago
|
||
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•8 years ago
|
||
This patch removes the VARIANT_KEYWORD check and the result is as expected. But the test for border-image property fails.
Assignee | ||
Comment 33•8 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•8 years ago
|
||
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 35•8 years ago
|
||
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+
Comment 36•8 years ago
|
||
> 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•8 years ago
|
||
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•8 years ago
|
||
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 39•8 years ago
|
||
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+
Updated•8 years ago
|
Blocks: devtools/auto-completion
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment 40•8 years ago
|
||
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 41•8 years ago
|
||
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-
Comment 42•8 years ago
|
||
Mayank -- are you still working on this? It seems very close at this point.
Flags: needinfo?(mayanksri18)
Comment 43•8 years ago
|
||
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•8 years ago
|
||
(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•8 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•8 years ago
|
||
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)
Comment 47•8 years ago
|
||
(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
Comment 48•8 years ago
|
||
(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
Blocks: devtools/auto-completion
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•8 years ago
|
||
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 50•8 years ago
|
||
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•8 years ago
|
Attachment #8828093 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8833442 -
Attachment is obsolete: true
Comment 51•8 years ago
|
||
(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•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
Attaching updated patch.
Thanks.
Attachment #8845045 -
Flags: review?(ttromey)
Comment 55•8 years ago
|
||
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•8 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•8 years ago
|
Attachment #8843714 -
Attachment is obsolete: true
Comment 57•8 years ago
|
||
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•8 years ago
|
||
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)
Comment 59•8 years ago
|
||
Any word on this?
Comment 60•8 years ago
|
||
(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•8 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.
Comment 62•8 years ago
|
||
(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•8 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)
Comment 64•8 years ago
|
||
(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)
Comment 65•8 years ago
|
||
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•8 years ago
|
||
Attached the rebuilt patch.
Flags: needinfo?(mayanksri18)
Attachment #8854907 -
Flags: review?(ttromey)
Comment 67•8 years ago
|
||
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•8 years ago
|
Attachment #8854907 -
Flags: review?(ttromey) → review+
Updated•8 years ago
|
Attachment #8845045 -
Attachment is obsolete: true
Comment 68•8 years ago
|
||
MozReview-Commit-ID: ACRxUEWBbSM
Comment 70•8 years ago
|
||
Updated•8 years ago
|
Attachment #8854907 -
Attachment is obsolete: true
Comment 71•8 years ago
|
||
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•8 years ago
|
Keywords: checkin-needed
Comment 72•8 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
Comment 73•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Assignee | ||
Comment 74•8 years ago
|
||
Thanks for mentoring Tom.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•