[webkit-dev] Review Queue

[Adam Barth]

On Wed, Sep 2, 2009 at 9:45 AM, Yong Li<yong.li at torchmobile.com> wrote:
> I'm a very first user of check-webkit-style (was called cpplint when I used
> it), and reported some bugs of it. The current indentation issue of some
> construtor inialization list were actually introduced by following the
> suggestion of old version cpplint. It must have been improved a lot, so I
> should run it again.

If check-webkit-style told you to indent the initializer list flush to
the right, then it's got a bug.  :)

See run 1 under "other punctuation" in

http://webkit.org/coding/coding-style.html

> Thank you so much for those comments. I like most of them. But I have
> questions about something:
>
> 1. one class one file
>
> I agree that it's a good style to create a file for each class. But
> sometimes the class is very light, and is only useful to one module, in this
> case putting it in the same file won't hurt anything. Second, there're a lot
> of such cases in webkit existing files, including some committed in near
> future.

Turns out I can't find this rule in style guide.  I might have made it
up or imported from another style guide.  In general, it's a better
idea to have short source files that accomplish a well-defined,
easy-to-verify task.  One class per file helps here.

> So why is it the reason for r- bug 28167?

I'll look at that patch in more detail again.

> 2. static initializer forbidden
>
> I'm not aware of this. Is it added in webkit code guidline?

I'm not sure if it's the in the style guide, but static initializers
won't even compile on some platforms (e.g., mac).  They run at
unpredictable times and impact startup performance.  Typically the way
we work around this is by having a function you call to get a pointer
to the object and keeping the static as a local in that function using
the DEFINE_STATIC_LOCAL macro.  You can search the code base for
examples of this pattern.

> Most the patches are for WINCE port only, and they won't affect any other
> existing port. Also, they are not read-only after being committed. So if we
> think some of them should be refactored or extracted out for sharing, we can
> always do it in the future. The png encoder of skia can definitely be shared
> by other ports, but the files were still committed to image-encoders\skia.
> Why are people requiring so much for brand-new wince port?

WebKit has a high bar for code quality.  Other ports (e.g., Chromium)
when through similar review processes when they landed too.  There are
certainly other development models which favor
commit-first-then-revise.  Historically, WebKit has taken the approach
of breaking large efforts like this up into small pieces and reviewing
them individually.

Honestly, I was probably a bit aggressive in r-ing your patches.
Hopefully you won't take that personally.  My hope was to get the ball
rolling again because the work on the WINCE port seemed to have
stalled out.  Having a bunch of r? patches languishing in the review
queue isn't really helpful to anyone.

Adam