On 3/19/2015 3:53 PM, Laurent Bourgès wrote:
> One important functional issue that I think we should address early
on is that there are a number of property references in
MarlinRenderingEngine and MarlinConst that I don't think are protected
by AccessController doPrivileged blocks. These pieces of code cannot be
run inside an applet or any context managed by a SecurityManager as
access to arbitrary system properties is one of the first things to be
disabled by security managers. I'd recommend addressing that early on -
your call as to whether you want to address it before the first push.
I understand it can be a problem but for me applets are legacy... Anyway
could you give me a code pattern to use... to properly get System
properties in that secure environment.
Look at the source for
java.awt.GraphicsEnvironment.getHeadlessProperty(). It's much more
complicated than your needs, but it shows the way to wrap some code in a
doPrivileged block.
I'd recommend moving all of the getProperty() calls into a single static
initialization block (in the Const class for instance), wrapped in a
single doPrivileged() block which then sets static booleans to the
indicated values so that you don't have to fire up a doPrivileged()
block every time anyone calls any of those methods. The methods in the
Const class then just become getters for the saved booleans.
> - There are a number of single-line "/** short comment */" style
comments sprinkled around. They aren't really doc comments since they
don't contain all of the parts that one would expect in a doc comment.
They should probably just be shortened to a regular comment (/* ... */),
and any that are embedded in a class talking about a field could just as
easily be a single-line comment format (// ...), but having them pretend
to be doc comments (with the "/**") seems to be overkill.
Ok I will take care. Could you indicate some of them?
I pulled up the patch file (from the top of the webrev index page) and
just used a regex search for "[/][*][*].*[*][/]". Lots of lines, but
they appear to be grouped in just a few new blocks of new code.
I agree some long lines remain but we have very large displays in 2015,
don't you ?
Yes, but they aren't all large enough to hold 2 pages of text side by
side if the lines get too long. Pull up the sdiffs of the webrev and
notice that you have to scroll side to side to review the changes on
some of your files (unless you have a really wide screen, but many of us
work on laptops so 2x80 characters is comfortable and as you get much
wider then you need to start scrolling side to side - 2x100 is iffy).
In particular, the sdiff of Renderer.java in your last patch overflows
the width of my retina MBP screen by about 25-30% mostly because of long
lines in the "new" version.
(I'll note that with your changes, Dasher.java was actually improved -
it just barely doesn't overflow that same screen because you shortened
some of the old lines that were in there from previous fixes - thanks!)
...jim
