Bug 47088

Summary: Reduce some code duplication
Product: WebKit Reporter: sadrul
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: gustavo, levin, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Revised patch to pass the tests abarth: review-

sadrul
Reported 2010-10-04 08:24:57 PDT
The attached patch reduces some code duplication to make the code a little more readable (and reduce the LOC by ~1500). This does not introduce any changes to the functionality, so no new layout tests is necessary.
Attachments
Patch (94.36 KB, patch)
2010-10-04 08:26 PDT, sadrul 		no flags
DetailsFormatted DiffDiff
Revised patch to pass the tests (94.37 KB, patch)
2010-10-04 09:12 PDT, sadrul 		abarth: review-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
sadrul
Comment 1 2010-10-04 08:26:02 PDT
Created attachment 69636 [details] Patch
WebKit Review Bot
Comment 2 2010-10-04 08:26:50 PDT
Attachment 69636 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/platform/LocalizedStrings.cpp:42: Tab found; better to use spaces [whitespace/tab] [1] WebCore/platform/gtk/LocalizedStringsGtk.cpp:57: Tab found; better to use spaces [whitespace/tab] [1] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 3 2010-10-04 08:52:15 PDT
Attachment 69636 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4241032
sadrul
Comment 4 2010-10-04 09:12:29 PDT
Created attachment 69641 [details] Revised patch to pass the tests Oops!
David Levin
Comment 5 2010-10-06 05:41:02 PDT
This patch is all above putting in macros in place of very simple and short C code. Putting in macro's so that one has another level of indirection in determining what the code does, doesn't seem like a move in a good direction to me. This makes it less readable to me.
Adam Barth
Comment 6 2010-10-10 12:38:26 PDT
Comment on attachment 69641 [details] Revised patch to pass the tests I agree with Dave Levin.
Note You need to log in before you can comment on or make changes to this bug.