680864 - Remove CHROMIUM_MOZILLA_BUILD ifdefs, since always defined and #ifndef codepaths broken anyway

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Ed Morley [:emorley]]

(2011-08-22 12:03:46) ejpbruel: Ms2ger: in child_process_host.cc, there is some code that is not enclosed in CHROMIUM_MOZILLA_BUILD, but can only work if that macro is enabled, because it relies on a function that is inherited from a class that is only included if that macro is there
(2011-08-22 12:04:10) ejpbruel: Ms2ger: is it possible that the use of this macro is currently unchecked, and therefore even broken in places?
(2011-08-22 12:04:12) Ms2ger: ejpbruel, yeah, it's been fiction for a while already
(2011-08-22 12:04:39) ejpbruel: Ms2ger: we might wanna get rid of it then, its only purpose now is to confuse me when studying code
(2011-08-22 12:05:08) Ms2ger: Go for it :)

Comment 1

[Mike Hommey [:glandium]]

Note that it's always defined *on Mozilla builds*. The code comes from chromium, and there, it's not defined. I don't know if the original chromium code now contains some of these CHROMIUM_MOZILLA_BUILD ifdefs, nor if we care about keeping them. Benjamin would know, I guess.

Comment 2

[Benjamin Smedberg]

The ifdefs were added to indicate the places that we modified the chromium code. Since we don't expect to stay synced with that code, we can probably remove them, but it's cjones' call.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We haven't upstreamed any of our local modifications.  There are a small number that might be generally applicable (one helper and one new file come to mind), but most really are Mozilla specific.  FWIW, we tried to upstream a very general performance fix for STLport and it was not taken.  The chromium IPC impl for linux diverged from us in an incompatible way since we pulled, and we don't stand to gain anything but work by updating (yet).  The long-term plan is to extract the code from the 4 or 5 nontrivial files we pulled and integrate it more tightly into Gecko, so we can do away with the needless duplication of the 20 or more files of "framework" code that we pulled, for which analogues mostly already exist in Gecko.  That isn't a high priority for anyone though.

I personally don't care much about ifdef CHROMIUM_MOZILLA_BUILD, but if it's confusing people who are trying to understand the code, then I'm all for nuking it.

Comment 4

[Ed Morley [:emorley]]

Attachment: Remove CHROMIUM_MOZILLA_BUILD

Removes the CHROMIUM_MOZILLA_BUILD ifdefs from the tree.

http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=6f5233f2fe68

Comment 5

[Ed Morley [:emorley]]

Attachment: Additional cleanup

I've kept this additional cleanup separate to make it easier to review, since it's everything that wasn't just mechanical CHROMIUM_MOZILLA_BUILD ifdef/ifndef removal.

IPC_MESSAGE_ENABLE_RPC is now always defined, so can be removed:
http://mxr.mozilla.org/mozilla-central/search?string=IPC_MESSAGE_ENABLE_RPC

The BASE_API prefix defines is now always empty, so can be removed:
http://mxr.mozilla.org/mozilla-central/search?string=BASE_API

MessageLoop::EnableHistogrammer(), MessageLoop::StartHistogrammer() and MessageLoop::HistogramEvent() no longer do anything, so have been taken out. 

This leaves the VALUE_TO_NUMBER_AND_NAME define unused, so has been taken out:
http://mxr.mozilla.org/mozilla-central/search?string=VALUE_TO_NUMBER_AND_NAME

Comment 6

[Ed Morley [:emorley]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0f3983da53fb
http://hg.mozilla.org/integration/mozilla-inbound/rev/08a3ae5c564a

Comment 7

[Mounir Lamouri (:mounir)]

http://hg.mozilla.org/mozilla-central/rev/0f3983da53fb
http://hg.mozilla.org/mozilla-central/rev/08a3ae5c564a