[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 #58 from Yong Li <yong.li at torchmobile.com>  2009-08-08 18:28:21 PDT ---
(In reply to comment #57)
> (From update of attachment 34391 [details])
> 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.

Yes, very useful. Thanks.

> 
> > +struct jpeg_dest_mgr {
> > +    struct jpeg_destination_mgr pub;
> > +    Vector<char> buffer;
> > +    Vector<char>* dump;
> > +};
> 
> Naming style (here and below)

I see { should be in the next line here. I'm told that "m_" should not be used
for "struct"

> 
> > +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)
> 

ok.

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

ok. Maybe we found a bug of check_webkit_style? :)

> 
> > +    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?

Good catch
> 
> 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);
> ?
try to benefit from inline vector. if total bytes of a row is less than 2k, we
don't need to allocate memory from heap.

> 
> > +    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;

*foo++ is very popular in image processing code. Not sure if it can help
compiler optimization.

> 
> > +#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.

Seems webkit prefer "while" to "for". Somehow I started hating "while" a few
years ago.

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