[Webkit-unassigned] [Bug 27511] Add WinCE specific platform/graphics files to WebCore

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27511





--- Comment #57 from Peter Kasting <pkasting at google.com>  2009-08-08 17:27:07 PDT ---
(From update of attachment 34391)
Since Eric has asked for some of my comments in the past, I decided to comment
on the first patch on here as if I were a reviewer.  Perhaps you'll find these
useful.

> +struct jpeg_dest_mgr {
> +    struct jpeg_destination_mgr pub;
> +    Vector<char> buffer;
> +    Vector<char>* dump;
> +};

Naming style (here and below)

> +void jpeg_init_destination(j_compress_ptr compressData)
> +{
> +    jpeg_dest_mgr* dest = (jpeg_dest_mgr*)compressData->dest;
> +    dest->buffer.resize(4096);
> +    dest->pub.next_output_byte  = (JOCTET*)dest->buffer.data();
> +    dest->pub.free_in_buffer    = dest->buffer.size();
> +}

Please use C++-style casts (here and below)

> +bool compressBitmapToJpeg(SharedBitmap* bmp, Vector<char>& jpegData)
> +{
> +    struct jpeg_compress_struct compressData= {0};

Spacing (here and below)

> +    Vector<JSAMPLE, 2048> rowBuffer;
> +    if (setjmp(err.setjmp_buffer)) {
> +        jpeg_destroy_compress(&compressData);
> +        return false;
> +    }
> +
> +    jpeg_start_compress(&compressData, TRUE);
> +
> +    rowBuffer.resize(compressData.image_width * 3);

Why not declare rowBuffer just above this last line?

Also, the 2k capacity value seems arbitrary.  How do we know that that's a good
value?  Why not just:
    Vector<JSAMPLE> rowBuffer(compressData.image_width * 3);
?

> +    if (bmp->is16bit()) {
> +        int paddedWidth = bmp->bitmapInfo().paddedWidth();
> +        int skip = paddedWidth - compressData.image_width;

A more descriptive name might be |scanlinePadding| or similar.

> +        for (; pixel < pixelEnd; pixel += skip) {
> +            JSAMPLE* dst = rowBuffer.data();
> +            for (const unsigned short* rowEnd = pixel + compressData.image_width; pixel < rowEnd;) {
> +                unsigned short value = *pixel++;

Might be a little clearer to do:
for (...; ...; ++pixel) {
    unsigned short value = *pixel;

> +#if IMAGE_NO_ALPHA_USE_RGB555
> +                *dst++ = JSAMPLE((value >> 7) & 0xF8);
> +                *dst++ = JSAMPLE((value >> 2) & 0xF8);
> +                *dst++ = JSAMPLE((value << 3) & 0xF8);
> +#else

Might want an end-of-line comment on each #if with the bit format for clarity,
e.g. // xrrrrrgg gggbbbbb
And perhaps one on each write to *dst with which component is being written,
e.g. // R

> +        for (; pixel < pixelEnd; ) {

I find "while (pixel < pixelEnd) {" clearer.

> --- /dev/null
> +++ b/WebCore/platform/image-encoders/wince/PNGEncoder.cpp
> @@ -0,0 +1,167 @@
> +/*
> + * Copyright (c) 2006-2009, Google Inc. All rights reserved.
> + * Copyright (c) 2009 Torch Mobile, Inc. All rights reserved.

Food for thought: One thing that might make this easier to review would be to
first land a direct copy of the Skia files, then land a patch with your diffs.

> +    int byteDepth = bmp->is16bit() ? 3 : 4;
> +    Vector<unsigned char, 2048> rowBuffer;
> +    rowBuffer.resize(bmp->width() * byteDepth);

Same capacity/size comment as in JPEG encoder.

> +    if (bmp->is16bit()) {
> +        unsigned short* src = (unsigned short*)bmp->bytes();
> +        for (int y = 0; y < (int)bmp->height(); ++y) {

C++-style casts please (here and below)

> +#if IMAGE_NO_ALPHA_USE_RGB555
> +                *dst++ = unsigned char((value >> 7) & 0xF8);
> +                *dst++ = unsigned char((value >> 2) & 0xF8);
> +                *dst++ = unsigned char((value << 3) & 0xF8);
> +#else
> +                *dst++ = unsigned char((value >> 8) & 0xF8);
> +                *dst++ = unsigned char((value >> 3) & 0xFC);
> +                *dst++ = unsigned char((value << 3) & 0xF8);
> +#endif

Same suggestions re: EOL comments as in JPEG encoder.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.