Bug 27511 – Add WinCE specific platform/graphics files to WebCore

• Component: Platform
• Status: RESOLVED WONTFIX

[Yong Li]

not including font-related files. font-related files is added through bug 27509

[Yong Li]

Created attachment 33202 [details]
graphics files for WINCE port

not including font-related files

[George Staikos]

Comment on attachment 33202 [details]
graphics files for WINCE port

Same issue here - LGPL license on files that are entirely new by Torch.

[Yong Li]

Created attachment 33262 [details]
fixed style errors reported by cpplint and added correct license text

[Peter Kasting]

Seems like some of these would be better suited for platform/image-decoders/wince/ ?

[Yong Li]

(In reply to comment #4)
> Seems like some of these would be better suited for
> platform/image-decoders/wince/ ?

You mean ImageDecoderUtilities.*? Hm... seems I should remove them and replace ByteBuffer with Vector<char> in where it's referenced.

[Yong Li]

Created attachment 33331 [details]
removed unnecessary files

removed unnecessary files

[Peter Kasting]

(In reply to comment #6)
> Created an attachment (id=33331) [details]
> removed unnecessary files

Your ChangeLog comment is now out-of-date.

[Peter Kasting]

(In reply to comment #6)
> Created an attachment (id=33331) [details]

You probably want to put PNGEncoder.* and JPEGEncoder.* in platform/image-encoders/wince/ (see the skia/ directory that is already there, and contains a PNGImageEncoder.cpp that is very similar to your PNGEncoder.cpp).

[Yong Li]

(In reply to comment #8)
> (In reply to comment #6)
> > Created an attachment (id=33331) [details] [details]
> 
> You probably want to put PNGEncoder.* and JPEGEncoder.* in
> platform/image-encoders/wince/ (see the skia/ directory that is already there,
> and contains a PNGImageEncoder.cpp that is very similar to your
> PNGEncoder.cpp).

Yes, I copied PNGEncoder code from theirs and did some modifications. JPEGEncoder is written by me.

[Peter Kasting]

(In reply to comment #9)
> Yes, I copied PNGEncoder code from theirs and did some modifications.
> JPEGEncoder is written by me.

Right; putting these alongside the others makes it easier to realize there are multiple copies so we can refactor in a more cross-platform way.

[Yong Li]

Created attachment 33546 [details]
move encoder files to platform/image_encoders/wince

[Yong Li]

Created attachment 33813 [details]
1) image encoders

[Yong Li]

Created attachment 33814 [details]
2) SharedBitmap

[Yong Li]

Created attachment 33815 [details]
3) ImageBuffer

[Yong Li]

Created attachment 33816 [details]
4) ImageFrameSink

[Yong Li]

Created attachment 33817 [details]
5) MediaPlayer

[Yong Li]

Created attachment 33818 [details]
6) Path

[Yong Li]

Created attachment 33819 [details]
7) Color, Gradient, Image

[Yong Li]

Created attachment 33820 [details]
7) Color, Gradient, Image

[Peter Kasting]

Your first patch contains a ChangeLog listing all the files you've changed in
total, and your other patches don't have any ChangeLogs.

ImageFrameSink is basically a copy of the cross-platform ImageDecoder with one
or two small changes.  Would it be better to write this as a patch to
ImageDecoder?

Your implementation of BitmapImage::draw() is incorrect.  StartAnimation() needs to be called before drawing, not after.  Seems like maybe you wrote this implementation a year or two ago.  You might want to go study how the CG or Skia implementations have been modified since then.

Some of your files have vim formatting comments at bottom.  Seems like these should be stripped.

[Yong Li]

(In reply to comment #20)
> Your first patch contains a ChangeLog listing all the files you've changed in
> total, and your other patches don't have any ChangeLogs.

I split them for easier review. Hope they can be committed at one time.

> 
> ImageFrameSink is basically a copy of the cross-platform ImageDecoder with one
> or two small changes.  Would it be better to write this as a patch to
> ImageDecoder?
> 

Do you mean RGBA32Buffer? Don't want to mess it up. Also, I think RGBA32Buffer should be extracted to a separate header.

Hm... just found a problem there: setRGBA there is actuallly set to ARGB. (32bit Bitmap is ARGB by default on windows)

> Your implementation of BitmapImage::draw() is incorrect.  StartAnimation()
> needs to be called before drawing, not after.  Seems like maybe you wrote this
> implementation a year or two ago.  You might want to go study how the CG or
> Skia implementations have been modified since then.

You're right. Thanks.

> 
> Some of your files have vim formatting comments at bottom.  Seems like these
> should be stripped.

what's that?

[Peter Kasting]

(In reply to comment #21)
> (In reply to comment #20)
> > Your first patch contains a ChangeLog listing all the files you've changed in
> > total, and your other patches don't have any ChangeLogs.
> 
> I split them for easier review. Hope they can be committed at one time.

It would be easier for reviewers and anyone landing the patch if each patch was self-contained...

> > ImageFrameSink is basically a copy of the cross-platform ImageDecoder with one
> > or two small changes.  Would it be better to write this as a patch to
> > ImageDecoder?
> > 
> 
> Do you mean RGBA32Buffer? Don't want to mess it up. Also, I think RGBA32Buffer
> should be extracted to a separate header.

Yes, the RGBA32Buffer inside ImageDecoder.h.

I agree that eventually it should be moved into its own place, but for now it's not there, and it's hard to remember that the WinCE port happens to have a completely differently-named implementation living off in a separate place.  When I make changes to the cross-platform version, I'll likely break your build.

> > Some of your files have vim formatting comments at bottom.  Seems like these
> > should be stripped.
> 
> what's that?

I guess it's only ImageSourceWince.cpp.  "// vim: ts=4 sw=4 et" at bottom.

[Yong Li]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)

> > 
> > Do you mean RGBA32Buffer? Don't want to mess it up. Also, I think RGBA32Buffer
> > should be extracted to a separate header.
> 
> Yes, the RGBA32Buffer inside ImageDecoder.h.
> 
> I agree that eventually it should be moved into its own place, but for now it's
> not there, and it's hard to remember that the WinCE port happens to have a
> completely differently-named implementation living off in a separate place. 
> When I make changes to the cross-platform version, I'll likely break your
> build.
> 

OK. I'll make it shared code with RGBA32Buffer. But, this bug is supposed to only contain new files. The changes in ImageDecoder.h will go to another bug. To reduce the naming change, I'll "typedef RGBA32Buffer ImageFrameSink;", what do you think?

[Peter Kasting]

(In reply to comment #23)
> OK. I'll make it shared code with RGBA32Buffer. But, this bug is supposed to
> only contain new files. The changes in ImageDecoder.h will go to another bug.
> To reduce the naming change, I'll "typedef RGBA32Buffer ImageFrameSink;", what
> do you think?

Hmm... I'm trying to figure out what the best landing sequence here is, since those ImageDecoder changes might take a while to work through.

Seems like there are a lot of options:

* Land ImageFrameSink as-is, in graphics/wince.  Perhaps add comments atop ImageDecoder.h/ImageFrameSink.h noting the similarities between the two, in hopes that people won't break your port.  File a new bug about merging the two together.

* Break RGBA32Buffer out of ImageDecoder.h into its own header, but don't change anything else about it.  Put your versions alongside this header/implementation.  Not sure what the names of each should be at that point.  Hope that this provides enough motivation for people to combine the two eventually.

* Wait until RGBA32Buffer gets pulled out of ImageDecoder and moved over to graphics/.  Modify RGBA32Buffer directly, and rename it something more accurate.  (Although TBH I despise the name "ImageFrameSink" as it feels somewhere between meaningless and inaccurate.  Just "ImageFrame" would probably be better.)

* Various other combinations of the above

It doesn't seem fair to either force you to refactor all the image classes or make you wait on somebody else to do it (although it would be nice to have help refactoring things...).  Perhaps the first option is the best thing for the WinCE port.  Even though I suggested modifying RGBA32Buffer, I have the feeling it's going to take some time and effort to examine the changes you've made and figure out how to best merge them in to the code everyone else is using (hopefully with few if any #ifdefs).  So maybe a strategy that gets your files into the tree is better...

[Yong Li]

Created attachment 34068 [details]
7) Color, Gradient, Image

call startAnimation() first

[Yong Li]

(In reply to comment #24)

Yes, the image frame sink stuff is a very big change. We tried to separate it out to avoid affecting other platforms badly.

[Peter Kasting]

(In reply to comment #26)
> Yes, the image frame sink stuff is a very big change. We tried to separate it
> out to avoid affecting other platforms badly.

FWIW, I'm in the midst of actively working on the Image subsystem, and I have speculative plans to split RGBA32Buffer out of ImageDecoder.h, rename it, and move it to graphics/ in a week or two.  That might make it slightly easier to modify so that it will work for WinCE too (or it might not help much, not sure).

[Yong Li]

(In reply to comment #27)
> (In reply to comment #26)
> > Yes, the image frame sink stuff is a very big change. We tried to separate it
> > out to avoid affecting other platforms badly.
> 
> FWIW, I'm in the midst of actively working on the Image subsystem, and I have
> speculative plans to split RGBA32Buffer out of ImageDecoder.h, rename it, and
> move it to graphics/ in a week or two.  That might make it slightly easier to
> modify so that it will work for WinCE too (or it might not help much, not
> sure).

It's good to split RGBA32Buffer out, create more generic interface as image decoding buffer, and allow different implementations for different ports or products. Actually our ImageFrameSink is such an interface, and it should be very easy to implement ImageFrameSink with current RGBA32Buffer or Vector<char>. Would you please consider making new RGBA32Buffer as close to ImageFrameSink.h as possible? That could make merging my code easier :)

[George Staikos]

(In reply to comment #28)
> It's good to split RGBA32Buffer out, create more generic interface as image
> decoding buffer, and allow different implementations for different ports or
> products. Actually our ImageFrameSink is such an interface, and it should be
> very easy to implement ImageFrameSink with current RGBA32Buffer or
> Vector<char>. Would you please consider making new RGBA32Buffer as close to
> ImageFrameSink.h as possible? That could make merging my code easier :)

   If Peter wants to have this, and your patch provides this, then breaking it out into a separate patch should be sufficient to solve everyones' problems I think.

[Peter Kasting]

(In reply to comment #28)
> It's good to split RGBA32Buffer out, create more generic interface as image
> decoding buffer, and allow different implementations for different ports or
> products.

The very first patch will just move it somewhere without changing the implementation.  The next thing I'd like to do afterward is take a hard look at your ImageFrameSink and see if I can figure out how to add those capabilities to the existing cross-plaform code in a way that doesn't require tons of #ifdefs.

> it should be
> very easy to implement ImageFrameSink with current RGBA32Buffer

Yes, one possibility before this functionality gets included directly (or perhaps in place of doing that?) would be to make your ImageFrameSink a subclass of this other class and simply extend it.

[Eric Seidel (no email)]

Comment on attachment 33813 [details]
1) image encoders

Why do you need image encoders?  More information is needed in your ChangeLog. Also, you should get pkasting to look at this as he's most familiar with our encoder/decoder stuff.

[Yong Li]

(In reply to comment #31)
> (From update of attachment 33813 [details])
> Why do you need image encoders?  More information is needed in your ChangeLog.

I think this is obvious. We need image encoders to support the canvas feature of saving image buffer to data url. See platform/image-encoders. There's already a "skia" folder.

> Also, you should get pkasting to look at this as he's most familiar with our
> encoder/decoder stuff.

Yes, we had talks. He suggested to move wince image encoders to this folder.

[Peter Kasting]

(In reply to comment #32)
> (In reply to comment #31)
> > Also, you should get pkasting to look at this as he's most familiar with our
> > encoder/decoder stuff.
> 
> Yes, we had talks. He suggested to move wince image encoders to this folder.

Note that I never actually looked at the code.

It would be even more awesome if the cross-platform bits were factored into a common file and then you had platform-specific extensions for skia and wince.  If you're not planning on doing that before landing, are you planning to do it after landing?  (I did a similar "refactor after landing" with the skia image decoders.)

[Yong Li]

(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #31)
> > > Also, you should get pkasting to look at this as he's most familiar with our
> > > encoder/decoder stuff.
> > 
> > Yes, we had talks. He suggested to move wince image encoders to this folder.
> 
> Note that I never actually looked at the code.
> 
> It would be even more awesome if the cross-platform bits were factored into a
> common file and then you had platform-specific extensions for skia and wince. 
> If you're not planning on doing that before landing, are you planning to do it
> after landing?  (I did a similar "refactor after landing" with the skia image
> decoders.)

Yes, that would be awesome. The platform-specific issue is that the image format in source buffer is differently. Except that, our png encoder is just a clone of the skia png encoder. And our jpeg encoder can be used for other ports with a little bit modification, too. I would like to do it when I get time.

[Eric Seidel (no email)]

Comment on attachment 33814 [details]
2) SharedBitmap

r- for lack of ChagneLog.

Please run check-webkit-style.

g_ is not a pattern we use:
 static SharedBitmap::DrawPatternFunc g_drawPattern = 0;
 52 static SharedBitmap::DrawImageFunc g_drawImage = 0;


This can't be the only place in the project we need this?
 54 static inline int stableRound(double d)
please look for other implementations.

What is this class even for?  Please explain in a ChagneLog.

Why crateInstance instead of create?
 77 PassRefPtr<SharedBitmap> SharedBitmap::createInstance(bool is16bit, int w, int h, bool initPixels)

Indent:
SharedBitmap::SharedBitmap(const BitmapInfo& bmpInfo, HBITMAP hbmp, void* pixels)
 88 : m_bmpInfo(bmpInfo)
 89 , m_locked(false)
 90 

Please have george review your patches before posting again.  These are not nearly ready.

[Eric Seidel (no email)]

Comment on attachment 33815 [details]
3) ImageBuffer

No ChangeLog.  r-

Please have George review your patches before posting again.  As demonstrated by the previous 2 patches, these are not ready for real review yet and posting them for review with George at least looking at them is just wasting other folks time.  George should be helping his contractors to follow the WebKit process, not depending on reviewers to teach you all.

[Eric Seidel (no email)]

Comment on attachment 33816 [details]
4) ImageFrameSink

No ChangeLog.  r-.

[Eric Seidel (no email)]

Comment on attachment 33817 [details]
5) MediaPlayer

No ChangeLog.  r-.

[Eric Seidel (no email)]

Comment on attachment 33818 [details]
6) Path

No ChangeLog.  r-.

[Eric Seidel (no email)]

Comment on attachment 34068 [details]
7) Color, Gradient, Image

No ChangeLog.  r-.

[Eric Seidel (no email)]

Please do this in multiple bugs, not just one bug.  And please make sure that you have George, or Nikolas, or some other experienced TorchMobile WebKit contributer work with you before posting these again.

[Yong Li]

(In reply to comment #36)
> (From update of attachment 33815 [details])
> No ChangeLog.  r-
> 
> Please have George review your patches before posting again.  As demonstrated
> by the previous 2 patches, these are not ready for real review yet and posting
> them for review with George at least looking at them is just wasting other
> folks time.  George should be helping his contractors to follow the WebKit
> process, not depending on reviewers to teach you all.

Sorry for wasting your time

[George Staikos]

(In reply to comment #36)
> (From update of attachment 33815 [details])
> No ChangeLog.  r-
> 
> Please have George review your patches before posting again.  As demonstrated
> by the previous 2 patches, these are not ready for real review yet and posting
> them for review with George at least looking at them is just wasting other
> folks time.  George should be helping his contractors to follow the WebKit
> process, not depending on reviewers to teach you all.

I can review all I want but they're here because other people should see them.  I'm not a babysitter and I don't work for you.  If it's such a problem, don't review them and leave them for someone else.

[Eric Seidel (no email)]

(In reply to comment #43)
> (In reply to comment #36)
> > (From update of attachment 33815 [details] [details])
> > No ChangeLog.  r-
> > 
> > Please have George review your patches before posting again.  As demonstrated
> > by the previous 2 patches, these are not ready for real review yet and posting
> > them for review with George at least looking at them is just wasting other
> > folks time.  George should be helping his contractors to follow the WebKit
> > process, not depending on reviewers to teach you all.
> 
> I can review all I want but they're here because other people should see them. 
> I'm not a babysitter and I don't work for you.  If it's such a problem, don't
> review them and leave them for someone else.

I don't work for you either. :)  You are paying these people to contribute to WebKit.  The WinCE patches have been an undue burden on the WebKit review queue as of late.  I know, because I just read through it twice in 2 days and reviewed most of the 50 bugs we just closed out. :)

I just ask that you work with the people you hire to improve the quality of patches posted to the review queue.  I'm very happy to review patches form the WinCE folks.  I just ask that they have a prayer of passing review before being posted.  In the case of this bug none of the patches even had ChangeLogs, let alone passing WebKit style.  In other bugs much of my r-ing of WinCE patches was about copy/paste code and patch size.

It seems that at least the lack of ChangeLogs, style issues (check-webkit-style) and patch size should be easy for you to work with the people you've hired to work on WebKit to solve.  Just as I work actively with the Google contributers to make sure that they are following WebKit process. :)

Thanks for your understanding.

[Adam Treat]

(In reply to comment #44)
> It seems that at least the lack of ChangeLogs, style issues
> (check-webkit-style) and patch size should be easy for you to work with the
> people you've hired to work on WebKit to solve.  Just as I work actively with
> the Google contributers to make sure that they are following WebKit process. :)
> 
> Thanks for your understanding.

I would like to say that your repeated notes of check-webkit-style seem out of place to me.  I know that Yong has been using this tool in submitting these patches. What's more, we have contributed an equal or greater amount of the current check-webkit-style checks and bugfixes as anyone else in the process of this upstreaming project.

In spite of all this, you should note that check-webkit-style is just a tool and an error prone one at that.  It has many false positives and it doesn't catch lots of style violations that it should.  We're trying to improve the tool and our code at the same time.

Thanks!

[Yong Li]

(In reply to comment #44)
> (In reply to comment #43)
> > (In reply to comment #36)
> > > (From update of attachment 33815 [details] [details] [details])
> > > No ChangeLog.  r-
> > > 
> > > Please have George review your patches before posting again.  As demonstrated
> > > by the previous 2 patches, these are not ready for real review yet and posting
> > > them for review with George at least looking at them is just wasting other
> > > folks time.  George should be helping his contractors to follow the WebKit
> > > process, not depending on reviewers to teach you all.
> > 
> > I can review all I want but they're here because other people should see them. 
> > I'm not a babysitter and I don't work for you.  If it's such a problem, don't
> > review them and leave them for someone else.
> 
> I don't work for you either. :)  You are paying these people to contribute to
> WebKit.  The WinCE patches have been an undue burden on the WebKit review queue
> as of late. I know, because I just read through it twice in 2 days and
> reviewed most of the 50 bugs we just closed out. :)
> 
> I just ask that you work with the people you hire to improve the quality of
> patches posted to the review queue.  I'm very happy to review patches form the
> WinCE folks.  I just ask that they have a prayer of passing review before being
> posted.  In the case of this bug none of the patches even had ChangeLogs, let
> alone passing WebKit style.  In other bugs much of my r-ing of WinCE patches
> was about copy/paste code and patch size.
> 
> It seems that at least the lack of ChangeLogs, style issues
> (check-webkit-style) and patch size should be easy for you to work with the
> people you've hired to work on WebKit to solve.  Just as I work actively with
> the Google contributers to make sure that they are following WebKit process. :)
> 
> Thanks for your understanding.

The ChangeLog is all in the first patch. Peter Kasting has pointed out that we should split the ChangeLog into different patches. We're going to do that anyway. But the source files are still good to review. Recently we post a lot of patches for WINCE port, because we have worked on this for more than 1.5 years, and just started upstreaming. But we don't expect that they can be committed in short time. We're not pushing you. But this kind of review ( r- with "No ChangeLog" ) won't take more than 0.5 min, right? Thanks for reviewing and commenting.

[Maciej Stachowiak]

Eric has a good point that the system works better if patches are submitted with simple structural issues addressed. However, we should also make an effort to be courteous to new contributors, even if their first few patches have some basic mistakes.

[Yong Li]

Created attachment 34391 [details]
1) Image encoders

[Yong Li]

Created attachment 34392 [details]
2) SharedBitmap

[Yong Li]

Created attachment 34393 [details]
2) SharedBitmap

[Yong Li]

Created attachment 34394 [details]
3) ImageBuffer

[Yong Li]

Created attachment 34395 [details]
4) ImageFrameSink (used as image decoding buffer)

[Yong Li]

Created attachment 34396 [details]
5) MediaPlayer

Written by Crystal Zhang

[Yong Li]

Created attachment 34398 [details]
6) Path PlatformPath

[Yong Li]

Created attachment 34400 [details]
7) Image ImageSource

[Yong Li]

Created attachment 34401 [details]
8) Color Gradient

[Peter Kasting]

Comment on attachment 34391 [details]
1) Image encoders

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.

[Yong Li]

(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.

[Peter Kasting]

(In reply to comment #58)
> > > +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"

Based on http://webkit.org/coding/coding-style.html , the struct name should be CamelCase (with the first letter capitalized), and the members should have m_ (I don't see an exception for structs and haven't heard this before).  Also the names should be more descriptive than "dest_mgr" or "pub"; use full words where possible.

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

I haven't used this technique with the WTF library so I'm pretty ignorant of its implementation detail.  If you don't declare the 2k capacity, is the vector guaranteed to heap-allocate no matter the size, or would it still do something smart for you under the hood?  How was 2k picked here -- was it arbitrary, or based on some data?  Is there a downside other than stack usage?  Comments would be helpful.

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

I cannot speak with perfect authority, but I did spend five years as a compiler engineer, and my impression is that your code is if anything less likely to be optimized than my suggestion.  (It's very likely the two generate the same code.)

It would be awesome if you had some sort of testcase that exercised a lot of this code in a way that could be timed.  Of course benchmarking inner loops is tricky given the effects of processor caches and the like, but such a thing would be immensely valuable in the future, where I believe we'll try to rearchitect the Image system to include the capabilities you need more directly, instead of as something of a fork (like you've had to maintain until now).  It would help reassure me that, for example, replacing some particular pixel-writing loop with a call to an inlined function did not actually hurt performance.  Otherwise, the best we can do is probably manual assembly inspection which is pretty error-prone when determining real perf.

> > > +        for (; pixel < pixelEnd; ) {
> > 
> > I find "while (pixel < pixelEnd) {" clearer.
> 
> Seems webkit prefer "while" to "for". Somehow I started hating "while" a few
> years ago.

I think I'm a bigger fan of for() (over while()) than most, since I tend to get nitpicky about minimizing variable scopes.  But in the case where there is no index variable declared and no iteration clause, while() means precisely what you're doing, whereas for() just results in adding ; ; and making things more opaque.  Precision in syntax is a virtue...

Anyway, glad you found the comments helpful.  I haven't reviewed the other patches, other things might arise there.

[Yong Li]

(In reply to comment #59)
> (In reply to comment #58)
> > > > +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"
> 
> Based on http://webkit.org/coding/coding-style.html , the struct name should be
> CamelCase (with the first letter capitalized), and the members should have m_
> (I don't see an exception for structs and haven't heard this before).  Also the
> names should be more descriptive than "dest_mgr" or "pub"; use full words where
> possible.
> 
> > > 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.
> 
> I haven't used this technique with the WTF library so I'm pretty ignorant of
> its implementation detail.  If you don't declare the 2k capacity, is the vector
> guaranteed to heap-allocate no matter the size, or would it still do something
> smart for you under the hood?  How was 2k picked here -- was it arbitrary, or
> based on some data?  Is there a downside other than stack usage?  Comments
> would be helpful.
> 
Heap allocation is slower than stack allocation. 2K is arbitrary.

> > > > +            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.
> 
> I cannot speak with perfect authority, but I did spend five years as a compiler
> engineer, and my impression is that your code is if anything less likely to be
> optimized than my suggestion.  (It's very likely the two generate the same
> code.)
> 
> It would be awesome if you had some sort of testcase that exercised a lot of
> this code in a way that could be timed.  Of course benchmarking inner loops is
> tricky given the effects of processor caches and the like, but such a thing
> would be immensely valuable in the future, where I believe we'll try to
> rearchitect the Image system to include the capabilities you need more
> directly, instead of as something of a fork (like you've had to maintain until
> now).  It would help reassure me that, for example, replacing some particular
> pixel-writing loop with a call to an inlined function did not actually hurt
> performance.  Otherwise, the best we can do is probably manual assembly
> inspection which is pretty error-prone when determining real perf.

In the case that compiler doesn't optimize, *foo++ may result this kind of machine code 
1) move foo to register
2) save *foo to register or some variable
3) increase the register by one that keeps foo
4) save the register back to stack

But if ++foo is called later, then = *foo moves foo to register once, and later ++foo does that again.

Of course, most compilers may be smart enough to optimize this when they find foo is not used in-between *foo and ++foo. Long time ago, it was said smaller C/C++ code could result fast machine code. This may be the reason why there's "++" operator in C. Because "foo += 1" is even more readable.

But the point is not that, I have no problem with choosing either one. I just think it may be a time waste on such small things. Each C++ programmer has his personal programming style, because everyone has different mind. I agree with that some basic coding style rules do help, but too many brainwashing things just reduce the fun of C++ programming and waste time. I believe that you would write very different code if you implemented it. If the reviewer was me, probably you would be asked to change *foo and ++foo to *foo++.

> 
> > > > +        for (; pixel < pixelEnd; ) {
> > > 
> > > I find "while (pixel < pixelEnd) {" clearer.
> > 
> > Seems webkit prefer "while" to "for". Somehow I started hating "while" a few
> > years ago.
> 
> I think I'm a bigger fan of for() (over while()) than most, since I tend to get
> nitpicky about minimizing variable scopes.  But in the case where there is no
> index variable declared and no iteration clause, while() means precisely what
> you're doing, whereas for() just results in adding ; ; and making things more
> opaque.  Precision in syntax is a virtue...

In this case, I agree that while() is better. But what's your opinion about "while(true)" vs. "for(;;)"?

[Yong Li]

(In reply to comment #59)
> (In reply to comment #58)
> > > > +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"
> 
> Based on http://webkit.org/coding/coding-style.html , the struct name should be
> CamelCase (with the first letter capitalized), and the members should have m_
> (I don't see an exception for structs and haven't heard this before).  Also the
> names should be more descriptive than "dest_mgr" or "pub"; use full words where
> possible.
> 

The struct name "jpeg_dest_mgr" follows libjpeg style. The existing JPEGImageDecoder.cpp does the same:

struct decoder_error_mgr {
    struct jpeg_error_mgr pub;  /* "public" fields for IJG library*/
    jmp_buf setjmp_buffer;      /* For handling catastropic errors */
};

About "m_" for the members in struct: this is a good example that one reviewer says this, but another says opposite. Sigh...

I'm going to use:

class JpegDestinationManager
{
public:
    Foo m_foo;
    ...
};

Will anyone say that the data member shouldn't be public? or suggest me to change back to struct?

[Eric Seidel (no email)]

Seems the style guide needs some updating:
http://webkit.org/coding/coding-style.html
it does not cover the struct Foo { int bar; } case.  (Whether it should be "bar" vs. "m_bar").

[Peter Kasting]

(In reply to comment #60)
> Heap allocation is slower than stack allocation. 2K is arbitrary.

I know heap allocation is slower :).  You didn't answer my question.  Does WTF::Vector have any code in it to automatically stack-allocate for smaller vectors?  (There are certainly Gecko classes that do...)

I would definitely comment in the code about 2k being arbitrary, and why you're doing it.

> In the case that compiler doesn't optimize, *foo++ may result this kind of
> machine code 
> 1) move foo to register
> 2) save *foo to register or some variable
> 3) increase the register by one that keeps foo
> 4) save the register back to stack
> 
> But if ++foo is called later, then = *foo moves foo to register once, and later
> ++foo does that again.

For RISC chips that need to have something in a register to update it, the compiler will allocate |foo| to a register for the body of the loop because it's the loop index.  For chips (like x86, IIRC... x86 assembly is not my strong suit) that can directly modify stack variables, no movement in and out of a register is necessary.

The reason I suggest putting ++pixel in the for() declaration is that it's much clearer to a reader what's happening in the loop -- you see the test and iteration on one line, not broken into pieces and hidden in a later statement's side effect.  From an optimization perspective, this kind of code helps the compiler convert the loop into a decrementing loop, unroll it, etc.

> But the point is not that, I have no problem with choosing either one. I just
> think it may be a time waste on such small things.

There is no WebKit style rule on this one, but I don't consider clarity a small thing, and I'll always suggest what I think is the clearest code.  Statements with side effects are inherently less clear, even if (as in this case) the code is simple enough that it's not too much more difficult to determine what the code is doing.

Note that writing your loop iteration as suggested also makes it slightly easier if we convert the code to use some kind of iterator or other interface to get at the source data -- which we will likely need to do when we combine this code with the other platforms'.  See also how the image decoders changed to not using direct memory access in their inner loops, in favor of an inline function in the header to do the right thing depending on the platform.

> In this case, I agree that while() is better. But what's your opinion about
> "while(true)" vs. "for(;;)"?

I have used both.  while(true) is significantly more common, and thus easier for most people to read, and therefore slightly better IMO.

(In reply to comment #61)
> About "m_" for the members in struct: this is a good example that one reviewer
> says this, but another says opposite. Sigh...
> 
> I'm going to use:
> 
> class JpegDestinationManager
> {
> public:
>     Foo m_foo;
>     ...
> };
> 
> Will anyone say that the data member shouldn't be public? or suggest me to
> change back to struct?

I don't know what WebKit style is on that last question.  Google style (which I only mention because it's the set of style rules I happen to know well) would suggest you use a struct rather than a class for something that's purely a container of public members, and personally I agree; I would leave it as a struct and change the names as you suggested.  Someone like Eric should have the last word on that kind of thing, though.

[Yong Li]

(In reply to comment #63)
> (In reply to comment #60)
> > Heap allocation is slower than stack allocation. 2K is arbitrary.
> 
> I know heap allocation is slower :).  You didn't answer my question.  Does
> WTF::Vector have any code in it to automatically stack-allocate for smaller
> vectors?  (There are certainly Gecko classes that do...)

No. I'm kind of familiar with Vector (I modified it a little bit, but the patch has not been post yet)

> 
> I would definitely comment in the code about 2k being arbitrary, and why you're
> doing it.

Just a guess, no other reason. 2K seems not too big, compared to other big buffer on stack, and canvas objects narrower than 683 can benefit.
> 
> There is no WebKit style rule on this one, but I don't consider clarity a small
> thing, and I'll always suggest what I think is the clearest code.  Statements
> with side effects are inherently less clear, even if (as in this case) the code
> is simple enough that it's not too much more difficult to determine what the
> code is doing.

Well, as I mentioned, += 1 might be clearer that ++. Making code too much readable just makes me feel that I'm not doing C++, or I'm doing C++ in MS style :) I may be sick at the point. But still, too many restrictions and requirements on such things make programming no more interesting, because one tries to write code as he was another person, tries to follow another person's mind. Although he can still do it, but there's no fun.

> 
> 
> > In this case, I agree that while() is better. But what's your opinion about
> > "while(true)" vs. "for(;;)"?
> 
> I have used both.  while(true) is significantly more common, and thus easier
> for most people to read, and therefore slightly better IMO.

Sigh. I don't know why "while(true)" is more common. Probably it's written in some programming books? Yeah, there's no such powerful "for(;;)" in languages other than C/C++, that may be the reason.

> 
> (In reply to comment #61)
> > About "m_" for the members in struct: this is a good example that one reviewer
> > says this, but another says opposite. Sigh...
> > 
> > I'm going to use:
> > 
> > class JpegDestinationManager
> > {
> > public:
> >     Foo m_foo;
> >     ...
> > };
> > 
> > Will anyone say that the data member shouldn't be public? or suggest me to
> > change back to struct?
> 
> I don't know what WebKit style is on that last question.  Google style (which I
> only mention because it's the set of style rules I happen to know well) would
> suggest you use a struct rather than a class for something that's purely a
> container of public members, and personally I agree; I would leave it as a
> struct and change the names as you suggested.  Someone like Eric should have
> the last word on that kind of thing, though.

It should be struct in this case, because it's used by libjpeg, like a C struct. Probably it should better be wrapped with "extern "C"", although it doesn't really change anything. And at the point, jpeg_dest_mgr is a better name.

"pub" acts as the base class of jpeg_dest_mgr. Another idea is:

struct JpegDestinationManager: jpeg_destination_mgr
{
};

What do you think?

[Peter Kasting]

(In reply to comment #64)
> Making code too much
> readable just makes me feel that I'm not doing C++, or I'm doing C++ in MS
> style :) I may be sick at the point. But still, too many restrictions and
> requirements on such things make programming no more interesting, because one
> tries to write code as he was another person, tries to follow another person's
> mind. Although he can still do it, but there's no fun.

I don't really know what to say.  In general style guides aren't designed around what you find fun.  But like I said, there's no WebKit style rule on this one.  I was giving my personal opinion.

If there was a style rule, and you said it wasn't very fun, I don't think it'd earn much sympathy...

> Sigh. I don't know why "while(true)" is more common.

Probably because "while" was designed for the case when you don't need a loop initializer or increment.

> It should be struct in this case, because it's used by libjpeg, like a C
> struct.

I don't have the contents of libjpeg.h in front of me so it's hard for me to say exactly how something is used, but it looks to me like this struct is entirely your own, set up in compressBitmapToJpeg() and then read in the various helper functions you define.  And I can't see how the libjpeg sources could make use of a type defined in your code rather than theirs.  From context here it looks like they let you stick some user data on the jpeg_compress_struct they define.  In that case, you should define the struct in a way that matches WebKit style.

> "pub" acts as the base class of jpeg_dest_mgr. Another idea is:
> 
> struct JpegDestinationManager: jpeg_destination_mgr
> {
> };
> 
> What do you think?

I think that is not good practice if the libjpeg C code expects to access a jpeg_destination_mgr struct at offset 0 in this object.  It probably works, but passing C++ classes with inheritance into C code to use makes me uneasy.

[Yong Li]

(In reply to comment #65)
> 
> Probably because "while" was designed for the case when you don't need a loop
> initializer or increment.

where "for" is designed for all the cases, including the case that no condition check is needed. isn't empty expression better than "true"? :)

> 
> > It should be struct in this case, because it's used by libjpeg, like a C
> > struct.
> 
> > "pub" acts as the base class of jpeg_dest_mgr. Another idea is:
> > 
> > struct JpegDestinationManager: jpeg_destination_mgr
> > {
> > };
> > 
> > What do you think?
> 
> I think that is not good practice if the libjpeg C code expects to access a
> jpeg_destination_mgr struct at offset 0 in this object.  It probably works, but
> passing C++ classes with inheritance into C code to use makes me uneasy.

Using class inheritance can save a reinterpret_cast, and also a weird member "pub". Only thing I should take care is providing a constructor to zero the base class memory. Hm..., I find a bug: using fastCalloc to construct jpeg_dest_mgr is wrong, those Vector members might not be happy with memory zeroed although it works in reality.

going to update the patch tomorrow.

[Adam Treat]

(In reply to comment #66)
> (In reply to comment #65)
> > 
> > Probably because "while" was designed for the case when you don't need a loop
> > initializer or increment.
> 
> where "for" is designed for all the cases, including the case that no condition
> check is needed. isn't empty expression better than "true"? :)

Yong, pkasting: this kind of conversation should be done at the pub over a beer or in an IRC private message if the subject matter really is that fascinating to you guys.  Personally, I find the debate to be void of any real signal and really high on the noise.

This bug report should focus on the actual substantive code.  Not trivial issues like this.  The whole purpose of review is to make the code better and to (hopefully) make ourselves better at what we do, not to devolve into ridiculous arguments over absolutely trivial and personal coding styles.

[Adam Treat]

(In reply to comment #67)
> (In reply to comment #66)
> > (In reply to comment #65)
> > > 
> > > Probably because "while" was designed for the case when you don't need a loop
> > > initializer or increment.
> > 
> > where "for" is designed for all the cases, including the case that no condition
> > check is needed. isn't empty expression better than "true"? :)
> 
> Yong, pkasting: this kind of conversation should be done at the pub over a beer
> or in an IRC private message if the subject matter really is that fascinating
> to you guys.  Personally, I find the debate to be void of any real signal and
> really high on the noise.
> 
> This bug report should focus on the actual substantive code.  Not trivial
> issues like this.  The whole purpose of review is to make the code better and
> to (hopefully) make ourselves better at what we do, not to devolve into
> ridiculous arguments over absolutely trivial and personal coding styles.

Uhm, I'm sorry. That came off way harsh...  Rather, in the interest of being positive let me just say that after 60+ comments it'd probably be a good idea to refocus on more substantive parts. :)

[Eric Seidel (no email)]

Comment on attachment 34394 [details]
3) ImageBuffer

The ChangeLog really should have more information on what these classes are for, what they do.  Why does the WinCE port use this new "SharedBitmap" and what is it for?  It seems ImageBufferData is just to allow ImageBuffer to be backed by a SharedBitmap, the question is why? ;)  Likewise BufferedImage seems to exist so that Image can be backed by a ImageBufferData.  Why wouldn't WinCE have used the BitmapImage abstraction that other classes use?  Image exists to abstract between SVGImage, PDFDocumentImage and BitmapImage.  Seems you have created a duplicate/parallel BitmapImage-like subclass, no?

(Not necessarily giving objection here, just seeking more information!) :)

Need more info in the ChangeLog to perform a good review.  Also, I think given how comment-full this bug is now, these patches really should be each on their own bugs so they can be discussed where folks have some prayer of being able to read all the relevant comments. :)

Indent:
+: m_data(size)
+, m_size(size)

UNUSED_ARG is what you want:
+    grayScale; // Currently not used

c++ casts:
+    const unsigned char* src = (unsigned char*)m_data.m_bitmap->bytes();

r-, mostly for the lack of ChangeLog information.

[Eric Seidel (no email)]

Comment on attachment 34401 [details]
8) Color Gradient

OK.

[Eric Seidel (no email)]

Comment on attachment 34398 [details]
6) Path PlatformPath

Neat that you wrote your own Path implementation.  Sad that you had to. :(

We avoid using "default:" when switching enums so that the compiler can catch modifications to that enum/missing case statements:
15     //case PathCloseSubpath:
 316     default:

I would have broken "contains" up into static-inline helper functions.

Again here:
65     //case PathCloseSubpath:
 466     default:

Indent:
 472 : m_penLifted(true)

I'll have to read this more closely later.  Probably needs its own bug.

[Eric Seidel (no email)]

Comment on attachment 34393 [details]
2) SharedBitmap

_set/_clear is not normal WK style naming.

indents for:
+: m_bmpInfo(bmpInfo)
+, m_locked(false)
etc.

naming
+    SharedBitmap* rtn = new SharedBitmap(is16bit, w, h, initPixels);

I'll have to look closer later.

[Eric Seidel (no email)]

I think especially after Peter and Yong's long discussion about one of these patches, we should land the approved patches and push the rest of onto their own bugs.  Some of these are certain to generate further commentary since there is a lot of new code to be reviewed here.  It's nice to see more of the "meat" of the WinCE port landing.

[Yong Li]

(In reply to comment #71)
> (From update of attachment 34398 [details])
> Neat that you wrote your own Path implementation.  Sad that you had to. :(
> 
> We avoid using "default:" when switching enums so that the compiler can catch
> modifications to that enum/missing case statements:
> 15     //case PathCloseSubpath:
>  316     default:
> 

Not all ports use same compiler. I've seen some coding rules (not webkit one) saying "default:" should always present in a switch block, and like this idea. In this case, the compiler won't generate code to compare the value with PathCloseSubpath. Condition checks are expensive on ARM. Probably I should add ASSERT ( ... == PathCloseSubpath); after "default:". Then that looks perfect.

[Eric Seidel (no email)]

(In reply to comment #74)
> (In reply to comment #71)
> > (From update of attachment 34398 [details] [details])
> > Neat that you wrote your own Path implementation.  Sad that you had to. :(
> > 
> > We avoid using "default:" when switching enums so that the compiler can catch
> > modifications to that enum/missing case statements:
> > 15     //case PathCloseSubpath:
> >  316     default:
> > 
> 
> Not all ports use same compiler. I've seen some coding rules (not webkit one)
> saying "default:" should always present in a switch block, and like this idea.
> In this case, the compiler won't generate code to compare the value with
> PathCloseSubpath. Condition checks are expensive on ARM. Probably I should add
> ASSERT ( ... == PathCloseSubpath); after "default:". Then that looks perfect.

Sounds like pre-mature optimization to me. :)

The "default:" logic used in WebKit is about trying to catch errors at runtime instead of compile time.

I don't think it's a big deal either way.  But I don't think optimizing for some one compiler that you've used at one point in your lifetime is ever good plan.  This code will be read by many people (hence the style guidelines), compiled by many different compilers and run on many different CPUs before it's removed form the WebKit repository. :)  If testing show that this is slow in a certain configuration then we certainly should change it.  But before then, I wouldn't worry about the compiler much.  Perf testing is likely to show that we have much larger fish to fry.

[Yong Li]

(In reply to comment #69)
> (From update of attachment 34394 [details])
> The ChangeLog really should have more information on what these classes are
> for, what they do.  Why does the WinCE port use this new "SharedBitmap" and
> what is it for?  It seems ImageBufferData is just to allow ImageBuffer to be
> backed by a SharedBitmap, the question is why? ;)  Likewise BufferedImage seems
> to exist so that Image can be backed by a ImageBufferData.  Why wouldn't WinCE
> have used the BitmapImage abstraction that other classes use?  Image exists to
> abstract between SVGImage, PDFDocumentImage and BitmapImage.  Seems you have
> created a duplicate/parallel BitmapImage-like subclass, no?
> 
> (Not necessarily giving objection here, just seeking more information!) :)
> 
> Need more info in the ChangeLog to perform a good review.  Also, I think given
> how comment-full this bug is now, these patches really should be each on their
> own bugs so they can be discussed where folks have some prayer of being able to
> read all the relevant comments. :)
> 

All ports use BitmapImage class, including WINCE port. But platform image type (defined as "NativeImagePtr") is still required. On Windows platform, it can be HBITMAP. SharedBitmap is a replacement of HBITMAP, and is more flexible and useful than HBITMAP.

I don't think this basic acknowledge of WebCore should go to ChangeLog. I've mentioned NativeImagePtr in there, and I think people who are familiar with WebCore image stuff can understand it with no problem.

If we need more description to help new webkit developers understand webkit source code, ChangeLog is not a good place to hold it.

(BTW, could you please give more info to bug 28019 why you think it's a wrong approach?)

[Eric Seidel (no email)]

(In reply to comment #76)
> (In reply to comment #69)
> > (From update of attachment 34394 [details] [details])
> All ports use BitmapImage class, including WINCE port. But platform image type
> (defined as "NativeImagePtr") is still required. On Windows platform, it can be
> HBITMAP. SharedBitmap is a replacement of HBITMAP, and is more flexible and
> useful than HBITMAP.
> 
> I don't think this basic acknowledge of WebCore should go to ChangeLog. I've
> mentioned NativeImagePtr in there, and I think people who are familiar with
> WebCore image stuff can understand it with no problem.

I'm about as familiar with WebKit Images as you can expect from a reviewer. :)  I've worked on the Image stuff several times, including the most recent re-factoring splitting out BitmapImage from Image.   Why does WinCE need a NativeImagePtr Image wrapper and other ports don't?  I guess other ports use BitmapImage to wrap their NativeImagePtr, no?

> If we need more description to help new webkit developers understand webkit
> source code, ChangeLog is not a good place to hold it.

Agreed.  The ChangeLog is about making your change understandable to those reviewing it and later people reading it.  If the code itself needs explanation that needs to go in the code.

ChangeLogs are in part about making your reviews obviously correct to those reviewing them.  Anything that seems non-obvious should be explained in plain english.

Non-obvious reviews tend to get r-'d or more likely just ignored until they rot.  As others (more famous than I) have noted, ones code is read many more times than it is written, and it's important to make it obviously correct to each of those people reading it. :)  This is part of why we have reviews and style guidelines.

I think we need to move these into separate bugs given how much parallel discussion is going on.  (Small bugs, with small patches is part of trying to make your patches obviously correct for quick review.  Once bugs or patches get to a certain size, reviewers tend to ignore them or r- them.)

[Yong Li]

(In reply to comment #77)
> (In reply to comment #76)
> > (In reply to comment #69)
> > > (From update of attachment 34394 [details] [details] [details])
> > All ports use BitmapImage class, including WINCE port. But platform image type
> > (defined as "NativeImagePtr") is still required. On Windows platform, it can be
> > HBITMAP. SharedBitmap is a replacement of HBITMAP, and is more flexible and
> > useful than HBITMAP.
> > 
> > I don't think this basic acknowledge of WebCore should go to ChangeLog. I've
> > mentioned NativeImagePtr in there, and I think people who are familiar with
> > WebCore image stuff can understand it with no problem.
> 
> I'm about as familiar with WebKit Images as you can expect from a reviewer. :) 
> I've worked on the Image stuff several times, including the most recent
> re-factoring splitting out BitmapImage from Image.   Why does WinCE need a
> NativeImagePtr Image wrapper and other ports don't?  I guess other ports use
> BitmapImage to wrap their NativeImagePtr, no?
> 

#if PLATFORM(WX)
class wxBitmap;
class wxGraphicsBitmap;
#elif PLATFORM(CG)
typedef struct CGImageSource* CGImageSourceRef;
typedef struct CGImage* CGImageRef;
typedef const struct __CFData* CFDataRef;
#elif PLATFORM(QT)
#include <qglobal.h>
QT_BEGIN_NAMESPACE
class QPixmap;
QT_END_NAMESPACE
#elif PLATFORM(CAIRO)
struct _cairo_surface;
typedef struct _cairo_surface cairo_surface_t;
#elif PLATFORM(SKIA)
class NativeImageSkia;
#elif PLATFORM(WINCE)
#include "SharedBitmap.h"
#endif

[Yong Li]

(In reply to comment #78)
> (In reply to comment #77)
> > (In reply to comment #76)
> > > (In reply to comment #69)
> > > > (From update of attachment 34394 [details] [details] [details] [details])
> > > All ports use BitmapImage class, including WINCE port. But platform image type
> > > (defined as "NativeImagePtr") is still required. On Windows platform, it can be
> > > HBITMAP. SharedBitmap is a replacement of HBITMAP, and is more flexible and
> > > useful than HBITMAP.
> > > 
> > > I don't think this basic acknowledge of WebCore should go to ChangeLog. I've
> > > mentioned NativeImagePtr in there, and I think people who are familiar with
> > > WebCore image stuff can understand it with no problem.
> > 
> > I'm about as familiar with WebKit Images as you can expect from a reviewer. :) 
> > I've worked on the Image stuff several times, including the most recent
> > re-factoring splitting out BitmapImage from Image.   Why does WinCE need a
> > NativeImagePtr Image wrapper and other ports don't?  I guess other ports use
> > BitmapImage to wrap their NativeImagePtr, no?
> > 
> 
#if PLATFORM(WX)
class ImageDecoder;
typedef ImageDecoder* NativeImageSourcePtr;
typedef const Vector<char>* NativeBytePtr;
#if USE(WXGC)
typedef wxGraphicsBitmap* NativeImagePtr;
#else
typedef wxBitmap* NativeImagePtr;
#endif
#elif PLATFORM(CG)
typedef CGImageSourceRef NativeImageSourcePtr;
typedef CGImageRef NativeImagePtr;
#elif PLATFORM(QT)
class ImageDecoderQt;
typedef ImageDecoderQt* NativeImageSourcePtr;
typedef QPixmap* NativeImagePtr;
#elif PLATFORM(CAIRO)
class ImageDecoder;
typedef ImageDecoder* NativeImageSourcePtr;
typedef cairo_surface_t* NativeImagePtr;
#elif PLATFORM(SKIA)
class ImageDecoder;
typedef ImageDecoder* NativeImageSourcePtr;
typedef NativeImageSkia* NativeImagePtr;
#elif PLATFORM(WINCE)
class ImageDecoder;
typedef ImageDecoder* NativeImageSourcePtr;
typedef RefPtr<SharedBitmap> NativeImagePtr;
#endif

[Yong Li]

Going to create a new bug for this. Just want to mention:

(In reply to comment #57)
> 
> > +    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?

I just remember that this is necessary. If I declare rowBuffer below "setjmp", then when the block in "setjmp" is reached, the destructor of rowBuffer won't be called. So it leaks. going to add a comment

[Peter Kasting]

(In reply to comment #79)
> > > > I don't think this basic acknowledge of WebCore should go to ChangeLog. I've
> > > > mentioned NativeImagePtr in there, and I think people who are familiar with
> > > > WebCore image stuff can understand it with no problem.
> > > 
> > > I'm about as familiar with WebKit Images as you can expect from a 

FWIW, I agree with Eric.  I've worked on the Image code as much as anyone (probably more) lately, and I don't understand why you're doing this.

> #elif PLATFORM(WINCE)
> class ImageDecoder;
> typedef ImageDecoder* NativeImageSourcePtr;
> typedef RefPtr<SharedBitmap> NativeImagePtr;
> #endif

The RefPtr<> here really scares me.  Every other port uses a raw pointer.  I fought for weeks (spread over two years) with memory problems in the Skia port before we managed to understand and precisely duplicate the CG and Cairo models of memory management for this object.  (For reference, we originally did things much like you're doing here.)  I am not sure if the cross-platform code will be both correct and efficient with this kind of usage, and it will definitely be difficult for others to understand what you're doing.  I strongly recommend you use a raw pointer and allocate/free it at the same points the other ports do.

In order to avoid deep copying, your native image object can internally refcount.  That's how the other ports address this issue.

[Yong Li]

(In reply to comment #81)
> (In reply to comment #79)
> > > > > I don't think this basic acknowledge of WebCore should go to ChangeLog. I've
> > > > > mentioned NativeImagePtr in there, and I think people who are familiar with
> > > > > WebCore image stuff can understand it with no problem.
> > > > 
> > > > I'm about as familiar with WebKit Images as you can expect from a 
> 
> FWIW, I agree with Eric.  I've worked on the Image code as much as anyone
> (probably more) lately, and I don't understand why you're doing this.

We are not using Qt, CG, Cairo or any other graphics library. We just use native WINCE API. HBITMAP is ugly and some GDI APIs doesn't even exist on WINCE. Think about QPixmap, CGImageRef...

> 
> > #elif PLATFORM(WINCE)
> > class ImageDecoder;
> > typedef ImageDecoder* NativeImageSourcePtr;
> > typedef RefPtr<SharedBitmap> NativeImagePtr;
> > #endif
> 
> The RefPtr<> here really scares me.  Every other port uses a raw pointer.  I
> fought for weeks (spread over two years) with memory problems in the Skia port
> before we managed to understand and precisely duplicate the CG and Cairo models
> of memory management for this object.  (For reference, we originally did things
> much like you're doing here.)  I am not sure if the cross-platform code will be
> both correct and efficient with this kind of usage, and it will definitely be
> difficult for others to understand what you're doing.  I strongly recommend you
> use a raw pointer and allocate/free it at the same points the other ports do.
> 
> In order to avoid deep copying, your native image object can internally
> refcount.  That's how the other ports address this issue.

Every other port? What is this?

typedef CGImageRef NativeImagePtr;

Using smart pointers is definitely an advantage, for example, it can avoid memory leaks (about 1.5 year ago, there was a bug that FrameData leaked. Now it has been fixed. but if all platform NativeImagePtr were reference counted, it wouldn't even be a problem, and "clear(true)" wouldn't necessary in ~FrameData()). Not mentioning that we use SharedBitmap as both decoding buffer and rendering buffer so that no extra memory copying is requried.

Generally, I don't think it's a good idea to throw away helpful changes just because we want to be same as other ports. It is supposed to be platform-specific in this case. If we do everything in the same way, why do we need ports?

[Peter Kasting]

Please follow Eric's suggestion and split these patches into their own bugs.  This is getting unwieldy.

(In reply to comment #82)
> Every other port? What is this?
> 
> typedef CGImageRef NativeImagePtr;

From the way the CG code uses that it seems like it works like a typedef to a pointer to a refcounting object.  They're certainly still explicitly releasing it: see the CGImageRelease() call in ImageCG.cpp:FrameData::clear().

But perhaps I am mistaken about what that type is.  

> Using smart pointers is definitely an advantage, for example, it can avoid
> memory leaks (about 1.5 year ago, there was a bug that FrameData leaked. Now it
> has been fixed. but if all platform NativeImagePtr were reference counted, it
> wouldn't even be a problem, and "clear(true)" wouldn't necessary in
> ~FrameData()).

As noted above, even CG does an explicit release here.  I am not assuaged.

> Not mentioning that we use SharedBitmap as both decoding buffer
> and rendering buffer so that no extra memory copying is requried.

Yes, that is how other ports e.g. CG and Skia work too.  For Skia, for example, the setRGBA() call in ImageDecoder.h writes directly to the buffer inside the SkBitmap.

> Generally, I don't think it's a good idea to throw away helpful changes just
> because we want to be same as other ports.

Generally, things that one port does differently from everyone else raise red flags for people trying to read the code.

> If we do everything in the same way, why do we
> need ports?

Because all the ports are built atop cross-platform code.  They should only be different where they must, and we're in the process of trying to reduce the differences further.

I wasn't saying your code is guaranteed to be wrong.  I was saying I'm unable to tell for sure that it's right, whereas if your NativeImage type internally refcounted, and NativeImagePtr was a raw pointer, I wouldn't even have to worry, because the idiom is the same.  All I'd need to check is that you allocated and freed in the right places.  The strength of copying other ports is that we can review your code and guarantee it's correct, and others can read and understand what you're doing.  It's the same reason we have style guides.

[Yong Li]

(In reply to comment #69)
> (From update of attachment 34394 [details])
> The ChangeLog really should have more information on what these classes are
> for, what they do.  Why does the WinCE port use this new "SharedBitmap" and
> what is it for?  It seems ImageBufferData is just to allow ImageBuffer to be
> backed by a SharedBitmap, the question is why? ;)  Likewise BufferedImage seems
> to exist so that Image can be backed by a ImageBufferData.  Why wouldn't WinCE
> have used the BitmapImage abstraction that other classes use?  Image exists to
> abstract between SVGImage, PDFDocumentImage and BitmapImage.  Seems you have
> created a duplicate/parallel BitmapImage-like subclass, no?
> 
> (Not necessarily giving objection here, just seeking more information!) :)
> 
> Need more info in the ChangeLog to perform a good review.  Also, I think given
> how comment-full this bug is now, these patches really should be each on their
> own bugs so they can be discussed where folks have some prayer of being able to
> read all the relevant comments. :)
> 
> Indent:
> +: m_data(size)
> +, m_size(size)
> 
> UNUSED_ARG is what you want:
> +    grayScale; // Currently not used
> 
> c++ casts:
> +    const unsigned char* src = (unsigned char*)m_data.m_bitmap->bytes();
> 
> r-, mostly for the lack of ChangeLog information.

Sorry, I misunderstood your question. Yes, BufferedImage is parallel to BitmapImage. The reason we use BufferedImage is that BitmapImage is too expensive in this case. Another option is to implement BitmapImage(NativeImagePtr, ImageObserver* = 0) as other ports do.

ImageBuffer::image() is supposed to return an Image interface which is used only for render. BufferedImage is very thin, and I think it makes things much simpler than doing it in another way.

[George Staikos]

Comment on attachment 34396 [details]
5) MediaPlayer

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 3afc825..2facda8 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,16 @@
> [phone]  Yong Li  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        WINCE PORT: MediaPlayerProxy and MediaPlayerPrivate
> +        https://bugs.webkit.org/show_bug.cgi?id=27511
> +
> +        Written by Crystal Zhang <[e-mail]>
> +
> +        * platform/graphics/wince/MediaPlayerPrivateWince.h: Added.
> +        * platform/graphics/wince/MediaPlayerProxy.cpp: Added.
> +        * platform/graphics/wince/MediaPlayerProxy.h: Added.


  Should probably put Crystal's name in the ChangeLog entry instead.


> +WebMediaPlayerProxy::WebMediaPlayerProxy(MediaPlayer* player)
> +: m_mediaPlayer(player)
> +, m_init(false)
> +, m_hasSentResponseToPlugin(false)

   Need indenting


> +WebMediaPlayerProxy::~WebMediaPlayerProxy()
> +{
> +    m_instance.release();
> +}
> +ScriptInstance WebMediaPlayerProxy::pluginInstance()

  Needs a blank line


> +}
> +void WebMediaPlayerProxy::load(const String& url)
> +{
> +    if (!m_init)
> +        initEngine();
> +    if (m_init)
> +        invokeMethod("play");
> +}
> +void WebMediaPlayerProxy::initEngine()

  More blank lines missing


> +    NamedNodeMap* a = element->attributes();
> +    if (a) {
> +        for (unsigned i = 0; i < a->length(); ++i) {
> +            Attribute* it = a->attributeItem(i);
> +            paramNames.append(it->name().localName().string());
> +            paramValues.append(it->value().string());
> +        }
> +    }

   I guess you should remove the braces around the if, as ugly as it becomes.


> +HTMLMediaElement* WebMediaPlayerProxy::element()
> +{
> +    return static_cast<HTMLMediaElement*>(m_mediaPlayer->mediaPlayerClient());
> +
> +}

   Extra blank line here

> +
> +void WebMediaPlayerProxy::invokeMethod(const String& methodName)
> +{
> +    Frame* frame = element()->document()->frame();
> +    RootObject *root = frame->script()->bindingRootObject();
> +    if (!root)
> +        return;
> +    ExecState *exec = root->globalObject()->globalExec();
> +    Instance* instance = pluginInstance().get();
> +    if (!instance)
> +        return;
> +
> +    instance->begin();
> +    Class *aClass = instance->getClass();
> +    Identifier iden(exec, methodName);
> +    MethodList methodList = aClass->methodsNamed(iden, instance);
> +    ArgList args;
> +    /*JSValuePtr result =*/
> +    instance->invokeMethod(exec, methodList , args);
> +    instance->end();


   This needs a comment to indicate why the result variable is commented out I think.


   After those minor cleanups it will be good to go.

[Darin Adler]

> > +    NamedNodeMap* a = element->attributes();
> > +    if (a) {
> > +        for (unsigned i = 0; i < a->length(); ++i) {
> > +            Attribute* it = a->attributeItem(i);
> > +            paramNames.append(it->name().localName().string());
> > +            paramValues.append(it->value().string());
> > +        }
> > +    }
> 
>    I guess you should remove the braces around the if, as ugly as it becomes.

If I understand you correctly, I think you're saying that you think the WebKit coding style requires removing these braces.

That's wrong. The WebKit coding style says braces should be omitted from one-line bodies, and this is more than one line. Perhaps you were thinking it said to omit them from one-statement bodies?

The braces should stay.

[George Staikos]

(In reply to comment #86)
> > > +    NamedNodeMap* a = element->attributes();
> > > +    if (a) {
> > > +        for (unsigned i = 0; i < a->length(); ++i) {
> > > +            Attribute* it = a->attributeItem(i);
> > > +            paramNames.append(it->name().localName().string());
> > > +            paramValues.append(it->value().string());
> > > +        }
> > > +    }
> > 
> >    I guess you should remove the braces around the if, as ugly as it becomes.
> 
> If I understand you correctly, I think you're saying that you think the WebKit
> coding style requires removing these braces.
> 
> That's wrong. The WebKit coding style says braces should be omitted from
> one-line bodies, and this is more than one line. Perhaps you were thinking it
> said to omit them from one-statement bodies?
> 
> The braces should stay.

 +1!

That's not clear from my reading but I definitely approve.

[Darin Adler]

(In reply to comment #87)
> That's not clear from my reading but I definitely approve.

Did you look at the examples?

[George Staikos]

(In reply to comment #88)
> (In reply to comment #87)
> > That's not clear from my reading but I definitely approve.
> 
> Did you look at the examples?

Yes, and it doesn't strictly fit into the "braces" example by my read, but I know what you're implying.  On another note I have seen the "if + comment + one liner" rule being applied in reverse but I don't remember where at the moment.  Thankfully the compiler is forgiving. :)

[Adam Treat]

Comment on attachment 34400 [details]
7) Image ImageSource

ImageSource is no longer a forked file for each port.  Rather, all ports that are use the built-in image decoders are moving to a cross-platform ImageSource.cpp file.  Please refactor to use this new file and (only if necessary) add #if/#else for any behavior changes need by WINCE port for this file.  See: https://bugs.webkit.org/show_bug.cgi?id=27965

[Peter Kasting]

(In reply to comment #90)
> ImageSource is no longer a forked file for each port.  Rather, all ports that
> are use the built-in image decoders are moving to a cross-platform
> ImageSource.cpp file.  Please refactor to use this new file and (only if
> necessary) add #if/#else for any behavior changes need by WINCE port for this
> file.  See: https://bugs.webkit.org/show_bug.cgi?id=27965

And if behavior differences are needed, chat with me/Adam; maybe we can find a better way than #ifdefs (one or two virtual functions, or splitting ImageSource.cpp into a couple pieces, for example).

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	A	WebCore/platform/graphics/wince/ColorWince.cpp
	A	WebCore/platform/graphics/wince/GradientWince.cpp
Committed r47158
	M	WebCore/ChangeLog
	A	WebCore/platform/graphics/wince/ColorWince.cpp
	A	WebCore/platform/graphics/wince/GradientWince.cpp
r47158 = 4c84387c23d3b46cd7698b406d8ce9d740430bcd (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47158

[Eric Seidel (no email)]

Comment on attachment 34401 [details]
8) Color Gradient

Hit bug 28230 again.  multi-patch bugs still confuse our tools. :)

[Eric Seidel (no email)]

Hit bug 28230 again.

[Yong Li]

Comment on attachment 34401 [details]
8) Color Gradient

seems already landed?

[Eric Seidel (no email)]

Comment on attachment 34401 [details]
8) Color Gradient

bugzilla-tool isn't quite smart enough to know how to reject this patch yet.  It is smart enough to know that you're not a reviewer however. :)  Only reviewers can set r+.

Exception: Committer "Yong Li" <[e-mail]> does not have review rights.

Documentation of the review process is part of the committer agreement that you signed. :)

http://webkit.org/quality/lifecycle.html has more information if you need.

[Yong Li]

(In reply to comment #96)
> (From update of attachment 34401 [details])
> bugzilla-tool isn't quite smart enough to know how to reject this patch yet. 
> It is smart enough to know that you're not a reviewer however. :)  Only
> reviewers can set r+.
> 
> Exception: Committer "Yong Li" <[e-mail]> does not have review
> rights.
> 
> Documentation of the review process is part of the committer agreement that you
> signed. :)
> 
> http://webkit.org/quality/lifecycle.html has more information if you need.

that patch is already landed. just want to mark it with something other than "No flag".

[Eric Seidel (no email)]

Closing bug since all patches have been landed.

[Yong Li]

apparently it depends on 3 child bugs