Last Comment Bug 752187 - Support unprefixed linear-gradient & radial-gradient
: Support unprefixed linear-gradient & radial-gradient
Status: RESOLVED FIXED
[qa+]
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: mozilla16
Assigned To: Masatoshi Kimura [:emk]
:
Mentors:
: 712474 (view as bug list)
Depends on: 591600 685400 771975 941300
Blocks: unprefix 823673
  Show dependency treegraph
 
Reported: 2012-05-05 02:54 PDT by Jean-Yves Perrier [:teoli]
Modified: 2015-07-14 11:10 PDT (History)
11 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Implement unprefixed gradients (45.72 KB, patch)
2012-05-06 09:44 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
Tests (95.45 KB, patch)
2012-05-06 09:44 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
Part 1: Separate color stops parsing (1.72 KB, patch)
2012-07-02 08:38 PDT, Masatoshi Kimura [:emk]
dbaron: review+
Details | Diff | Review
Part 2: Separate legacy gradient line parsing (5.30 KB, patch)
2012-07-02 08:39 PDT, Masatoshi Kimura [:emk]
dbaron: review+
Details | Diff | Review
Part 3: Separate linear gradient parsing and radial gradient parsing (6.99 KB, patch)
2012-07-02 08:40 PDT, Masatoshi Kimura [:emk]
dbaron: review+
Details | Diff | Review
Part 4: Remove aIsRadial parameter (6.52 KB, patch)
2012-07-02 08:41 PDT, Masatoshi Kimura [:emk]
dbaron: review+
Details | Diff | Review
Part 5: Remove "to" corner parsing from ParseRadialGradient (3.78 KB, patch)
2012-07-02 08:41 PDT, Masatoshi Kimura [:emk]
dbaron: review+
Details | Diff | Review
Part 6: Rename mIsToCorner to mIsLegacySyntax (4.80 KB, patch)
2012-07-02 08:41 PDT, Masatoshi Kimura [:emk]
dbaron: review+
Details | Diff | Review
Part 7: Add aIsLegacy parameter to parsing functions (5.76 KB, patch)
2012-07-02 08:42 PDT, Masatoshi Kimura [:emk]
dbaron: review+
Details | Diff | Review
Part 8: Implement unprefixed linear-gradient parsing (5.21 KB, patch)
2012-07-02 08:42 PDT, Masatoshi Kimura [:emk]
dbaron: review+
Details | Diff | Review
Part 9: Implement unprefixed radial-gradient parsing (17.66 KB, patch)
2012-07-02 08:43 PDT, Masatoshi Kimura [:emk]
dbaron: review+
Details | Diff | Review
Part 10: Implement redering unprefixed gradients (9.85 KB, patch)
2012-07-02 08:43 PDT, Masatoshi Kimura [:emk]
dbaron: review+
Details | Diff | Review
Part 11: Implement serializing unprefixed gradients (8.10 KB, patch)
2012-07-02 08:44 PDT, Masatoshi Kimura [:emk]
dbaron: review+
Details | Diff | Review
Part 12: Implement serializing unprefixed computed values (5.60 KB, patch)
2012-07-02 08:45 PDT, Masatoshi Kimura [:emk]
dbaron: review+
Details | Diff | Review
Tests (95.71 KB, patch)
2012-07-02 08:45 PDT, Masatoshi Kimura [:emk]
dbaron: review+
Details | Diff | Review
Part 1: Separate color stops parsing (2.32 KB, patch)
2012-07-06 18:33 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review
Part 2: Separate legacy gradient line parsing (5.17 KB, patch)
2012-07-06 18:33 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review
Part 3: Separate linear gradient parsing and radial gradient parsing (6.88 KB, patch)
2012-07-06 18:34 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review
Part 4: Remove aIsRadial parameter (6.46 KB, patch)
2012-07-06 18:35 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review
Part 5: Remove "to" corner parsing from ParseRadialGradient (3.72 KB, patch)
2012-07-06 18:35 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review
Part 6: Rename mIsToCorner to mIsLegacySyntax (4.88 KB, patch)
2012-07-06 18:36 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review
Part 7: Add aIsLegacy parameter to parsing functions (5.72 KB, patch)
2012-07-06 18:36 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review
Part 8: Implement unprefixed linear-gradient parsing (5.15 KB, patch)
2012-07-06 18:37 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review
Part 9: Implement unprefixed radial-gradient parsing (18.20 KB, patch)
2012-07-06 18:37 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review
Part 10: Implement rendering unprefixed gradients (9.93 KB, patch)
2012-07-06 18:39 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review
Part 12: Implement serializing unprefixed computed values (5.62 KB, patch)
2012-07-06 18:40 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review
Tests (95.67 KB, patch)
2012-07-06 18:41 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review

Description Jean-Yves Perrier [:teoli] 2012-05-05 02:54:34 PDT
Now that CSS3 Images has reached CR, we should ASAP drop prefix on linear-gradient().

1) Our implementation is fairly close to the final syntax and semantic (mainly need a change in angle direction, I think)
2) We pushed for it at the CSSWG (and accepted heart-breaking decisions like postponing element() to CSS4 Images). We need to act in a coherent way: would be sad to have done this, which impairs our fight against -webkit-box-reflect, and not harvest the positive side of it.
3) k9o-related evangelism need this to be successful. 

(There are so many k9o-related meta bugs that I don't know which one this one should block)
Comment 1 Jason Smith [:jsmith] 2012-05-05 14:24:51 PDT
Gradients are on the list of CSS properties that have high evidence in top sites of webkit-only use (example: foursquare). Nominating for kilimanjaro, as this affects top sites.
Comment 2 :Ms2ger 2012-05-06 06:25:12 PDT
Saw you had a patch on try; hope you don't mind me assigning the bug to you.
Comment 3 Masatoshi Kimura [:emk] 2012-05-06 09:44:14 PDT
Created attachment 621428 [details] [diff] [review]
Implement unprefixed gradients
Comment 4 Masatoshi Kimura [:emk] 2012-05-06 09:44:37 PDT
Created attachment 621429 [details] [diff] [review]
Tests
Comment 5 Masatoshi Kimura [:emk] 2012-05-06 09:46:00 PDT
This patch also contains an implementation of unprefixed radial-gradient().
Comment 6 Masatoshi Kimura [:emk] 2012-05-17 13:06:50 PDT
Win7 debug mochitest-4 output Exceeds 5 MB again...
https://tbpl.mozilla.org/php/getParsedLog.php?id=11775874&tree=Try
Comment 7 Brad Lassey [:blassey] (use needinfo?) 2012-05-30 13:15:07 PDT
blocking on making a decision on whether we'll drop the prefix in time for k9o
Comment 8 Jason Smith [:jsmith] 2012-06-06 16:47:08 PDT
(In reply to Brad Lassey [:blassey] from comment #7)
> blocking on making a decision on whether we'll drop the prefix in time for
> k9o

Tantek - Do you think we would be able to drop the prefix for gradients in time for k9o?
Comment 9 Jason Smith [:jsmith] 2012-06-06 16:47:25 PDT
(In reply to Jason Smith [:jsmith] from comment #8)
> (In reply to Brad Lassey [:blassey] from comment #7)
> > blocking on making a decision on whether we'll drop the prefix in time for
> > k9o
> 
> Tantek - Do you think we would be able to drop the prefix for gradients in
> time for k9o?

Or better question - Should we do this?
Comment 10 Tantek Çelik 2012-06-06 16:50:28 PDT
Yes we should unprefix gradients. The reasoning in Comment 0 (description) is correct.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-14 14:19:37 PDT
Comment on attachment 621428 [details] [diff] [review]
Implement unprefixed gradients

This would have been much easier to review if it had been a patch queue;
the reorganization should not have been in the same patch as the
functional changes.  I keep thinking I'm going to get through it, but I
think it might actually make more sense to try to split the patch up
first.

However, a few comments that I've already noticed:

I don't see the point of nsCSSValueGradient::mRadiusX if it's always a
reference to mRadialSize.  It's just a wasted word of storage.  If you
want something with two names, use a getter function.

It also looks like mRadiusY is also redundant, since you use either
mRadiusY or mRadialShape in the operator==.
Comment 12 Masatoshi Kimura [:emk] 2012-07-02 08:38:43 PDT
Created attachment 638365 [details] [diff] [review]
Part 1: Separate color stops parsing
Comment 13 Masatoshi Kimura [:emk] 2012-07-02 08:39:10 PDT
Created attachment 638366 [details] [diff] [review]
Part 2: Separate legacy gradient line parsing
Comment 14 Masatoshi Kimura [:emk] 2012-07-02 08:40:23 PDT
Created attachment 638367 [details] [diff] [review]
Part 3: Separate linear gradient parsing and radial gradient parsing

This is just copy & paste. Actual changes are included in following patches.
Comment 15 Masatoshi Kimura [:emk] 2012-07-02 08:41:01 PDT
Created attachment 638368 [details] [diff] [review]
Part 4: Remove aIsRadial parameter
Comment 16 Masatoshi Kimura [:emk] 2012-07-02 08:41:29 PDT
Created attachment 638369 [details] [diff] [review]
Part 5: Remove "to" corner parsing from ParseRadialGradient
Comment 17 Masatoshi Kimura [:emk] 2012-07-02 08:41:59 PDT
Created attachment 638370 [details] [diff] [review]
Part 6: Rename mIsToCorner to mIsLegacySyntax
Comment 18 Masatoshi Kimura [:emk] 2012-07-02 08:42:28 PDT
Created attachment 638371 [details] [diff] [review]
Part 7: Add aIsLegacy parameter to parsing functions
Comment 19 Masatoshi Kimura [:emk] 2012-07-02 08:42:56 PDT
Created attachment 638372 [details] [diff] [review]
Part 8: Implement unprefixed linear-gradient parsing
Comment 20 Masatoshi Kimura [:emk] 2012-07-02 08:43:27 PDT
Created attachment 638373 [details] [diff] [review]
Part 9: Implement unprefixed radial-gradient parsing
Comment 21 Masatoshi Kimura [:emk] 2012-07-02 08:43:55 PDT
Created attachment 638374 [details] [diff] [review]
Part 10: Implement redering unprefixed gradients
Comment 22 Masatoshi Kimura [:emk] 2012-07-02 08:44:28 PDT
Created attachment 638375 [details] [diff] [review]
Part 11: Implement serializing unprefixed gradients
Comment 23 Masatoshi Kimura [:emk] 2012-07-02 08:45:03 PDT
Created attachment 638376 [details] [diff] [review]
Part 12: Implement serializing unprefixed computed values
Comment 24 Masatoshi Kimura [:emk] 2012-07-02 08:45:34 PDT
Created attachment 638377 [details] [diff] [review]
Tests
Comment 25 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-02 14:44:31 PDT
Comment on attachment 638366 [details] [diff] [review]
Part 2: Separate legacy gradient line parsing

I don't think you actually need the |return false| behavior here at all; if there's an error the later could should catch it as well.

So I'd prefer to make the return value here be what you have in the |haveGradientLine| out parameter, make the default: case just be "break;", and rename the function to IsGradientLine.  (I don't see how it's specific to being a legacy gradient line.)

I think you should also rename the function's parameters to begin with "a" followed by a capital letter.  (The same for the previous patch as well.)

r=dbaron with that
Comment 26 Masatoshi Kimura [:emk] 2012-07-02 15:06:37 PDT
(In reply to David Baron [:dbaron] from comment #25)
> (I don't see how it's specific
> to being a legacy gradient line.)
Non-legacy syntax parsing will be added in part 8 & 9. The latest gradient-line syntax is different and incompatible from legacy one, so I divided the code.
Comment 27 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-02 15:12:17 PDT
Comment on attachment 638371 [details] [diff] [review]
Part 7: Add aIsLegacy parameter to parsing functions

>+    nsAutoString ident = tk->mIdent;
>+    bool isLegacy = false;
>+    if (StringBeginsWith(ident, NS_LITERAL_STRING("-moz-"))) {
>+      ident.Cut(0, 5);
>+      isLegacy = true;
>+    }
>+    bool isRepeating = false;
>+    if (StringBeginsWith(ident, NS_LITERAL_STRING("repeating-"))) {
>+      ident.Cut(0, 10);
>+      isRepeating = true;
>+    }
>+
>+    if (ident.LowerCaseEqualsLiteral("linear-gradient"))
>+      return ParseLinearGradient(aValue, isRepeating, isLegacy);
>+
>+    if (ident.LowerCaseEqualsLiteral("radial-gradient"))
>+      return ParseRadialGradient(aValue, isRepeating, isLegacy);

Rather than mutating strings, I think this is better to do with dependent strings, like this:

    nsDependentString tmp1(tk->mIdent, 0);
    bool isLegacy = false;
    if (StringBeginsWith(tmp1, NS_LITERAL_STRING("-moz-"))) {
      tmp1.Rebind(tk->mIdent, 5);
      isLegacy = true;
    }

    nsDependentString tmp2(tmp1, 0);
    bool isRepeating = false;
    if (StringBeginsWith(tmp2, NS_LITERAL_STRING("repeating-"))) {
      tmp2.Rebind(tmp1, 10);
      isRepeating = true;
    }

    if (tmp2.LowerCaseEqualsLiteral("linear-gradient"))
       ...

also, please brace (put {} around) the one-line if bodies at the end of the quote above given that you're rewriting them.

r=dbaron with that (if it works, at least, otherwise ok to leave it)
Comment 28 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-02 15:14:42 PDT
(I think it's actually ok to use a single temporary rather than 2, after looking a little more closely at nsTDependentString_CharT::Rebind and nsTSubstring_CharT::Finalize.)
Comment 29 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-02 15:18:28 PDT
(In reply to Masatoshi Kimura [:emk] from comment #26)
> (In reply to David Baron [:dbaron] from comment #25)
> > (I don't see how it's specific
> > to being a legacy gradient line.)
> Non-legacy syntax parsing will be added in part 8 & 9. The latest
> gradient-line syntax is different and incompatible from legacy one, so I
> divided the code.

OK, then leave the function name.  (I still need to understand part 8.)
Comment 30 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-03 15:12:47 PDT
Comment on attachment 638373 [details] [diff] [review]
Part 9: Implement unprefixed radial-gradient parsing

Instead of calling the new constant table kRadialGradientExtentKTable,
rename the old one to kRadialGradientLegacySizeKTable and reuse the
old name (switching all uses).

>+  cssGradient->GetRadialShape().SetNoneValue();

>+  cssGradient->GetRadialSize().SetNoneValue();

you don't need to do these explicitly; it's already none from the constructor


>+    haveSize =
>+      ParseNonNegativeVariant(cssGradient->GetRadiusX(), VARIANT_LP, nsnull);
>+    if (haveSize) {
>+      // vertical extent is optional
>+      bool haveYSize =
>+        ParseNonNegativeVariant(cssGradient->GetRadiusY(), VARIANT_LP, nsnull);
>+      if (!haveShape) {
>+        haveShape = ParseVariant(cssGradient->GetRadialShape(), VARIANT_KEYWORD,
>+                                 nsCSSProps::kRadialGradientShapeKTable);

This overwrites the RadiusX with the RadialShape.  However, you don't
need to store the shape value here -- you only need to check that it's
'circle' if haveYSize is false and 'ellipse' if haveYSize is true.  So
you should put it in a different variable.

>+      if (!haveYSize &&
>+            (cssGradient->GetRadiusX().GetUnit() == eCSSUnit_Percent ||
>+             shape == NS_STYLE_GRADIENT_SHAPE_ELLIPTICAL) ||
>+          haveYSize && shape == NS_STYLE_GRADIENT_SHAPE_CIRCULAR) {

I think this would actually be easier to read with a ternary operator:

      if (haveYSize
            ? shape == NS_STYLE_GRADIENT_SHAPE_CIRCULAR
            : cssGradient->GetRadiusX().GetUnit() == eCSSUnit_Percent ||
              shape == NS_STYLE_GRADIENT_SHAPE_ELLIPTICAL) {


r=dbaron with those things fixed
Comment 31 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-03 15:21:25 PDT
Comment on attachment 638374 [details] [diff] [review]
Part 10: Implement redering unprefixed gradients

nsRuleNode.cpp:

>+    SetCoord(gradient->GetRadiusX(), aResult.mRadiusX, nsStyleCoord(),
>+             SETCOORD_LPO | SETCOORD_STORE_CALC,
>+             aContext, aPresContext, aCanStoreInRuleTree);
>+    if (gradient->GetRadiusY().GetUnit() != eCSSUnit_None) {
>+      SetCoord(gradient->GetRadiusY(), aResult.mRadiusY, nsStyleCoord(),
>+               SETCOORD_LPO | SETCOORD_STORE_CALC,
>+               aContext, aPresContext, aCanStoreInRuleTree);
>+      aResult.mShape = NS_STYLE_GRADIENT_SHAPE_ELLIPTICAL;
>+    } else {
>+      aResult.mShape = NS_STYLE_GRADIENT_SHAPE_CIRCULAR;
>+    }
>+    aResult.mSize = NS_STYLE_GRADIENT_SIZE_EXPLICIT_SIZE;

These should both use SETCOORD_LP rather than SETCOORD_LPO.  You
don't need to handle eCSSUnit_None.

You also need to set aResult.mRadiusY to mRadiusX when the Y unit is 
none.  (You should add a test that catches the resulting misrendering.)

r=dbaron with those things fixed
Comment 32 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-05 14:47:36 PDT
Comment on attachment 638376 [details] [diff] [review]
Part 12: Implement serializing unprefixed computed values

>+    } else if (aGradient->mBgPosX.GetUnit() != eStyleUnit_Percent ||
>+        aGradient->mBgPosX.GetPercentValue() != 0.5f ||
>+        aGradient->mBgPosY.GetUnit() != eStyleUnit_Percent ||
>+        aGradient->mBgPosY.GetPercentValue() != (isRadial ? 0.5f : 1.0f)) {

I'm a little scared of adding these conditions, but it is the right thing to do, and they look correct, so I guess it's best to keep them.

Fix the indent here too.

r=dbaron with that
Comment 33 Masatoshi Kimura [:emk] 2012-07-06 18:33:25 PDT
Created attachment 639882 [details] [diff] [review]
Part 1: Separate color stops parsing
Comment 34 Masatoshi Kimura [:emk] 2012-07-06 18:33:56 PDT
Created attachment 639883 [details] [diff] [review]
Part 2: Separate legacy gradient line parsing
Comment 35 Masatoshi Kimura [:emk] 2012-07-06 18:34:36 PDT
Created attachment 639884 [details] [diff] [review]
Part 3: Separate linear gradient parsing and radial gradient parsing
Comment 36 Masatoshi Kimura [:emk] 2012-07-06 18:35:16 PDT
Created attachment 639885 [details] [diff] [review]
Part 4: Remove aIsRadial parameter
Comment 37 Masatoshi Kimura [:emk] 2012-07-06 18:35:51 PDT
Created attachment 639887 [details] [diff] [review]
Part 5: Remove "to" corner parsing from ParseRadialGradient
Comment 38 Masatoshi Kimura [:emk] 2012-07-06 18:36:23 PDT
Created attachment 639888 [details] [diff] [review]
Part 6: Rename mIsToCorner to mIsLegacySyntax
Comment 39 Masatoshi Kimura [:emk] 2012-07-06 18:36:50 PDT
Created attachment 639889 [details] [diff] [review]
Part 7: Add aIsLegacy parameter to parsing functions
Comment 40 Masatoshi Kimura [:emk] 2012-07-06 18:37:17 PDT
Created attachment 639890 [details] [diff] [review]
Part 8: Implement unprefixed linear-gradient parsing
Comment 41 Masatoshi Kimura [:emk] 2012-07-06 18:37:46 PDT
Created attachment 639891 [details] [diff] [review]
Part 9: Implement unprefixed radial-gradient parsing
Comment 42 Masatoshi Kimura [:emk] 2012-07-06 18:39:34 PDT
Created attachment 639892 [details] [diff] [review]
Part 10: Implement rendering unprefixed gradients
Comment 43 Masatoshi Kimura [:emk] 2012-07-06 18:40:26 PDT
Created attachment 639893 [details] [diff] [review]
Part 12: Implement serializing unprefixed computed values
Comment 44 Masatoshi Kimura [:emk] 2012-07-06 18:41:06 PDT
Created attachment 639894 [details] [diff] [review]
Tests
Comment 45 Masatoshi Kimura [:emk] 2012-07-06 18:43:46 PDT
All review comments are resolved.

(In reply to David Baron [:dbaron] from comment #31)
> You also need to set aResult.mRadiusY to mRadiusX when the Y unit is 
> none.  (You should add a test that catches the resulting misrendering.)
Modified radial-shape-*-1b.html to catch this error.
Comment 48 j.j. 2012-07-21 11:08:09 PDT
Dão, why did you remove dependency to bug 776225?
It's somewhat common to do this. E.g. is makes it easy to find the right bug to resolve future duplicates.
Comment 49 Dão Gottwald [:dao] 2012-07-21 11:10:00 PDT
(In reply to j.j. (inactive in 2012) from comment #48)
> Dão, why did you remove dependency to bug 776225?

Because that bug is invalid anyway and the extra bugmail due to the resolved/reopened fights is annoying and unnecessary.
Comment 50 j.j. 2012-07-21 11:23:28 PDT
Calculating your annoyance against bug triager's convenience and accuracy is an unsolved equation.
Comment 51 Dão Gottwald [:dao] 2012-07-21 11:26:41 PDT
I don't see the accuracy. The bug is invalid, tracking it as a dependency isn't useful. Feel free to add it to the "blocks" list, though.
Comment 52 Masatoshi Kimura [:emk] 2012-07-23 23:34:04 PDT
*** Bug 712474 has been marked as a duplicate of this bug. ***
Comment 53 Daniel Holbert [:dholbert] 2015-07-14 11:10:48 PDT
--> Updating summary to make this bug more clearly distinguishable from bug 1176496 (which is where we actually drop support for prefixed gradients).

Note You need to log in before you can comment on or make changes to this bug.