[webkit-reviews] review denied: [Bug 33119] svn-apply breaks on patch files with leading white space : [Attachment 52855] proposed patch with unittests

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Apr 30 05:54:06 PDT 2010


Chris Jerdonek <cjerdonek at webkit.org> has denied Zoltan Horvath
<zoltan at webkit.org>'s request for review:
Bug 33119: svn-apply breaks on patch files with leading white space
https://bugs.webkit.org/show_bug.cgi?id=33119

Attachment 52855: proposed patch with unittests
https://bugs.webkit.org/attachment.cgi?id=52855&action=review

------- Additional Comments from Chris Jerdonek <cjerdonek at webkit.org>
Marking this patch r- because it no longer applies.

Also, some high-level comments regarding the directory being created here:

> +++ WebKitTools/Scripts/webkitperl/tools_unittest/svn-apply.pl       
(revision 0)

Might it make more sense to continue the existing pattern of one *_unittest/
directory per file to be unit-tested?  For example, svn-apply_unittest/ and
svn-unapply_unittest/?

If code can be shared between tests of code in svn-apply and svn-unapply,
maybe that can be handled by adding a test-support file in webkitperl/.

Finally, in cases where svn-apply and svn-unapply define the same or nearly
the same methods (because of cut-and-paste), it may also make sense to
refactor those areas to use the same code.  Then for each refactored
function, one function can be unit-testted instead of two.  That's not
something you necessarily need to be the one to do, but it's something
to consider.


More information about the webkit-reviews mailing list