[webkit-dev] coding style and comments

[Adam Barth]

As an example of a harmful comment, this comment just wasted 15
minutes of my time:

http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.h#L351

The comment was true when written (although confusing because I
thought it meant that these functions are virtual so we should avoid
calling them and using encoding() directory), but the comment is no
longer meaningful because Document::encoding is no longer virtual.

IMHO, the maintenance burden of some comments often out-weighs their
value.  Some comments can be valuable.  For example, this comment has
been helpful to me in the past:

http://trac.webkit.org/browser/trunk/Source/WebCore/loader/FrameLoader.cpp#L2717

It saved me a bunch of time digging through the history of FrameLoader
to understand why that code was written the way it was.

Adam


On Sun, Jan 30, 2011 at 10:27 AM, Adam Barth <abarth at webkit.org> wrote:
> On Sun, Jan 30, 2011 at 7:40 AM, David Kilzer <ddkilzer at webkit.org> wrote:
>> An interesting case study would be the (still ongoing?) refactoring of the
>> loader code over the past few months (thanks to Adam Barth and others).   Is
>> it easier to understand now than before the refactoring started?  How many
>> comments were added in the process (FIXMEs notwithstanding)?
>
> We're not really adding that many comments.  Mostly we're breaking
> down monolithic objects like FrameLoader into more understandable
> bundles of state.  For example, NavigationScheduler is a lot more
> understandable now that it's separate from FrameLoader:
>
> http://trac.webkit.org/browser/trunk/WebCore/loader/FrameLoader.cpp?rev=48958
> http://trac.webkit.org/browser/trunk/Source/WebCore/loader/NavigationScheduler.cpp
>
> In that case, after breaking NavigationScheduler out of FrameLoader,
> the code got completely re-written to actually use C++ inheritance
> instead of faking it with structs.  A similar thing happened (although
> perhaps not as successfully) with HistoryController, which also used
> to be part of FrameLoader.
>
> More recently, we've been removing redundant state.  For example,
>
> http://trac.webkit.org/changeset/76702/trunk/Source/WebCore/loader/FrameLoader.cpp
>
> which means deleting a bunch of comments warning folks about the
> dangers of not keeping various pieces of state synchronized properly.
> I could be wrong, but I don't think we've added very many comments at
> all.
>
> Personally, I use FIXMEs a lot in new code.  For example, in the
> XSSFilter I'm working on, there are lots of FIXMEs that are notes
> about things I haven't gotten around to implementing yet (e.g., that
> the code needs to handle POST but doesn't yet) or ideas for
> optimizations that would be premature at this point (e.g., an extra
> memcpy that could be eliminated).
>
> Adam
>