Closed
Bug 264132
Opened 20 years ago
Closed 18 years ago
Implement fallback for SVG paint servers
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
People
(Reporter: scootermorris, Assigned: longsonr)
References
Details
Attachments
(2 files, 4 obsolete files)
18 years ago
(deleted),
patch
|
tor
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
The SVG 1.1 specification indicates that authors should provide fallback colors
when specifying URI's for paint servers in case there is an error in processing
the target uri. See http://www.w3.org/TR/SVG11/painting.html#SpecifyingPaint
for details. The Mozilla SVG Gradient implementation does not parse the
fallback information.
Reporter | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Comment 1•20 years ago
|
||
See the end of bug 244917 comment 107 and bug 244917 comment 108.
Assignee | ||
Comment 2•18 years ago
|
||
*** Bug 363560 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 3•18 years ago
|
||
Would you mind if I had a go at this Scooter?
Reporter | ||
Comment 4•18 years ago
|
||
Absolutely not! I'd be happy to have you take this on.
Assignee | ||
Comment 5•18 years ago
|
||
Assignee: scootermorris → longsonr
Assignee | ||
Comment 6•18 years ago
|
||
Comment on attachment 248499 [details] [diff] [review]
patch
Note that stop-color is just a colour according to the SVG spec.
Attachment #248499 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•18 years ago
|
||
Comment on attachment 248499 [details] [diff] [review]
patch
oops. Doesn't work in all circumstances.
Attachment #248499 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 8•18 years ago
|
||
Previous patch would not reserialise properly. It put normal fill/stroke colours in twice.
Assignee | ||
Updated•18 years ago
|
Attachment #248499 -
Attachment is obsolete: true
Assignee | ||
Updated•18 years ago
|
Attachment #248519 -
Flags: review?(bzbarsky)
Comment 9•18 years ago
|
||
I'm pretty swamped with existing reviews; you really want someone else if you want the review before February.
Assignee | ||
Comment 10•18 years ago
|
||
Attachment #248519 -
Attachment is obsolete: true
Attachment #248635 -
Flags: review?(dbaron)
Attachment #248519 -
Flags: review?(bzbarsky)
Comment 11•18 years ago
|
||
Comment on attachment 248635 [details] [diff] [review]
updated patch
>+PRBool CSSParserImpl::ParsePaint(nsresult& aErrorCode,
>+ nsCSSValuePair* aResult,
>+ nsCSSProperty aPropID)
This could be significantly simplified by using early returns for failure cases and coalescing the ending parts (see below). You also don't need to use temporaries in case of failure since TransferTempData / ClearTempData take care of that for you.
>+{
>+ nsCSSValue color;
>+ if (ParseVariant(aErrorCode, color, VARIANT_HC | VARIANT_NONE | VARIANT_URL,
>+ nsnull)) {
>+ nsCSSValue fallbackColor;
>+ if (color.GetUnit() == eCSSUnit_URL) {
>+ if (ParseVariant(aErrorCode, fallbackColor, VARIANT_HC | VARIANT_NONE,
>+ nsnull)) {
This should be just "VARIANT_COLOR | VARIANT_NONE"; inherit is only valid on its own.
>+ if (ExpectEndProperty(aErrorCode, PR_TRUE)) {
>+ aResult->mXValue = color;
>+ aResult->mYValue = fallbackColor;
>+ mTempData.SetPropertyBit(aPropID);
>+ return PR_TRUE;
>+ }
>+ return PR_FALSE;
>+ }
>+ fallbackColor.SetColorValue(NS_RGB(0, 0, 0));
Why black rather than SetNoneValue() ?
>+ } else {
>+ fallbackColor = color;
>+ }
>+
>+ if (ExpectEndProperty(aErrorCode, PR_TRUE)) {
>+ aResult->mXValue = color;
>+ aResult->mYValue = fallbackColor;
>+ mTempData.SetPropertyBit(aPropID);
>+ return PR_TRUE;
>+ }
>+ }
>+ return PR_FALSE;
So I think this should be simplified to something like:
>+ if (!ParseVariant(aErrorCode, aResult->mXValue,
>+ VARIANT_HC | VARIANT_NONE | VARIANT_URL,
>+ nsnull))
>+ return PR_FALSE;
>+ if (color.GetUnit() == eCSSUnit_URL) {
>+ if (!ParseVariant(aErrorCode, aResult->mYValue,
>+ VARIANT_COLOR | VARIANT_NONE,
>+ nsnull))
>+ aResult->mYValue.SetColorValue(NS_RGB(0, 0, 0)); // XXX or SetNoneValue?
>+ } else {
>+ aResult->mYValue = aResult->mXValue;
>+ }
>+
>+ if (!ExpectEndProperty(aErrorCode, PR_TRUE))
>+ return PR_FALSE;
>+
>+ mTempData.SetPropertyBit(aPropID);
>+ return PR_TRUE;
>Index: layout/style/nsRuleNode.cpp
>+SetSVGPaint(const nsCSSValuePair& aValue, const nsStyleSVGPaint& parentPaint,
>- } else if (aValue.GetUnit() == eCSSUnit_URL) {
>+ } else if (aValue.mXValue.GetUnit() == eCSSUnit_URL) {
> aResult.mType = eStyleSVGPaintType_Server;
>- aResult.mPaint.mPaintServer = aValue.GetURLValue();
>+ aResult.mPaint.mPaintServer = aValue.mXValue.GetURLValue();
> NS_IF_ADDREF(aResult.mPaint.mPaintServer);
>+ SetColor(aValue.mYValue, parentPaint.mFallbackColor, aPresContext, aContext, aResult.mFallbackColor, aInherited);
You don't want to pass parentPaint.mFallbackColor through to this -- I'd pass NS_RGB(0, 0, 0) instead, an add an assertion that aValue.mYValue.GetUnit() != eCSSUnit_Inherit. You do need to pass aInherited through for the currentColor case, though.
Also, you need to handle None yourself; SetColor doesn't handle that. You might be able to get away with handling it using NS_RGBA(0, 0, 0, 0).
>- } else if (SetColor(aValue, parentPaint.mPaint.mColor, aPresContext, aContext, aResult.mPaint.mColor, aInherited)) {
>+ } else if (SetColor(aValue.mXValue, parentPaint.mPaint.mColor, aPresContext, aContext, aResult.mPaint.mColor, aInherited)) {
> aResult.mType = eStyleSVGPaintType_Color;
>+ aResult.mFallbackColor = aResult.mPaint.mColor;
Why set mFallbackColor here but not all the other cases?
> nsSVGGeometryFrame::SetupCairoStroke(gfxContext *aContext,
>+ } else if (GetStyleSVG()->mStroke.mType == eStyleSVGPaintType_Server) {
>+ SetupCairoColor(ctx,
>+ GetStyleSVG()->mStroke.mFallbackColor,
>+ GetStyleSVG()->mFillOpacity);
You'll need to figure out how to handle however you handled 'none' as a fallback color here.
Attachment #248635 -
Flags: review?(dbaron) → review-
Comment 12•18 years ago
|
||
(In reply to comment #11)
> Why set mFallbackColor here but not all the other cases?
To clarify, I don't think you need to set it there.
Assignee | ||
Comment 13•18 years ago
|
||
(In reply to comment #11)
> (From update of attachment 248635 [details] [diff] [review])
> Why black rather than SetNoneValue() ?
As per bug 354295 Opera/Safari default to black if the URL is invalid to make it easier to spot. Note that as far as I can tell Opera does not seem to support fallback colours for stroke. I've therefore left the code defaulting to black if no fallback is specified.
> So I think this should be simplified to something like:
>
> >+ if (!ParseVariant(aErrorCode, aResult->mXValue,
> >+ VARIANT_HC | VARIANT_NONE | VARIANT_URL,
> >+ nsnull))
> >+ return PR_FALSE;
...
Done.
>
> You don't want to pass parentPaint.mFallbackColor through to this -- I'd pass
> NS_RGB(0, 0, 0) instead, an add an assertion that aValue.mYValue.GetUnit() !=
> eCSSUnit_Inherit. You do need to pass aInherited through for the currentColor
> case, though.
Done.
>
> Also, you need to handle None yourself; SetColor doesn't handle that. You
> might be able to get away with handling it using NS_RGBA(0, 0, 0, 0).
Done. Changed SVG code to support RGBA colours too.
> >+ aResult.mFallbackColor = aResult.mPaint.mColor;
>
> Why set mFallbackColor here but not all the other cases?
You're right this is not necessary. Removed.
Attachment #248635 -
Attachment is obsolete: true
Attachment #250343 -
Flags: review?(dbaron)
Comment 14•18 years ago
|
||
Comment on attachment 250343 [details] [diff] [review]
address review comments
>+ aResult->mYValue.SetColorValue(NS_RGB(0, 0, 0));
I'm still a little skeptical about using black, but I suppose it's ok.
> // stop-color:
>- SetSVGPaint(SVGData.mStopColor, parentSVGReset->mStopColor,
>- mPresContext, aContext, svgReset->mStopColor, inherited);
>+ if (eCSSUnit_None == SVGData.mStopColor.GetUnit()) {
>+ svgReset->mStopColor = NS_RGBA(0, 0, 0, 0);
>+ } else {
>+ SetColor(SVGData.mStopColor, parentSVGReset->mStopColor, mPresContext, aContext, svgReset->mStopColor, aInherited);
>+ }
stop-color doesn't take 'none' as a value, so this change seems unnecessary. Instead you should change CSSParserImpl::ParseSingleValueProperty so VARIANT_NONE is not allowed.
With that, r=dbaron.
Attachment #250343 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 15•18 years ago
|
||
Comment on attachment 250343 [details] [diff] [review]
address review comments
Fixed stop-color processing to exclude invalid 'none' possibility.
Attachment #250343 -
Attachment is obsolete: true
Attachment #250348 -
Flags: superreview?(tor)
Attachment #250348 -
Flags: superreview?(tor) → superreview+
Assignee | ||
Comment 16•18 years ago
|
||
Additional changes required:
integrated with bug 326143 (flood-color changed in the same way as stop-color)
integrated with bug 360316 (merge opacity changes)
Assignee | ||
Updated•18 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 17•18 years ago
|
||
Comment on attachment 250940 [details] [diff] [review]
version checked in
>Index: layout/style/nsRuleNode.cpp
>- SetSVGPaint(SVGData.mStopColor, parentSVGReset->mStopColor,
>- mPresContext, aContext, svgReset->mStopColor, inherited);
>+ SetColor(SVGData.mStopColor, parentSVGReset->mStopColor, mPresContext, aContext, svgReset->mStopColor, aInherited);
Oops, this should have continued to use inherited, not aInherited, but I asked tor to fix that in bug 326143.
Assignee | ||
Comment 18•18 years ago
|
||
(In reply to comment #17)
> (From update of attachment 250940 [details] [diff] [review])
> >Index: layout/style/nsRuleNode.cpp
> >- SetSVGPaint(SVGData.mStopColor, parentSVGReset->mStopColor,
> >- mPresContext, aContext, svgReset->mStopColor, inherited);
> >+ SetColor(SVGData.mStopColor, parentSVGReset->mStopColor, mPresContext, aContext, svgReset->mStopColor, aInherited);
>
> Oops, this should have continued to use inherited, not aInherited, but I asked
> tor to fix that in bug 326143.
>
I think tor fixed flood-color. This fault is stop-color. bug 366535 raised to fix it.
Assignee | ||
Comment 19•18 years ago
|
||
(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 250940 [details] [diff] [review] [details])
> > >Index: layout/style/nsRuleNode.cpp
> > >- SetSVGPaint(SVGData.mStopColor, parentSVGReset->mStopColor,
> > >- mPresContext, aContext, svgReset->mStopColor, inherited);
> > >+ SetColor(SVGData.mStopColor, parentSVGReset->mStopColor, mPresContext, aContext, svgReset->mStopColor, aInherited);
> >
> > Oops, this should have continued to use inherited, not aInherited, but I asked
> > tor to fix that in bug 326143.
> >
>
> I think tor fixed flood-color. This fault is stop-color. bug 366535 raised to
> fix it.
>
On checking further I was wrong. Tor fixed both of them. 'Tis all my fault.
You need to log in
before you can comment on or make changes to this bug.
Description
•