Closed
Bug 1302787
Opened 8 years ago
Closed 8 years ago
add devtools support for css-color-4
Categories
(DevTools :: Inspector, enhancement, P2)
DevTools
Inspector
Tracking
(firefox52 fixed)
RESOLVED
FIXED
Firefox 52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: tromey, Assigned: jerry)
References
()
Details
Attachments
(5 files, 12 obsolete files)
1.28 KB,
patch
|
tromey
:
review+
|
Details | Diff | Splinter Review |
1.13 KB,
patch
|
tromey
:
review+
|
Details | Diff | Splinter Review |
10.57 KB,
patch
|
Details | Diff | Splinter Review | |
3.03 KB,
patch
|
Details | Diff | Splinter Review | |
16.29 KB,
patch
|
Details | Diff | Splinter Review |
Bug 1295456 is implementing css-color-4. This adds new color function syntax
to CSS. devtools will need some updates for this; at least css-color.js, but
perhaps some other things as well.
Comment 1•8 years ago
|
||
Inspector bug triage (filter on CLIMBING SHOES).
Severity: normal → enhancement
Priority: -- → P2
Assignee | ||
Comment 2•8 years ago
|
||
There are some devtool testings failed with the bug 1295456 css-color-4 implementation. I try to update our devtool to pass the testings.
Assignee: nobody → hshih
Status: NEW → ASSIGNED
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 3•8 years ago
|
||
This is the js version of css-color-4 color function implementation from bug 1295456.
Attachment #8798746 -
Flags: review?(ttromey)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8798747 -
Flags: review?(ttromey)
Assignee | ||
Comment 5•8 years ago
|
||
fix the wrong hsl() to rgba value.
Attachment #8798800 -
Flags: review?(ttromey)
Assignee | ||
Updated•8 years ago
|
Attachment #8798746 -
Attachment is obsolete: true
Attachment #8798746 -
Flags: review?(ttromey)
Assignee | ||
Comment 6•8 years ago
|
||
The "rgb(24, 25, 45, 1)" is valid now. Turn to use "rgb(24, 25%, 45, 1)".
Attachment #8798801 -
Flags: review?(ttromey)
Reporter | ||
Comment 7•8 years ago
|
||
Comment on attachment 8798800 [details] [diff] [review]
P1. Implement css-color-4 color function changes in devtool parser. v2
Review of attachment 8798800 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you for doing this. I have a number of comments, but they're generally nit-picking; I think this patch looks good overall.
However, I have one bigger concern. This patch unconditionally changes devtools to support the new color style.
That's correct when debugging a current firefox - but not when debugging an older version (or when using valence
to debug some other browser).
I think we're going to need to add an attribute on the css properties actor (perhaps in the db) and then have the inspector
adapt. This means that the parsing functions are going to need a mode where they reject css-4 colors.
There's also the question of whether the color-cycling code (in CssColor) should try to preserve the comma-less
state of its input.
I'm sure you didn't think you were signing up to completely fix devtools for css-4 colors :)
So I would suggest fixing up your current patches and adding the new parsing mode.
If you make the mode default to old-style but change the test to use the new style, then maybe
that will let all the inspector tests continue to work? And then I can fix up the remaining issues
in a follow-up bug.
If you're feeling motivated to fix everything of course I won't stop you :)
I can provide some pointers in this case.
I'm going to "blank" this review considering it needs some more work.
::: devtools/shared/css/color.js
@@ +5,5 @@
> "use strict";
>
> const Services = require("Services");
>
> +const {angleUtils} = require("devtools/client/shared/css-angle");
Files in devtools/shared can't require files from devtools/client, so this will need to be done some other way.
I tend to think this require isn't needed, see below.
@@ +650,5 @@
> * @return {CSSToken} The next non-whitespace, non-comment token; or
> * null at EOF.
> */
> function getToken(lexer) {
> + if (lexer.mHasPushBackToken) {
devtools doesn't generally use the mMumble style. Please rename this property. I think "_hasPushBackToken" would be fine.
@@ +651,5 @@
> * null at EOF.
> */
> function getToken(lexer) {
> + if (lexer.mHasPushBackToken) {
> + if (lexer.mCurrentToken && lexer.mCurrentToken.tokenType !== "comment" &&
This logic looks suspect to me. First, can the current token here ever be "comment" or "whitespace"? I couldn't see how.
Second, if there is a pushback token, and it is one of these (or null, meaning EOF), then this code will fall through to the ordinary parsing loop without resetting mHasPushbackToken.
I think it's probably better to just remove this "if".
@@ +676,5 @@
> + * @param {CSSLexer} lexer The lexer
> + */
> +function unGetToken(lexer) {
> + if (lexer.mHasPushBackToken) {
> + alert('double pushback');
Throw an exception here, I don't think we use alert in devtools. Also, we don't use single quotes; this is enforced by eslint.
@@ +684,5 @@
> +
> +/**
> + * A helper function to exame a symbol is exist or not.
> + *
> + * @param {CSSLexer} lexer The lexer
This should have an @param line for "symbol" as well.
@@ +687,5 @@
> + *
> + * @param {CSSLexer} lexer The lexer
> + * @return {Boolean} The expect symbol is parsed or not.
> + */
> +function ExpectSymbol(lexer, symbol) {
Since this isn't a constructor the name should start with a lower-case letter.
@@ +808,5 @@
> + if (token.tokenType === "number") {
> + val = token.number;
> + } else if (token.tokenType === "dimension") {
> + let angleValidator = new angleUtils.CssAngle(token.number + token.text);
> + if (!angleValidator.valid) {
CssAngle.valid boils down to a simple check:
return (token.tokenType === "dimension"
&& token.text.toLowerCase() in CssAngle.ANGLEUNIT);
... but also instantiates a new lexer, which seems excessive here, considering that this function already has a token.
I appreciate the desire for DRY; if you want to preserve that then perhaps moving ANGLEUNIT to devtools/shared/css/properties-db would be appropriate.
@@ +855,2 @@
>
> + let commaSeparator = ",";
I think "const" here would be clearer. Also for hasComma and separatorBeforeAlpha.
@@ +891,5 @@
> + // comma-less expression:
> + // rgb() = rgb( component{3} [ / <alpha-value> ]? )
> + // the expression with comma:
> + // rgb() = rgb( component#{3} , <alpha-value>? )
> + let commaSeparator = ",";
The same "const" treatment in this function too.
Attachment #8798800 -
Flags: review?(ttromey)
Reporter | ||
Comment 8•8 years ago
|
||
Comment on attachment 8798747 [details] [diff] [review]
P2. remove the now-unused requireComma() function in devtool colorUtils. v1
Review of attachment 8798747 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you. This is good.
Attachment #8798747 -
Flags: review?(ttromey) → review+
Reporter | ||
Comment 9•8 years ago
|
||
Comment on attachment 8798801 [details] [diff] [review]
P3. set a new invalid rgb color value for devtool test case.
Review of attachment 8798801 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you. This looks good.
Attachment #8798801 -
Flags: review?(ttromey) → review+
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Tom Tromey :tromey from comment #7)
> Comment on attachment 8798800 [details] [diff] [review]
> P1. Implement css-color-4 color function changes in devtool parser. v2
>
> Review of attachment 8798800 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Thank you for doing this. I have a number of comments, but they're
> generally nit-picking; I think this patch looks good overall.
>
> However, I have one bigger concern. This patch unconditionally changes
> devtools to support the new color style.
> That's correct when debugging a current firefox - but not when debugging an
> older version (or when using valence
> to debug some other browser).
Can we use firefox to debug other browser
>
> I think we're going to need to add an attribute on the css properties actor
> (perhaps in the db) and then have the inspector
> adapt. This means that the parsing functions are going to need a mode where
> they reject css-4 colors.
>
Could you tell more about this? Do you mean that have a pref for the new mode parser?
> There's also the question of whether the color-cycling code (in CssColor)
> should try to preserve the comma-less
> state of its input.
>
What's color-cycling code? Is it related to animation?
https://en.wikipedia.org/wiki/Color_cycling
> I'm sure you didn't think you were signing up to completely fix devtools for
> css-4 colors :)
> So I would suggest fixing up your current patches and adding the new parsing
> mode.
> If you make the mode default to old-style but change the test to use the new
> style, then maybe
> that will let all the inspector tests continue to work? And then I can fix
> up the remaining issues
> in a follow-up bug.
>
> If you're feeling motivated to fix everything of course I won't stop you :)
> I can provide some pointers in this case.
>
> I'm going to "blank" this review considering it needs some more work.
>
> ::: devtools/shared/css/color.js
> @@ +5,5 @@
> > "use strict";
> >
> > const Services = require("Services");
> >
> > +const {angleUtils} = require("devtools/client/shared/css-angle");
>
> Files in devtools/shared can't require files from devtools/client, so this
> will need to be done some other way.
>
> I tend to think this require isn't needed, see below.
>
I still try to share the css-angle parsing code for both client side and the color parser in devtools/shared/css/color.js .
How about move devtools/client/shared/css-angle.js to devtools/shared/css/ folder?
> @@ +808,5 @@
> > + if (token.tokenType === "number") {
> > + val = token.number;
> > + } else if (token.tokenType === "dimension") {
> > + let angleValidator = new angleUtils.CssAngle(token.number + token.text);
> > + if (!angleValidator.valid) {
>
> CssAngle.valid boils down to a simple check:
>
> return (token.tokenType === "dimension"
> && token.text.toLowerCase() in CssAngle.ANGLEUNIT);
>
> ... but also instantiates a new lexer, which seems excessive here,
> considering that this function already has a token.
>
> I appreciate the desire for DRY; if you want to preserve that then perhaps
> moving ANGLEUNIT to devtools/shared/css/properties-db would be appropriate.
>
Yes, I think we could to that. But, how about the conversion of various angle units to degree?
That code is already existed in css-angle.js. We might still have the duplicate code here.
Flags: needinfo?(ttromey)
Reporter | ||
Comment 11•8 years ago
|
||
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #10)
> Can we use firefox to debug other browser
Yes, see https://developer.mozilla.org/en-US/docs/Tools/Valence
> Could you tell more about this? Do you mean that have a pref for the new
> mode parser?
No, not a pref, just a parameter to the parsing functions.
It might be sufficient here to have the tests parse both ways (correctly
rejecting css-4 colors when appropriate); and then not worry about the
bigger devtools changes.
> > There's also the question of whether the color-cycling code (in CssColor)
> > should try to preserve the comma-less
> > state of its input.
> >
>
> What's color-cycling code? Is it related to animation?
> https://en.wikipedia.org/wiki/Color_cycling
In the inspector if you shift-click on a color swatch, it cycles through
different color representations, like name->hex->rgb->hsl.
The question here is whether this should try to preserve the spelling.
> I still try to share the css-angle parsing code for both client side and the
> color parser in devtools/shared/css/color.js .
> How about move devtools/client/shared/css-angle.js to devtools/shared/css/
> folder?
That's fine provided the resulting patch isn't instantiating a new lexer
just for this one use.
> Yes, I think we could to that. But, how about the conversion of various
> angle units to degree?
> That code is already existed in css-angle.js. We might still have the
> duplicate code here.
Yeah, if that's needed, it's fine to move the file. Or you could put
helper functions somewhere, like css/parsing-utils.js.
Flags: needinfo?(ttromey)
Assignee | ||
Comment 12•8 years ago
|
||
update for comment 7.
I will add an argument for the color parser to accept the old style syntax only in next patch.
Attachment #8800068 -
Flags: review?(ttromey)
Assignee | ||
Updated•8 years ago
|
Attachment #8798800 -
Attachment is obsolete: true
Assignee | ||
Comment 13•8 years ago
|
||
update for comment 7.
update for lint checking failed.
I will add an argument for the color parser to accept the old style syntax only in next patch.
Attachment #8800115 -
Flags: review?(ttromey)
Assignee | ||
Updated•8 years ago
|
Attachment #8800068 -
Attachment is obsolete: true
Attachment #8800068 -
Flags: review?(ttromey)
Assignee | ||
Comment 14•8 years ago
|
||
update hsl/rgb color function syntax in comment.
Attachment #8800246 -
Flags: review?(ttromey)
Assignee | ||
Updated•8 years ago
|
Attachment #8800115 -
Attachment is obsolete: true
Attachment #8800115 -
Flags: review?(ttromey)
Assignee | ||
Comment 15•8 years ago
|
||
Attachment #8800353 -
Flags: review?(ttromey)
Assignee | ||
Updated•8 years ago
|
Attachment #8800353 -
Attachment is obsolete: true
Attachment #8800353 -
Flags: review?(ttromey)
Assignee | ||
Comment 17•8 years ago
|
||
update for lint failed.
Attachment #8800503 -
Flags: review?(ttromey)
Assignee | ||
Updated•8 years ago
|
Attachment #8800246 -
Attachment is obsolete: true
Attachment #8800246 -
Flags: review?(ttromey)
Assignee | ||
Comment 18•8 years ago
|
||
update for lint failed
Attachment #8800504 -
Flags: review?(ttromey)
Assignee | ||
Updated•8 years ago
|
Attachment #8800493 -
Attachment is obsolete: true
Attachment #8800493 -
Flags: review?(ttromey)
Reporter | ||
Comment 19•8 years ago
|
||
Comment on attachment 8800503 [details] [diff] [review]
P1. Implement css-color-4 color function changes in devtool parser. v6
Review of attachment 8800503 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you. This looks good -- some minor nits, nothing serious.
::: devtools/shared/css/color.js
@@ +6,5 @@
>
> const Services = require("Services");
>
> +const {CSS_ANGLEUNIT} = require("devtools/shared/css/properties-db");
> +const {getAngleValueInDegrees} = require("devtools/shared/css/parsing-utils.js");
Remove the ".js" here.
@@ +681,5 @@
> + lexer._hasPushBackToken = true;
> +}
> +
> +/**
> + * A helper function to exame a symbol is exist or not.
Typo in "examine". But I think this should probably read more like:
A helper function that checks if the next token matches symbol.
If so, reads the token and returns true. If not, pushes the
token back and returns false.
::: devtools/shared/css/parsing-utils.js
@@ +1134,5 @@
> priority: declaration ? declaration.priority : ""
> };
> }
>
> +function getAngleValueInDegrees(angleValue, angleUnit) {
Please add a jsdoc comment describing the function,
arguments, and return value.
Attachment #8800503 -
Flags: review?(ttromey) → review+
Reporter | ||
Comment 20•8 years ago
|
||
Comment on attachment 8800504 [details] [diff] [review]
P4. add a mode for css-color-4 color parser to accept the old style syntax only. v3
Review of attachment 8800504 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you for doing this. This looks basically good, though I found one spot I think is in error.
Also I wonder what the test plan is for the "old-style" mode.
::: devtools/shared/css/color.js
@@ +1017,5 @@
> +
> + // Parse G, B and A.
> + // No need to check the separator after 'B'. It will be checked in 'A' values
> + // parsing.
> + let separatorBeforeAlpha = hasComma ? commaSeparator : "/";
This doesn't look like old-style rgb.
@@ +1040,5 @@
> * @param {String} name the color
> * @return {Object} an object of the form {r, g, b, a}; or null if the
> * name was not a valid color
> */
> +function colorToRGBA(name, oldColorFunctionSyntax = true) {
This needs a new @param comment explaining what the parameter means.
Attachment #8800504 -
Flags: review?(ttromey)
Assignee | ||
Comment 21•8 years ago
|
||
fix the bug for the invalid tailing token.
Assignee | ||
Updated•8 years ago
|
Attachment #8800503 -
Attachment is obsolete: true
Assignee | ||
Comment 22•8 years ago
|
||
Update for comment 20.
Rewrite parseOldStyleHsl and parseOldStyleRgb. there is a logic bug for the main if condition.
And we can't use parseColorComponent() for the old style hue component parsing. The function output is [0,255].
Update parseColorComponent() comment
Attachment #8801062 -
Flags: review?(ttromey)
Assignee | ||
Updated•8 years ago
|
Attachment #8800504 -
Attachment is obsolete: true
Assignee | ||
Comment 23•8 years ago
|
||
the test case for P4.
Attachment #8801064 -
Flags: review?(ttromey)
Reporter | ||
Comment 24•8 years ago
|
||
Comment on attachment 8801058 [details] [diff] [review]
P1. implement css-color-4 color function changes in devtool parser. v7. r=ttromey
Review of attachment 8801058 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks!
Attachment #8801058 -
Flags: review+
Reporter | ||
Comment 25•8 years ago
|
||
Comment on attachment 8801062 [details] [diff] [review]
P4. add a mode for css-color-4 color parser to accept the old style syntax only. v4
Review of attachment 8801062 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you. This looks good to me.
Attachment #8801062 -
Flags: review?(ttromey) → review+
Reporter | ||
Comment 26•8 years ago
|
||
Comment on attachment 8801064 [details] [diff] [review]
P5. add a test for old-style and css-color-4 color function syntax in devtool. v1
Review of attachment 8801064 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you. This is very nice.
Attachment #8801064 -
Flags: review?(ttromey) → review+
Assignee | ||
Comment 27•8 years ago
|
||
update for lint checking failed
Assignee | ||
Updated•8 years ago
|
Attachment #8801062 -
Attachment is obsolete: true
Assignee | ||
Comment 28•8 years ago
|
||
The previous test case is failed. I'm trying to test with another value.
It might be the rgb->hsl rounding problem. The devtool's output rgb value is only 1 unit different from gecko's rgb value (e.g. rgb(12,13,14) vs rgb(13,13,14)). If this is a big problem, I think we could fix that in another bug.
Assignee | ||
Updated•8 years ago
|
Attachment #8801064 -
Attachment is obsolete: true
Assignee | ||
Comment 29•8 years ago
|
||
Hi Tom,
In attachment 8801530 [details] [diff] [review], the default mode is setting for old-style.
In comment 11:
"No, not a pref, just a parameter to the parsing functions.
It might be sufficient here to have the tests parse both ways (correctly
rejecting css-4 colors when appropriate); and then not worry about the
bigger devtools changes."
Do you have any idea that how to turn on the new css-color-4 in devtool?
What do you think about creating a option in devtool setting page?
I'm not familiar with devtool, could you please fire the bug for these works?
And could you please also check comment 28?
Flags: needinfo?(ttromey)
Assignee | ||
Comment 30•8 years ago
|
||
update for lint checking failed.
Assignee | ||
Updated•8 years ago
|
Attachment #8801058 -
Attachment is obsolete: true
Assignee | ||
Comment 31•8 years ago
|
||
Please land these patch to m-c.
https://bugzilla.mozilla.org/attachment.cgi?id=8801553
https://bugzilla.mozilla.org/attachment.cgi?id=8798747
https://bugzilla.mozilla.org/attachment.cgi?id=8798801
https://bugzilla.mozilla.org/attachment.cgi?id=8801530
https://bugzilla.mozilla.org/attachment.cgi?id=8801531
try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40aa99af1f3d&selectedJob=29218925&group_state=expanded
Keywords: checkin-needed
Comment 32•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02c03ca774ee
Implement css-color-4 color function changes in devtool parser. r=tromey
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b3435dc45f8
Remove the now-unused requireComma() function in devtool colorUtils. r=tromey
https://hg.mozilla.org/integration/mozilla-inbound/rev/8597a7e99ac8
Set a new invalid rgb color value for devtool test case. r=tromey
https://hg.mozilla.org/integration/mozilla-inbound/rev/281ed6b1d3b4
Add a mode for css-color-4 color parser to accept the old style syntax only. r=tromey
https://hg.mozilla.org/integration/mozilla-inbound/rev/554359eddb93
Add a test for old-style and css-color-4 color function syntax in devtool. r=tromey
Keywords: checkin-needed
Comment 33•8 years ago
|
||
Sorry had to backed out this for lint failure, e.g., https://treeherder.mozilla.org/logviewer.html#?job_id=37754266&repo=mozilla-inbound#L4041
Flags: needinfo?(hshih)
Comment 34•8 years ago
|
||
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4c9d1de3534
Backed out changeset 554359eddb93 for lint failure
https://hg.mozilla.org/integration/mozilla-inbound/rev/b10b8e054d9f
Backed out changeset 281ed6b1d3b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c6fa8b34a0f
Backed out changeset 8597a7e99ac8
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f45b0e01383
Backed out changeset 0b3435dc45f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/9707f0972299
Backed out changeset 02c03ca774ee
Comment 35•8 years ago
|
||
Pushed by hshih@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bc049fc9f8c
implement css-color-4 color function changes in devtool parser. r=ttromey
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b1aa4e34f14
remove the now-unused requireComma() function in devtool colorUtils. r=ttromey
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3994652649a
set a new invalid rgb color value for devtool test case. r=ttromey
https://hg.mozilla.org/integration/mozilla-inbound/rev/acf3d3d8766d
add a mode for css-color-4 color parser to accept the old style syntax only. r=ttromey
https://hg.mozilla.org/integration/mozilla-inbound/rev/5820cd95d563
add a test for old-style and css-color-4 color function syntax in devtool. r=ttromey
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(hshih)
Reporter | ||
Comment 36•8 years ago
|
||
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #29)
> Do you have any idea that how to turn on the new css-color-4 in devtool?
> What do you think about creating a option in devtool setting page?
> I'm not familiar with devtool, could you please fire the bug for these works?
>
> And could you please also check comment 28?
I've filed follow-up bugs for both of these.
Flags: needinfo?(ttromey)
Comment 37•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4bc049fc9f8c
https://hg.mozilla.org/mozilla-central/rev/2b1aa4e34f14
https://hg.mozilla.org/mozilla-central/rev/c3994652649a
https://hg.mozilla.org/mozilla-central/rev/acf3d3d8766d
https://hg.mozilla.org/mozilla-central/rev/5820cd95d563
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
No longer depends on: 1347164
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•