RESOLVED FIXED 13699
Consolidate notImplemented() macro definitions into header 
https://bugs.webkit.org/show_bug.cgi?id=13699
Summary Consolidate notImplemented() macro definitions into header
Kevin Ollivier
Reported 2007-05-12 15:05:02 PDT
This patch takes all the different definitions of notImplemented() and notImplementedGdk() and consolidates them into one macro and one header file. This resolves problems such as building the CURL loader when not building the GDK port, and also removes a lot of redundant code. (Which is why the patch is so large...!)
Attachments
consolidate notImplemented() macros (96.85 KB, patch)
2007-05-12 15:06 PDT, Kevin Ollivier 		no flags
DetailsFormatted DiffDiff
Updated patch with suggestions implemented (96.87 KB, patch)
2007-05-13 13:01 PDT, Kevin Ollivier 		no flags
DetailsFormatted DiffDiff
Updated patch, removing ChromeClientWin change (95.95 KB, patch)
2007-05-13 13:21 PDT, Kevin Ollivier 		sam: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Kevin Ollivier
Comment 1 2007-05-12 15:06:29 PDT
Created attachment 14521 [details] consolidate notImplemented() macros
Sam Weinig
Comment 2 2007-05-12 19:33:12 PDT
Comment on attachment 14521 [details] consolidate notImplemented() macros This seems like a good idea. A couple points you may want to address: In NotImplemented.h - for the non-debug version of notImplemented() you may want to use (void(0)) as we do for ASSERT instead of {}. - the Qt and else versions should use WTF_PRETTY_FUNCTION instead of __FUNCTION__. In WebKit/COM/ChromeClientWin.cpp - you don't #include NotImplemented.h, you have just redefined the empty macro.
Sam Weinig
Comment 3 2007-05-12 19:40:26 PDT
Another thing you might want to consider, though I am unsure it whether the idea will be popular, is putting the notImplemented() macros in wtf/Assertions.h which would eliminate the need to #include the NotImplemented.h header everywhere as Assertions.h should be included everywhere automatically due to config.h.
Kevin Ollivier
Comment 4 2007-05-13 13:01:47 PDT
Created attachment 14537 [details] Updated patch with suggestions implemented
Kevin Ollivier
Comment 5 2007-05-13 13:21:09 PDT
Created attachment 14538 [details] Updated patch, removing ChromeClientWin change
Kevin Ollivier
Comment 6 2007-05-13 13:22:08 PDT
Okay, I just uploaded a new version with changes made. (Sorry I had to upload twice, I forgot to remove the ChromeClientWin typo the first time.) Regarding where to put the macro, I did already ask on webkit-dev about putting the function into Assertions.h, and was asked to put it into a separate header, so I left that part of the patch as-is.
Alp Toker
Comment 7 2007-05-27 14:00:30 PDT
Is there any word on this getting reviewed/landed? It's a small change that can really help to ease the building of various configurations of the ports. Maintaining the patch in the bug tracker while it waits for review is a bit pointless since it's a large patch which will rapidly diverge from TOT, so it would be best to just go ahead and get this in.
Sam Weinig
Comment 8 2007-05-27 16:23:28 PDT
Comment on attachment 14538 [details] Updated patch, removing ChromeClientWin change r=me assuming this still applies cleanly and you remove "WARNING: NO TEST CASES ADDED OR CHANGED" from the ChangeLog.
Sam Weinig
Comment 9 2007-05-27 18:38:57 PDT
Landed in r21827.
Note You need to log in before you can comment on or make changes to this bug.