[webkit-reviews] review denied: [Bug 23296] add Android platform-specific files to WebCore/platform : [Attachment 30005] new patch part 9 with ChangeLog

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri May 22 06:52:06 PDT 2009


Eric Seidel <eric at webkit.org> has denied Feng Qian <feng at chromium.org>'s
request for review:
Bug 23296: add Android platform-specific files to WebCore/platform
https://bugs.webkit.org/show_bug.cgi?id=23296

Attachment 30005: new patch part 9 with ChangeLog
https://bugs.webkit.org/attachment.cgi?id=30005&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
We don't generally commit commented out code:
 97 //void FrameView::updateBorder() { verifiedOk(); }

I'm surprised there are all these localized strings here when I just reviewed
another patch with unimplemented localized strings.  Seems these shoudl be
mvoed, no?

ASSERT(0);
we use ASSERT_NOT_REACHED()

In general this is fine.  Again if you were a commiter you could just fix it as
you landed, but as is, you should re-post this for someone else to land.

Please remove all the commetned out ones:
 718 //void Frame::clearPlatformScriptObjects() { notImplemented(); }

I think you could make all these 1-line definitions to make them more readable.
 Up to you.  But we generally found them more readable that way.  WK does not
have an 80 col limit like google does.

r- for the nit above.


More information about the webkit-reviews mailing list