Last Comment Bug 680864 - Remove CHROMIUM_MOZILLA_BUILD ifdefs, since always defined and #ifndef codepaths broken anyway
: Remove CHROMIUM_MOZILLA_BUILD ifdefs, since always defined and #ifndef codepa...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Ed Morley [:emorley]
:
: Gregory Szorc [:gps]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-22 04:14 PDT by Ed Morley [:emorley]
Modified: 2011-08-23 08:56 PDT (History)
6 users (show)
emorley: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove CHROMIUM_MOZILLA_BUILD (183.34 KB, patch)
2011-08-22 15:08 PDT, Ed Morley [:emorley]
cjones.bugs: review+
Details | Diff | Splinter Review
Additional cleanup (10.83 KB, patch)
2011-08-22 15:19 PDT, Ed Morley [:emorley]
cjones.bugs: review+
Details | Diff | Splinter Review

Description Ed Morley [:emorley] 2011-08-22 04:14:52 PDT
(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] 2011-08-22 05:04:32 PDT
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 [:bsmedberg] 2011-08-22 06:16:36 PDT
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 2011-08-22 10:08:33 PDT
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] 2011-08-22 15:08:29 PDT
Created attachment 554977 [details] [diff] [review]
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] 2011-08-22 15:19:42 PDT
Created attachment 554979 [details] [diff] [review]
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

Note You need to log in before you can comment on or make changes to this bug.