Bug 5877

Summary: Remove remaining APPLE_CHANGES and KHTML_NO_CPP_DOM blocks from WebCore
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: New BugsAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P4    
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Bug Depends on:    
Bug Blocks: 5929, 5931    
Attachments:
Description Flags
The patch
none
cleanup.pl script
none
A hacked version of the partial preprocessor (partpp.pl)
none
Responding to Darin's comments.
none
A newer hacked version of partpp.pl which has whitespace removal heuristics
none
Diff from the original (http://www.phost.de/~stefan/Files/partpp.pl) partpp.pl (for the curious)
none
SpacingHeuristics.pm perl module
none
completely script generated patch to remove APPLE_CHANGES and KHTML_NO_CPP_DOM (and whitespace)
mjs: review+
newer cleanup.pl (trivial partpp.pl wrapper) none

Description Eric Seidel (no email) 2005-11-29 04:43:10 PST
Remove remaining APPLE_CHANGES and KHTML_NO_CPP_DOM blocks from WebCore

The previous bug:
http://bugzilla.opendarwin.org/show_bug.cgi?id=5711
only removed APPLE_CHANGES from WebCore/khtml

This removes all the rest from WebCore (kwq mostly), with the exception of SVG, and additionally removes 
all KHTML_NO_CPP_DOM blocks.
Comment 1 Eric Seidel (no email) 2005-11-29 04:46:15 PST
Created attachment 4847 [details]
The patch
Comment 2 Eric Seidel (no email) 2005-11-29 04:46:54 PST
Created attachment 4848 [details]
cleanup.pl script
Comment 3 Eric Seidel (no email) 2005-11-29 04:47:31 PST
Created attachment 4849 [details]
A hacked version of the partial preprocessor (partpp.pl)
Comment 4 Eric Seidel (no email) 2005-11-29 04:54:54 PST
Comment on attachment 4847 [details]
The patch

This also removes all #if 0 blocks (which may or may not be a good thing).
Comment 5 Darin Adler 2005-11-29 07:36:20 PST
Comment on attachment 4847 [details]
The patch

This patch shows lots of cases where it leaves stray multiple blank lines. So
it's going to leave formatting a little messy. I'd like to see a version that
has a heuristic for nicely eating the excess blank lines.

I also see cases where there's a comment just before an #if 0 section. This
patch is going to lave those comments stranded. An example is in DOMHTML.mm.

Some of this already probably took place with the last "remove ifdefs" patch,
which I did not review in detail.
Comment 6 Eric Seidel (no email) 2005-11-29 14:59:21 PST
Created attachment 4855 [details]
Responding to Darin's comments.
Comment 7 Eric Seidel (no email) 2005-11-29 14:59:30 PST
Comment on attachment 4855 [details]
Responding to Darin's comments.

To address Darin's concerns, I did a couple things:

1.  I did a search throughout the entire project looking for "excessive"
newlines and removed them.
2.  I did a manual review of each of the previously changed files and removed
any extra comments, newlines.
3.  I removed dom_misc.* entirely, and moved the remaining DOM::EventListener
and DOM::NodeFilterCondition onto khtml::Shared<T>
4.  As part of my manual walk through khtml_part.cpp I removed
DIRECT_LINKAGE_TO_ECMA as well.
Comment 8 Eric Seidel (no email) 2005-11-29 15:00:04 PST
Comment on attachment 4847 [details]
The patch

Retracting review on this one for now.	See the new larger patch instead.
Comment 9 Darin Adler 2005-12-03 11:10:29 PST
Comment on attachment 4855 [details]
Responding to Darin's comments.

Here's an example of the kind of blank line thing I'm not so happy about:

---------

 #include "khtmlpart_p.h"

-#if !KHTML_NO_CPLUSPLUS_DOM
-#include "dom/html_document.h"
-#endif

 #include <CoreServices/CoreServices.h>

---------

and here's another:

---------

   cancelRedirection();

-#if !KHTML_NO_CPLUSPLUS_DOM
-  // null node activated.
-  emit nodeActivated(Node());
-#endif
 }

---------

Otherwise, this patch looks fine.
Comment 10 Eric Seidel (no email) 2005-12-04 02:26:11 PST
Comment on attachment 4855 [details]
Responding to Darin's comments.

Retracting review, posting a simpler patch instead.
Comment 11 Eric Seidel (no email) 2005-12-04 02:27:22 PST
Created attachment 4926 [details]
A newer hacked version of partpp.pl which has whitespace removal heuristics
Comment 12 Eric Seidel (no email) 2005-12-04 02:28:00 PST
Created attachment 4927 [details]
Diff from the original (http://www.phost.de/~stefan/Files/partpp.pl) partpp.pl (for the curious)
Comment 13 Eric Seidel (no email) 2005-12-04 02:28:48 PST
Created attachment 4928 [details]
SpacingHeuristics.pm perl module
Comment 14 Eric Seidel (no email) 2005-12-04 02:29:36 PST
Created attachment 4929 [details]
completely script generated patch to remove APPLE_CHANGES and KHTML_NO_CPP_DOM (and whitespace)
Comment 15 Eric Seidel (no email) 2005-12-04 02:32:23 PST
Comment on attachment 4929 [details]
completely script generated patch to remove APPLE_CHANGES and KHTML_NO_CPP_DOM (and whitespace)

This is a simpler patch which is entirely script generated and addresses
darin's whitespace concerns.  Script is also attached to the bug.  This should
be a trivial review which any reviewer should feel comfortable with.
Comment 16 Eric Seidel (no email) 2005-12-04 02:33:29 PST
Created attachment 4930 [details]
newer cleanup.pl (trivial partpp.pl wrapper)
Comment 17 Maciej Stachowiak 2005-12-04 03:21:30 PST
Comment on attachment 4929 [details]
completely script generated patch to remove APPLE_CHANGES and KHTML_NO_CPP_DOM (and whitespace)

rs=me