Darxus Reporter
	Description • 14 years ago

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b4pre) Gecko/20100817 Minefield/4.0b4pre
Build Identifier: 

No blue square.

Reproducible: Always

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 14 years ago

Does cairo's behavior for LINE_CAP_SQUARE just not match the one the SVG spec calls for here?

	Robert Longson [:longsonr]
	Comment 2 • 14 years ago

No it doesn't. You have to invent an orientation for the square so it's a bit artificial.

	Jonathan Watt [:jwatt] Assignee
	Comment 4 • 14 years ago

This causes us to fail:

http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObjectMiniApproved/painting-control-04-f.html

	Mikko Virkkilä
	Comment 5 • 14 years ago

I'm seeing this bug in 2d canvas with ctx.lineCap="square". Should I open a separate bug?

	Robert Longson [:longsonr]
	Comment 6 • 14 years ago

Perhaps you should try seeing whether cairo is willing to change?

	Jeff Muizelaar [:jrmuizel]
	Comment 7 • 14 years ago

(In reply to comment #6)
> Perhaps you should try seeing whether cairo is willing to change?

I'd like to see a justification for cairo having square caps for zero-length paths. Adobe Acrobat doesn't have square caps for zero-length paths which is where cairo's current behavior comes from.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 14 years ago

Mikko, the canvas thing is almost certainly a duplicate of this bug.  As long as you're willing to track this bug and retest once it's fixed, probably no separate bug needed...

	Jonathan Watt [:jwatt] Assignee
	Comment 9 • 14 years ago

(In reply to comment #7)
> I'd like to see a justification for cairo having square caps for zero-length
> paths. Adobe Acrobat doesn't have square caps for zero-length paths which is
> where cairo's current behavior comes from.

For consistency with cairo's /round/ caps behavior?

Alternatively, to allow a major cairo consumer (us) to conform to the specs for one of the open web languages we support?

	Jonathan Watt [:jwatt] Assignee
	Comment 12 • 14 years ago

I'm not particularly in love with these hacks, but I don't think it's too bad.

Jeff, can you comment on the "XXX tinyAdvance should probably depend on the CTM" comment in this patch?

	Robert Longson [:longsonr]
	Comment 13 • 14 years ago

Comment on attachment 505292 [details] [diff] [review]
patch 2: preliminary patch to stop hardcoding segment arg count

Why not just increment i by SVGPathSegUtils::ArgCountForType(segType) at the end directly rather than having a separate argCount variable?

	Jonathan Watt [:jwatt] Assignee
	Comment 14 • 14 years ago

Because I need that argCount variable in the subsequent patch.

	Jonathan Watt [:jwatt] Assignee
	Comment 15 • 14 years ago

Err, or I was in an earlier incarnation of the third patch. That's no longer the case in the current version, so I'll change it as you suggest.

	Robert Longson [:longsonr]
	Comment 16 • 14 years ago

Looking at bug 584623, it seems like linecap round has issues too. Should patch 3 do linecap round too?

The issue with linecap round is that cairo doesn't think there is a bounding box. This lead to the fix in bug Bug 510177. This fix should be reverted if we can get a proper solution to the linecap round issue.

	Jonathan Watt [:jwatt] Assignee
	Comment 17 • 14 years ago

I just commented on bug 584623 - it's WFM in beta 9. I can add a test for linecap round similar to the one included in patch 3 though.

	Robert Longson [:longsonr]
	Comment 18 • 14 years ago

Comment on attachment 505294 [details] [diff] [review]
patch 3: work around lack of cairo support

>+static PRBool IsMoveto(PRUint16 aSegType)

Nit: I realise you just moved this but IsMoveTo would be nicer :-) If you look at the comments further on though you might
not need to move this at all.

>+  // XXX tinyAdvance should probably depend on the CTM

I doubt it, we made that mistake with radial gradients and then had to change the delta not to depend on the CTM.

>+  double tinyAdvance = 0.002;

static const double. Capitalised?

I'm surprised this is sufficient. Cairo as we use it is 24.8 and 1/256 = 0.00390625
which is the smallest difference cairo can detect isn't it.

>+#define MAYBE_APPROXIMATE_ZERO_LENGTH_SUBPATH_SQUARE_CAPS                     \
>+  do {                                                                        \
>+    if (capsAreSquare && !subpathHasLength && subpathContainsNonArc &&        \
>+        SVGPathSegUtils::IsValidType(prevSegType) &&                          \
>+        (!IsMoveto(prevSegType) ||                                            \
>+         segType == nsIDOMSVGPathSeg::PATHSEG_CLOSEPATH)) {                   \
>+      ApproximateZeroLengthSubpathSquareCaps(segStart, aCtx);                 \
>+    }                                                                         \
>+  } while(0)

What's the do ... while(0) for? I thought you only needed that for macros that might be defined as nothing sometimes.

Could this not be an inline function instead?

>+    if (!IsMoveto(segType) && segEnd != segStart ||
>+        SVGPathSegUtils::IsQuadraticType(segType) && segEnd != cp1 ||
>+        SVGPathSegUtils::IsCubicType(segType) && (segEnd != cp1 || segEnd != cp2)) {
>+      subpathHasLength = PR_TRUE;
>+    }

Since the code above is a big switch on segType, couldn't you set this in the appropriate switch options?

>+    if (!IsMoveto(segType) && !IsArc(segType)) {
>+      subpathContainsNonArc = PR_TRUE;

ditto.

>diff --git a/layout/svg/base/src/nsSVGPathGeometryFrame.cpp b/layout/svg/base/src/nsSVGPathGeometryFrame.cpp
>--- a/layout/svg/base/src/nsSVGPathGeometryFrame.cpp
>+++ b/layout/svg/base/src/nsSVGPathGeometryFrame.cpp
>@@ -446,16 +446,22 @@ nsSVGPathGeometryFrame::Render(nsSVGRend
>   default:
>     gfx->SetAntialiasMode(gfxContext::MODE_COVERAGE);
>     break;
>   }
> 
>   /* save/restore the state so we don't screw up the xform */
>   gfx->Save();
> 
>+  // Hack to let SVGPathData::ConstructPath know if we have square caps:

Omit "Hack to".

>+  const nsStyleSVG* style = GetStyleSVG();
>+  if (style->mStrokeLinecap == NS_STYLE_STROKE_LINECAP_SQUARE) {
>+    gfx->SetLineCap(gfxContext::LINE_CAP_SQUARE);
>+  }
>+

Why is this only in nsSVGPathGeometryFrame::Render? Doesn't it need to be in GeneratePath? What about GetFrameForPoint, UpdateCoveredRegion. You should add a mochitest for GetFrameForPoint or add to the existing GetFrameForPoint mochitest. You'd need some kind of dynamic reftest where you move shapes about to prove that UpdateCoveredRegion works and you're not leaving artifacts.

	Jonathan Watt [:jwatt] Assignee
	Comment 19 • 14 years ago

(In reply to comment #18)
> Comment on attachment 505294 [details] [diff] [review]
> >+static PRBool IsMoveto(PRUint16 aSegType)
> 
> Nit: I realise you just moved this but IsMoveTo would be nicer :-)

I deliberately didn't uppercase the "t" in "to" for consistency with the SVG spec.

> >+  // XXX tinyAdvance should probably depend on the CTM
> 
> I doubt it, we made that mistake with radial gradients and then had to change
> the delta not to depend on the CTM.

This case is pretty different, and we definitely depend on the CTM (zoom out and squares stop showing, zoom in and they still show). I think the patch coming up handles this correctly.

> >+  double tinyAdvance = 0.002;
> 
> static const double. Capitalised?

const, sure, but why static? Not sure about the capitalized either, since we don't tend to do that, do we(?) (and it is only a four line function).

> I'm surprised this is sufficient. Cairo as we use it is 24.8 and 1/256 =
> 0.00390625
> which is the smallest difference cairo can detect isn't it.

I'd guess it was working for me because 0.002 is just over half of 0.00390625, so it was being rounded up. This code has now changed though.

> What's the do ... while(0) for? I thought you only needed that for macros that
> might be defined as nothing sometimes.

So that I could add a semicolon after uses of the macro, which feels better.

> Could this not be an inline function instead?

It could, but it would take seven arguments so probably doesn't generate much less code and would be less readable.

> Since the code above is a big switch on segType, couldn't you set this in the
> appropriate switch options?

Yeah, okay. I was just trying not to clutter all the switch statements, but I guess that's what I should do.

> Omit "Hack to".

Hmm, I think it is a hack really, especially after it's moved to GeneratePath as you rightly point out it should be.

	Robert Longson [:longsonr]
	Comment 21 • 14 years ago

> > >+  double tinyAdvance = 0.002;
> > 
> > static const double. Capitalised?
> 
> const, sure, but why static? Not sure about the capitalized either, since we
> don't tend to do that, do we(?) (and it is only a four line function).

static data is only initialised once. You get better performance.

Everything else seems fine.

	Jonathan Watt [:jwatt] Assignee
	Comment 22 • 14 years ago

Is that really true for |double|? I'd assumed that the bit pattern for the double value would just have been hardcoded into the binary implementation of the function generated by the compiler and no double initializer code would be invoked for it.

Anyway, now that the variable is a gfxSize instead of a double it can't be static:

https://developer.mozilla.org/index.php?title=en/C%2b%2b_Portability_Guide#Don%27t_use_static_constructors

Probably the only rational from that section that applies is the "may confuse leak tools" one which may not apply in this case, but we should probably follow the letter of the law anyway?

	Jonathan Watt [:jwatt] Assignee
	Comment 23 • 14 years ago

Comment on attachment 506285 [details] [diff] [review]
patch 3: work around lack of cairo support

Requesting approval in advance of Jeff's feedback on the implementation of ApproximateZeroLengthSubpathSquareCaps. I think it's actually okay, but in the worst case we'll just fail to draw caps under certain transforms or something, and if there's a problem like that we can retrospectively fix it.

	Jonathan Watt [:jwatt] Assignee
	Comment 24 • 14 years ago

Note the approval request is for all three patches. Maybe plusing needs to be marked on all three explicitly?

	Jonathan Watt [:jwatt] Assignee
	Comment 25 • 14 years ago

Of course we can't set subpathHasLength blindly or else that variable should really be named "lastSegHasLength", which is not what we want.

	Jonathan Watt [:jwatt] Assignee
	Comment 26 • 14 years ago

Modulo reftest failures due to random orange and the problem addressed in comment 25, the patches pass try server.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 27 • 14 years ago

Comment on attachment 506407 [details] [diff] [review]
patch 3: work around lack of cairo support

Let's take this after FF4.

	Jonathan Watt [:jwatt] Assignee
	Comment 28 • 13 years ago

unbitrot

	Jonathan Watt [:jwatt] Assignee
	Comment 29 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/cbaa924ffeca
http://hg.mozilla.org/mozilla-central/rev/ccd5d505ab00
http://hg.mozilla.org/mozilla-central/rev/5b78983c4822

	Jonathan Watt [:jwatt] Assignee
	Comment 30 • 11 years ago

longsonr: do you remember why arc was excluded in this patch? (That is, why subpathContainsNonArc exists.)