[rfc][icedtea-web] Console Output Encoding Fix

[Jie Kang]

----- Original Message -----
> On 07/16/2014 04:27 PM, Jie Kang wrote:
> > Hello
> >
> > Here is the revised patch with unit tests. If it looks okay I will work on
> > backporting it to 1.5.
> >
> >
> > Thanks,
> >
>  > diff --git a/netx/net/sourceforge/jnlp/util/logging/TeeOutputStream.java
>  > b/netx/net/sourceforge/jnlp/util/logging/TeeOutputStream.java
>  > --- a/netx/net/sourceforge/jnlp/util/logging/TeeOutputStream.java
>  > +++ b/netx/net/sourceforge/jnlp/util/logging/TeeOutputStream.java
>  > […]
>  > @@ -55,7 +54,7 @@
>  >
>  >      // Everthing written to TeeOutputStream is written to our log too
>  >
>  > -    private final StringBuffer string = new StringBuffer();
>  > +    private final ByteArrayOutputStream byteStorage = new
>  > ByteArrayOutputStream();
> 
> Naming: byteStorage is very misleading. What is a byte storage? Please rename
> this member variable to what it is, e.g. byteOutputStream.


Well to me a byte storage is some object that stores bytes. Personal style prefers not naming objects after the object identifier regardless, I have changed the name to byteOutputStream.

I guess this is situational but 'Jpanel jpanel' is something I hope to never see.


> 
>  >      private final boolean isError;
>  >
>  >      public TeeOutputStream(PrintStream stdStream, boolean isError) {
>  > […]
>  > @@ -82,32 +80,27 @@
>  >
>  >      @Override
>  >      public synchronized void write(byte[] b, int off, int len) {
>  > -        if (b == null) {
>  > -            throw new NullPointerException();
>  > -        } else if ((off < 0) || (off > b.length) || (len < 0)
>  > -                || ((off + len) > b.length) || ((off + len) < 0)) {
>  > -            throw new IndexOutOfBoundsException();
>  > -        } else if (len == 0) {
> 
> Err, testing len for 0 is indeed a good optimization. ;-)

Readded the test for 0 length.

> 
>  > -            return;
>  > -        }
>  > -        for (int i = 0; i < len; i++) {
>  > -            appendChar(b[off + i]);
>  > -        }
>  > +        appendByteArray(b, off, len);
>  >          super.write(b, off, len);
>  >      }
>  >
>  >      @Override
>  >      public synchronized void write(int b) {
>  > -        appendChar(b);
>  > +        appendByte(b);
> 
> Yep, proper renaming. Good. :-)
> 
>  >          super.write(b);
>  >      }
>  >
>  >      private void flushLog() {
>  > -        if (string.length() <= 0 ){
>  > -            return;
>  > +        String s = "";
> 
> Why initialize here with an empty string? Strings are immutable, remember?
> Besides, any of the ByteArrayOutputStream.toString() methods will always
> return
> a String, never null, so need to worry about null here.
> 
>  > +        try {
>  > +            s = byteStorage.toString("UTF-8");
> 
> Err, why do you suddenly assume the passed in byte array to be UTF-8 encoded?
> If
> anything, only the current default encoding can be assumed. So no, you cannot
> do
> this.
> 
>  > +        } catch (UnsupportedEncodingException e) {
>  > +            s = byteStorage.toString();
> 
> Just stick to ByteArrayOutputStream.toString() only.
> And, even if byteStorage.size() is 0 ByteArrayOutputStream.toString() will
> still
> return an empty string ("").
> 
>  >          }
>  > -        log(string.toString());
>  > -        string.setLength(0);
>  > +        if (s.length() > 0 ){
> 
> Formatting: Please remove the space before ) and add a space between ) and {.
> 
> Why do you check for s' length to be greater than 0? Cannot log handle empty
> strings?

I think there is no reason to log empty strings. There is no reason to propagate this to log.

> 
>  > +            log(s.toString());
> 
> ??? s is already of type String?
> 
>  > +            byteStorage.reset();
>  > +        }
>  >      }
>  >
>  >      @Override
>  > @@ -121,11 +114,23 @@
>  >          return isError;
>  >      }
>  >
>  > -    private void appendChar(int b) {
>  > -         if ( b <= 0 || b == '\n'){
>  > +    private void appendByte(int b) {
>  > +        byteStorage.write(b);
>  > +        if ( b <= 0 || b == '\n'){
> 
> Unfortunately, this is not correct. Like Jiri and me have been debating, it
> is
> not that simple in this case. With b being part of a multi-byte encoded
> character we cannot assume it to be denoting the 0 (\u0000) or new line
> (\u000a,
> \000a\u000c, or \u000c\u000a) code point.
> 
> So, what you actually would need to do is to convert the byte array
> (byteStorage) to a String and then check for the String's last character to
> be 0
> or new line. And yes, for every /single/ byte appended.\

Done.

> 
> Besides, incorrect formatting still remains.
> 
>  >              flushLog();
>  > -        } else {
>  > -            string.append((char)b);
>  > +        }
>  > +    }
>  > +
>  > +    private void appendByteArray(byte[] b, int off, int len) {
>  > +        byteStorage.write(b, off, len);
>  > +        String s = "";
> 
> Again, no need to initialize with an empty string here.
> 
>  > +        try {
>  > +            s = new String(b, off, len, "UTF-8");
> 
> Again, why do you suddenly assume the passed in byte array to be UTF-8
> encoded?
> 
>  > +        } catch (UnsupportedEncodingException e) {
>  > +            s = new String(b, off, len);
>  > +        }
>  > +        if(s.endsWith(System.getProperty("line.separator"))) {
> 
> Yep, something like this would need to happen in appendByte(int) too. ;-)
> 
> Yet, for better performance System.getProperty("line.separator") should be
> put
> into a final String member on TeeOutputStream's constuction.
> 
> Again, formatting: Add a space between if and (.
> 
>  > +            flushLog();
>  >          }
>  >      }
> 
> Jacob
> 

Thanks,

-- 

Jie Kang
-------------- next part --------------
A non-text attachment was scrubbed...
Name: console-1858-4.patch
Type: text/x-patch
Size: 5815 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140716/d168ef21/console-1858-4.patch>