Bug 13699 - Consolidate notImplemented() macro definitions into header
Summary: Consolidate notImplemented() macro definitions into header
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-05-12 15:05 PDT by Kevin Ollivier
Modified: 2007-05-27 18:38 PDT (History)
1 user (show)

See Also:


Attachments
consolidate notImplemented() macros (96.85 KB, patch)
2007-05-12 15:06 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
Updated patch with suggestions implemented (96.87 KB, patch)
2007-05-13 13:01 PDT, Kevin Ollivier
no flags Details | Formatted Diff | Diff
Updated patch, removing ChromeClientWin change (95.95 KB, patch)
2007-05-13 13:21 PDT, Kevin Ollivier
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Ollivier 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...!)
Comment 1 Kevin Ollivier 2007-05-12 15:06:29 PDT
Created attachment 14521 [details]
consolidate notImplemented() macros
Comment 2 Sam Weinig 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.
Comment 3 Sam Weinig 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.
Comment 4 Kevin Ollivier 2007-05-13 13:01:47 PDT
Created attachment 14537 [details]
Updated patch with suggestions implemented
Comment 5 Kevin Ollivier 2007-05-13 13:21:09 PDT
Created attachment 14538 [details]
Updated patch, removing ChromeClientWin change
Comment 6 Kevin Ollivier 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. 
Comment 7 Alp Toker 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.
Comment 8 Sam Weinig 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.
Comment 9 Sam Weinig 2007-05-27 18:38:57 PDT
Landed in r21827.