Bug 13699 – Consolidate notImplemented() macro definitions into header

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kevin Ollivier]

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...!)

[Kevin Ollivier]

Created attachment 14521 [details]
consolidate notImplemented() macros

[Sam Weinig]

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]

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]

Created attachment 14537 [details]
Updated patch with suggestions implemented

[Kevin Ollivier]

Created attachment 14538 [details]
Updated patch, removing ChromeClientWin change

[Kevin Ollivier]

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]

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 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]

Landed in r21827.