Closed Bug 680864 Opened 9 years ago Closed 9 years ago
_MOZILLA _BUILD ifdefs, since always defined and #ifndef codepaths broken anyway
(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 :)
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.
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.
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.
Removes the CHROMIUM_MOZILLA_BUILD ifdefs from the tree. http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=6f5233f2fe68
Attachment #554977 - Flags: review?(jones.chris.g)
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
Attachment #554979 - Flags: review?(jones.chris.g)
Attachment #554977 - Flags: review?(jones.chris.g) → review+
Attachment #554979 - Flags: review?(jones.chris.g) → review+
Target Milestone: --- → mozilla9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.