Closed Bug 752187 Opened 12 years ago Closed 12 years ago

Support unprefixed linear-gradient & radial-gradient

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16
blocking-kilimanjaro +

People

(Reporter: teoli, Assigned: emk)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [qa+])

Attachments

(13 files, 14 obsolete files)

8.10 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.32 KB, patch
emk
: review+
Details | Diff | Splinter Review
5.17 KB, patch
emk
: review+
Details | Diff | Splinter Review
6.88 KB, patch
emk
: review+
Details | Diff | Splinter Review
6.46 KB, patch
emk
: review+
Details | Diff | Splinter Review
3.72 KB, patch
emk
: review+
Details | Diff | Splinter Review
4.88 KB, patch
emk
: review+
Details | Diff | Splinter Review
5.72 KB, patch
emk
: review+
Details | Diff | Splinter Review
5.15 KB, patch
emk
: review+
Details | Diff | Splinter Review
18.20 KB, patch
emk
: review+
Details | Diff | Splinter Review
9.93 KB, patch
emk
: review+
Details | Diff | Splinter Review
5.62 KB, patch
emk
: review+
Details | Diff | Splinter Review
95.67 KB, patch
emk
: review+
Details | Diff | Splinter Review
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)
OS: Mac OS X → All
Hardware: x86 → All
Summary: Drop prefix for linear-gradients() → Drop prefix for linear-gradient()
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.
Saw you had a patch on try; hope you don't mind me assigning the bug to you.
Assignee: nobody → VYV03354
Attached patch Implement unprefixed gradients (obsolete) — Splinter Review
Attachment #621428 - Flags: review?(dbaron)
Attached patch Tests (obsolete) — Splinter Review
Attachment #621429 - Flags: review?(dbaron)
This patch also contains an implementation of unprefixed radial-gradient().
Summary: Drop prefix for linear-gradient() → Drop prefix for gradients
Win7 debug mochitest-4 output Exceeds 5 MB again...
https://tbpl.mozilla.org/php/getParsedLog.php?id=11775874&tree=Try
blocking on making a decision on whether we'll drop the prefix in time for k9o
blocking-kilimanjaro: ? → +
(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?
(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?
Yes we should unprefix gradients. The reasoning in Comment 0 (description) is correct.
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==.
Attachment #621428 - Attachment is obsolete: true
Attachment #621428 - Flags: review?(dbaron)
Attachment #638365 - Flags: review?(dbaron)
Attachment #638366 - Flags: review?(dbaron)
This is just copy & paste. Actual changes are included in following patches.
Attachment #638367 - Flags: review?(dbaron)
Attachment #638368 - Flags: review?(dbaron)
Attachment #638370 - Flags: review?(dbaron)
Attachment #638371 - Flags: review?(dbaron)
Attachment #638372 - Flags: review?(dbaron)
Attachment #638373 - Flags: review?(dbaron)
Attachment #638374 - Flags: review?(dbaron)
Attachment #638376 - Flags: review?(dbaron)
Attached patch Tests (obsolete) — Splinter Review
Attachment #621429 - Attachment is obsolete: true
Attachment #621429 - Flags: review?(dbaron)
Attachment #638377 - Flags: review?(dbaron)
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
Attachment #638366 - Flags: review?(dbaron) → review+
(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 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)
Attachment #638371 - Flags: review?(dbaron) → review+
(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.)
(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 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
Attachment #638373 - Flags: review?(dbaron) → review+
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
Attachment #638374 - Flags: review?(dbaron) → review+
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
Attachment #638376 - Flags: review?(dbaron) → review+
Attachment #638365 - Attachment is obsolete: true
Attachment #639882 - Flags: review+
Attachment #638366 - Attachment is obsolete: true
Attachment #639883 - Flags: review+
Attachment #638368 - Attachment is obsolete: true
Attachment #639885 - Flags: review+
Attachment #638369 - Attachment is obsolete: true
Attachment #639887 - Flags: review+
Attachment #638370 - Attachment is obsolete: true
Attachment #639888 - Flags: review+
Attachment #638371 - Attachment is obsolete: true
Attachment #639889 - Flags: review+
Attachment #638372 - Attachment is obsolete: true
Attachment #639890 - Flags: review+
Attachment #638373 - Attachment is obsolete: true
Attachment #639891 - Flags: review+
Attachment #638374 - Attachment is obsolete: true
Attachment #639892 - Flags: review+
Attachment #638376 - Attachment is obsolete: true
Attachment #639893 - Flags: review+
Attached patch TestsSplinter Review
Attachment #638377 - Attachment is obsolete: true
Attachment #639894 - Flags: review+
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.
Keywords: checkin-needed
Whiteboard: [qa+]
Target Milestone: --- → mozilla16
Depends on: 771975
Depends on: 591600
Blocks: 772342
Blocks: unprefix
Depends on: 776225
No longer depends on: 776225
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.
(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.
Calculating your annoyance against bug triager's convenience and accuracy is an unsolved equation.
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.
Blocks: 823673
No longer blocks: 772342
Depends on: 941300
--> Updating summary to make this bug more clearly distinguishable from bug 1176496 (which is where we actually drop support for prefixed gradients).
Summary: Drop prefix for gradients → Support unprefixed linear-gradient & radial-gradient
Depends on: 1391534
You need to log in before you can comment on or make changes to this bug.