[webkit-reviews] review granted: [Bug 23102] turn on unused parameter warnings in WebCore : [Attachment 26414] step 1 -- covers the simplest cases

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 23102: turn on unused parameter warnings in WebCore
https://bugs.webkit.org/show_bug.cgi?id=23102

Attachment 26414: step 1 -- covers the simplest cases
https://bugs.webkit.org/attachment.cgi?id=26414&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
r=me

Is there any good in leaving long lists of changed files in ChangeLog when
there are no per-file comments? I never found that useful, and did actually
find it a little harmful, as they tend to show up as false hits when searching
for meaningful changes.

-    ASSERT(m_handle == handle);

I find this kind of assertions extremely helpful as documentation at list, and
would prefer to have them kept. You could use ASSERT_UNUSED from my patch that
is pending review now.

+void Frame::adjustPageHeight(float *newBottom, float oldTop, float oldBottom,
float /*bottomLimit*/)

You could move a star here.

+void InitArenaPool(ArenaPool *pool, const char *, unsigned int size, unsigned
int align)

And here.

-    virtual PassRefPtr<TransformOperation> blend(const TransformOperation*
from, double progress, bool blendToIdentity = false)
+    virtual PassRefPtr<TransformOperation> blend(const TransformOperation*,
double, bool)

I don't think it's good style to have default arguments on base class version
of a function, but not on overrides - this makes it important which version to
use at call sites, which is against the purpose of virtual functions.