[webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)

[Ryosuke Niwa]

On Wed, Jul 11, 2012 at 6:54 AM, John Mellor <johnme at chromium.org> wrote:

> On Fri, Jul 6, 2012 at 11:49 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
>
>> On Jul 6, 2012 3:16 PM, "Per Bothner" <per.bothner at oracle.com> wrote:
>> > On 07/06/2012 02:02 PM, Ryosuke Niwa wrote:
>>
> >> On Fri, Jul 6, 2012 at 12:54 PM, Per Bothner <per.bothner at oracle.com>
>> wrote:
>>
> >>> You're deluding yourself if you think the code (or any code this
>>
> >>> large and complicated) is or can be self-evident.
>>
> >>
>>
> >> That's a pretty strong claim. I find that the vast majority of codebase
>>
> >> to be quite self-evident.
>>
> >
>>
> > If you (who I gather has quite a bit of WebKit experience) find
>> something
>>
> > self-evident, does not mean it is is.
>>
>> To be fair, as I said, I was able to read WebKit code without much
>> trouble 2-3 months into my internship. I'm not sympathetic to people who
>> can't read code as well as an undergraduate college student.
>>
>> Also, we need require some kind of prior knowledge of things. For
>> example, InlineBox might be a foreign concept to you but it's plain dead
>> obvious for anyone who has read CSS2.1 specifications.
>>
>> Similarly, there might be some classes that are self-explanatory for
>> someone who knows ES5 well and confusing for someone who is not.
>>
>> Where should we draw a line then?
>>
> Even obvious (to some) concepts like InlineBox have subtleties, for
> example not all inline-level elements have inline boxes. An unambiguous
> class-level comment could make this clearer, for example:
>
> // An inline box represents a rectangle that occurs on a line,
> corresponding to
> // all or part of some RenderObject. It must be inline-level and its
> contents
> // must participate in its containing inline formatting context. For
> example a
> // non-replaced element with a 'display' value of 'inline' generates an
> inline
> // box, as does an anonymous inline element (text directly contained
> inside a
> // block container element, not inside an inline element). But atomic
> // inline-level boxes (such as replaced inline-level elements, inline-block
> // elements, inline-table elements, and ruby elements) are not inline boxes
> // since they participate in their inline formatting context as a single
> // opaque box; these are handled by <insert class that deals with these>.
> // http://www.w3.org/TR/2011/REC-CSS2-20110607/visuren.html#inline-boxes
>

What's the point of adding this comment when the URL contains all the
information?  All we need is the URL.  If anything, we should be describing
the difference between the inline boxes in CSS2.1 and our implementation
instead.

Also, with that argument, we can start adding a WHOLE bunch of comments to
WebCore like what DOM node is, and what "mutation" means, what "content
attribute" is, etc... But we don't do that because we expect people to have
some domain-specific knowledge. Of course, I'm not opposed to adding
reference URLs as necessary since that'll be actually useful for
some obscure concepts.

- Ryosuke
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20120711/234da21e/attachment.html>