On Jan 31, 2011, at 1:06 PM, Peter Kasting wrote:
> On Mon, Jan 31, 2011 at 12:47 PM, Maciej Stachowiak <[email protected]> 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
_______________________________________________
webkit-dev mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
