[OpenJDK Rasterizer] Marlin renderer contribution for review

[Jim Graham]

Hi Laurent,

I'm now looking at your webrev.  There are a number of things that I 
think we should look at once it goes in, and I'll mention those in 
subsequent emails, but I want to just deal with basic issues that we 
should deal with before the first commit here...

AAShapePipe - the static println in TileState should probably be removed 
(for one thing, it will now be run regardless of whether or not Marlin 
is selected).

Is there anything about the patch to AAShapePipe that is specific to 
Marlin?  Perhaps it should have its own bugid?

Even though doCleanDirty is turned off, Dasher.java line 153 looks like 
it may overflow if you've ever had to widen the firstSegs array with it 
turned on.

Stroker.java, 312 - indent looks off.

Use of "fluent API".  In general I think that is a nice way to do thing 
in a builder-style API where a whole chain of related methods all return 
"this" to facilitate the chaining, but identifying isolated methods that 
happen to often be used just prior to a return of the object they were 
executing on seems to me to be more of a break in object encapsulation. 
  It adds a burden to the called method to deal with a return value so 
that we can eliminate a line of code in the caller.  But, in reality the 
caller still has to return something - it just returns a different value 
(the value saved in a local variable instead of the value returned by 
the called method) so you are actually adding code to the interface to 
make things appear simpler.  This isn't a veto of those cases here, but 
I wanted to point out that their value in isolated cases is questionable 
unlike the "chained setter methods" case for which they were initially 
targeted.

TransformingPathConsumer2D (and any other new code) - All methods should 
have a blank line between them.  This notably applies to the Path2D 
wrapper.  Also, a common style point throughout 2D is that when there is 
a continuation line in an argument list declaration, we place the "{" 
brace on a new line all by itself so that the eye has a good landmark 
for where the body of the method begins.  (See how the old "curveTo()" 
methods differ from the new one created in Path2DWrapper.)

TransformingPathConsumer2D - the if/else style being replaced here was 
actually intentional.  The lack of a trailing "return" statement ensures 
that all cases were specifically handled above in either an if, or its 
associated else block, and that none of the cases ended up falling 
through to a subsequent "catch-all" operation by accident.  Nothing is 
broken here, I just wanted to point out the "whys" of the original code 
style.  In the new code, there are still some bare "} else {" lines left 
which raises the question - what was the specific style goal in whether 
an "} else {" was eliminated or left intact?  The prior style goal was 
"no fallthrough cases anywhere" - the new style seems to be "usually 
fallthrough, but sometimes explicit alternate return values in an else 
clause".





-------- Here are some "things to consider later" ---------

Is there ever a reason to use doCleanDirty?  It might matter for arrays 
of objects to null out old references so as to not interfere with GC, 
but don't most growable arrays get filled in as they are used so old 
data isn't really processed?  I notice that it is turned off right now 
so perhaps that's an argument in favor of my comment, but it seems to me 
that having to clean out a primitive array would be an odd rare 
exception and so simply adding in a clear where it is needed in the 
unlikely case that it is needed would be fine (and get rid of a lot of 
cruft in the code).

I just realized the main case where we want to deal with cleaning out 
dirty arrays and it would be the crossings accumulation arrays.  It 
seems to me that those are special cases and so we should concentrate 
the cleaning code on those explicitly and not have a global "lets clean 
all arrays everywhere" setting.

Isn't declaring flatLeafCoefficients as final enough to not need to 
cache it's reference in a local variable?  Also, I would think you would 
gain more by simply deleting the array and storing the coefficients in 
fields instead?  An array index lookup should be no simpler than a local 
field access.

I was also skeptical of these Iterators that Denis created and thought 
that it may be better to just manually iterate the subdivisions instead 
of creating (or even keeping) an object around to hide the simple job of 
iterating a curve.  I now wonder if inlining the actions of the 
BreakPoint iterator might not improve performance for complex paths and 
leave less state that we need to manage.

FastMath - In general, if we use "(int) ceil()" a lot, we might want to 
consider offsetting the math so that all values are negative and then 
use a cast to (int).  In particular, if crossings are going to use 
"ceil(v - 0.5)" which is the proper way to do half-open intervals with 
centers included on top and left sides, then simply shifting everything 
once so that the entire range of interesting X,Y are negative would mean 
that all crossings calculations could be done by casting to integer, 
especially if we also offset them all by 0.5 as well.

FastMath - if we can prove that no values <0 are interesting (as in, the 
only interesting property about them is that they are less than 0), then 
we can just use a cast to integer.  This is particular true if we clamp 
a value to >= 0 after we convert it to an integer - in that case we 
might as well have used an int cast rather than a floor() computation.

Eventually we will need to make a decision on all LBO and TODO comments. 
  Either make a call and change the code and delete the comment, or 
submit the issue as a new bugid and reference the bugid rather than TODO 
or LBO.

Those were just a few thoughts/questions that came to mind as I was 
reviewing it, but I didn't want to get bogged down too much in the 
specifics that we can tweak later until we can get an implementation 
published for people to test on so I tried to back off and keep my 
review at a higher level.  I may have more comments as I continue to 
look through the code...

			...jim