[webkit-dev] coding style and comments

[Maciej Stachowiak]

On Jan 31, 2011, at 1:06 PM, Peter Kasting wrote:

> On Mon, Jan 31, 2011 at 12:47 PM, Maciej Stachowiak <mjs at apple.com> wrote:
> On Jan 31, 2011, at 11:01 AM, Peter Kasting wrote:
> I think people who favor comments tend to produce a lot of exactly this kind of comment. Except in some cases its verbose multiline comments that exceed the number of actual lines of code. Here's some more files with many comments that are better deleted or replaced with refactoring:
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/ImageDecoder.h
> http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/ImageDecoder.cpp
> 
> Oh good, files I know something about :)
> 
> To pick just one specific example:
> 
>    // ICOs always begin with a 2-byte 0 followed by a 2-byte 1.
>    // CURs begin with 2-byte 0 followed by 2-byte 2.
>    if (!memcmp(contents, "\x00\x00\x01\x00", 4) || !memcmp(contents, "\x00\x00\x02\x00", 4))
> 
> This would be more readable if the comments were deleted and the memcmps were replaced with calls to inline functions named hasICOMagicNumber/hasCURMagicNumber or the like.
> 
> Or just omitted altogether.  I don't think the comments here add much.  (They predate my involvement; they actually come from hyatt in 2006, and in fairness to him, this code at that time was very much "throw in something quick that works to get a Windows port up and running".)

Ye another option would be to use named constants for the magic strings.

> 
> Your choice of files is interesting otherwise though.  Aside from those 2006-era comments in create(), there are only three other comments in ImageDecoder.cpp, two of which seem worth having to me.  In ImageDecoder.h, many function declarations are commented, most to explain ownership details, caution users of functionality that needs to match in multiple places, or give more context to why callers would use the function.  There are definitely some comments lying around here that aren't terribly useful, but the majority of these have been helpful when tracing through various different image decoding bugs, especially memory errors triggered by our heuristic that throws away image data for large images sometimes, or when refreshing my memory on what this code does when I come back to it after a year of doing something else.
> 
> So I don't agree that "many" comments here (if "many" means "the majority") are unhelpful.  I guess, then, we do disagree about what types of comments are useful.

I do think the majority of comments in there are unhelpful. Here are some examples from the header that I hope speak for themselves:

        // Whether or not the size information has been decoded yet. This
        // default implementation just returns true if the size has been set and
        // we have not seen a failure. Decoders may want to override this to
        // lazily decode enough of the image to get the size.
        virtual bool isSizeAvailable()
        {
            return !m_failed && m_sizeAvailable; 
        }

        // Returns the size of the image.
        virtual IntSize size() const
        {
            return m_size;
        }

        // The total number of frames for the image.  Classes that support
        // multiple frames will scan the image data for the answer if they need
        // to (without necessarily decoding all of the individual frames).
        virtual size_t frameCount() { return 1; }

        // The number of repetitions to perform for an animation loop.
        virtual int repetitionCount() const { return cAnimationNone; }

        // Called to obtain the ImageFrame full of decoded data for rendering.
        // The decoder plugin will decode as much of the frame as it can before
        // handing back the buffer.
        virtual ImageFrame* frameBufferAtIndex(size_t) = 0;

        // Whether or not the underlying image format even supports alpha
        // transparency.
        virtual bool supportsAlpha() const { return true; }

It seems to me that none of these comments add info that is worth the code bloat.
Regards,
Maciej

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20110131/8cb81545/attachment.html>