Add RTC_ prefix to (D)CHECKs and related macros.

Add RTC_ prefix to (D)CHECKs and related macros.

We must remove dependency on Chromium, i.e. we can't use Chromium's base/logging.h. That means we need to define these macros in WebRTC also when doing Chromium builds. And this causes redefinition.

Alternative solutions:
* Check if we already have defined e.g. CHECK, and don't define them in that case. This makes us depend on include order in Chromium, which is not acceptable.
* Don't allow using the macros in WebRTC headers. Error prone since if someone adds it there by mistake it may compile fine, but later break if a header in added or order is changed in Chromium. That will be confusing and hard to enforce.
* Ensure that headers that are included by an embedder don't include our macros. This would require some heavy refactoring to be maintainable and enforcable.
* Changes in Chromium for this is obviously not an option.

BUG=chromium:468375
NOTRY=true

Committed: https://crrev.com/91d6edef35e7275879c30ce16ecb8b6dc73c6e4a
Cr-Commit-Position: refs/heads/master@{#9964}

[Henrik Grunell WebRTC, 2015-09-11 10:29:36]

henrikg@webrtc.org changed reviewers:
+ andrew@webrtc.org, mflodman@webrtc.org, tommi@webrtc.org

[Henrik Grunell WebRTC, 2015-09-11 10:31:48]

Before I go ahead and fix 80 cols etc. I wanted to get your opinion on this
change. It's to avoid redefinitions, I think this is a better option than using
various flags to maybe define these.

[kwiberg-webrtc, 2015-09-11 13:38:52]

On 2015/09/11 10:31:48, Henrik Grunell (webrtc) wrote:
> Before I go ahead and fix 80 cols etc. I wanted to get your opinion on this
> change. It's to avoid redefinitions, I think this is a better option than
using
> various flags to maybe define these.

The idea sounds appealing enough that paying the extra 4 characters at each call
site might be worth it.

Were you able to make any simplifications now that the macro names no longer
collide, or is this a pure rename?

[Henrik Grunell WebRTC, 2015-09-11 13:43:57]

On 2015/09/11 13:38:52, kwiberg-webrtc wrote:
> On 2015/09/11 10:31:48, Henrik Grunell (webrtc) wrote:
> > Before I go ahead and fix 80 cols etc. I wanted to get your opinion on this
> > change. It's to avoid redefinitions, I think this is a better option than
> using
> > various flags to maybe define these.
> 
> The idea sounds appealing enough that paying the extra 4 characters at each
call
> site might be worth it.
> 
> Were you able to make any simplifications now that the macro names no longer
> collide, or is this a pure rename?

I can at least remove the WEBRTC_CHROMIUM_BUILD define check in checks.h/cc. :)
I don't think much more can be done, bu haven't look into it.

[Andrew MacDonald, 2015-09-11 15:40:15]

On 2015/09/11 10:31:48, Henrik Grunell (webrtc) wrote:
> Before I go ahead and fix 80 cols etc. I wanted to get your opinion on this
> change. It's to avoid redefinitions, I think this is a better option than
using
> various flags to maybe define these.

I thought the plan was to move the overrides to Chromium?

How does e.g. v8 and nacl get away with it?

https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/base/logging.h
https://code.google.com/p/chromium/codesearch#chromium/src/native_client/src/...

[Andrew MacDonald, 2015-09-11 15:50:27]

One concern: will we lose anything wrt reporting webrtc CHECK crashes in Chrome
production?

[Henrik Grunell WebRTC, 2015-09-14 08:36:02]

On 2015/09/11 15:40:15, Andrew MacDonald wrote:
> On 2015/09/11 10:31:48, Henrik Grunell (webrtc) wrote:
> > Before I go ahead and fix 80 cols etc. I wanted to get your opinion on this
> > change. It's to avoid redefinitions, I think this is a better option than
> using
> > various flags to maybe define these.
> 
> I thought the plan was to move the overrides to Chromium?

Yes, that too but it's not enough since we can't include indirectly anything
from Chromium into WebRTC. Thus only implementation files moved to Chromium can
include from Chromium.

> 
> How does e.g. v8 and nacl get away with it?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/base/logging.h
>
https://code.google.com/p/chromium/codesearch#chromium/src/native_client/src/...

For v8, Chromium only includes from the v8/includes directory, and files there
don't include other v8 files.

For nacl, it's a limited set of header files that use the macros (4-5 or so) and
it seems to be just luck that there isn't a clash. If I add

#include "native_client/src/trusted/sel_universal/workqueue.h"

in a Chromium file, redefinition error.

[Henrik Grunell WebRTC, 2015-09-14 08:45:31]

On 2015/09/11 15:50:27, Andrew MacDonald wrote:
> One concern: will we lose anything wrt reporting webrtc CHECK crashes in
Chrome
> production?

No not that I can see. CHECK in Chromium doesn't do anything in particular wrt
reporting crashes.

[tommi, 2015-09-14 12:46:02]

On 2015/09/14 08:45:31, Henrik Grunell (webrtc) wrote:
> On 2015/09/11 15:50:27, Andrew MacDonald wrote:
> > One concern: will we lose anything wrt reporting webrtc CHECK crashes in
> Chrome
> > production?
> 
> No not that I can see. CHECK in Chromium doesn't do anything in particular wrt
> reporting crashes.

I'm still on the fence about this being good or bad.  It's good of course to get
rid of the dependency but bad since we're changing the name of something in our
repo to not clash with another and using CHECK would keep webrtc more aligned
with Chromium's.

To get a little bit more feedback, what about doing something like this:

* Add RTC_CHECK() as a define for CHECK so that people can start using it right
away.
* Send a PSA to webrtc eng about the change and see what people think - or if
there are other ideas.

[kwiberg-webrtc, 2015-09-14 13:04:27]

On 2015/09/14 12:46:02, tommi (webrtc) wrote:
> On 2015/09/14 08:45:31, Henrik Grunell (webrtc) wrote:
> > On 2015/09/11 15:50:27, Andrew MacDonald wrote:
> > > One concern: will we lose anything wrt reporting webrtc CHECK crashes in
> > Chrome
> > > production?
> > 
> > No not that I can see. CHECK in Chromium doesn't do anything in particular
wrt
> > reporting crashes.
> 
> I'm still on the fence about this being good or bad.  It's good of course to
get
> rid of the dependency but bad since we're changing the name of something in
our
> repo to not clash with another and using CHECK would keep webrtc more aligned
> with Chromium's.
> 
> To get a little bit more feedback, what about doing something like this:
> 
> * Add RTC_CHECK() as a define for CHECK so that people can start using it
right
> away.
> * Send a PSA to webrtc eng about the change and see what people think - or if
> there are other ideas.

Using RTC_CHECK instead of CHECK is a bit ugly, sure, but it's the only really
clean and simple solution I can think of that doesn't involve changing Chromium.

The clean and simple *and pretty* solution would be to break out CHECK et al.
from Chromium into a separate little project that others were free to use as
well. That might be a bunch of work, though, and I have no idea if the Chromium
people would want that---to them, it'd mean extra barriers to changing the
broken-out code, in exchange for making the life of other projects easier.

[Henrik Grunell WebRTC, 2015-09-14 14:37:09]

On 2015/09/14 13:04:27, kwiberg-webrtc wrote:
> On 2015/09/14 12:46:02, tommi (webrtc) wrote:
> > On 2015/09/14 08:45:31, Henrik Grunell (webrtc) wrote:
> > > On 2015/09/11 15:50:27, Andrew MacDonald wrote:
> > > > One concern: will we lose anything wrt reporting webrtc CHECK crashes in
> > > Chrome
> > > > production?
> > > 
> > > No not that I can see. CHECK in Chromium doesn't do anything in particular
> wrt
> > > reporting crashes.
> > 
> > I'm still on the fence about this being good or bad.  It's good of course to
> get
> > rid of the dependency but bad since we're changing the name of something in
> our
> > repo to not clash with another and using CHECK would keep webrtc more
aligned
> > with Chromium's.
> > 
> > To get a little bit more feedback, what about doing something like this:
> > 
> > * Add RTC_CHECK() as a define for CHECK so that people can start using it
> right
> > away.
> > * Send a PSA to webrtc eng about the change and see what people think - or
if
> > there are other ideas.
> 
> Using RTC_CHECK instead of CHECK is a bit ugly, sure, but it's the only really
> clean and simple solution I can think of that doesn't involve changing
Chromium.
> 
> The clean and simple *and pretty* solution would be to break out CHECK et al.
> from Chromium into a separate little project that others were free to use as
> well. That might be a bunch of work, though, and I have no idea if the
Chromium
> people would want that---to them, it'd mean extra barriers to changing the
> broken-out code, in exchange for making the life of other projects easier.

I agree, I don't like this but I have a hard time finding a better solution. We
include loads of WebRTC headers in Chromium and it will be a ton of work to, for
example, ensure that no header that's included by an embedder includes WebRTC's
"internal" check or logging. And to enforce that. I'm more than happy to hear
ideas on how to solve this, so check on the list is probably good.

[Andrew MacDonald, 2015-09-14 16:26:44]

On 2015/09/14 08:45:31, Henrik Grunell (webrtc) wrote:
> On 2015/09/11 15:50:27, Andrew MacDonald wrote:
> > One concern: will we lose anything wrt reporting webrtc CHECK crashes in
> Chrome
> > production?
> 
> No not that I can see. CHECK in Chromium doesn't do anything in particular wrt
> reporting crashes.

Not sure this matters, but the webrtc version just prints the stack trace to
stderr:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

while Chromium will log to its logfile. I suppose the stack traces are uploaded
in some other way though?

[Henrik Grunell WebRTC, 2015-09-14 19:33:51]

On 2015/09/14 16:26:44, Andrew MacDonald wrote:
> On 2015/09/14 08:45:31, Henrik Grunell (webrtc) wrote:
> > On 2015/09/11 15:50:27, Andrew MacDonald wrote:
> > > One concern: will we lose anything wrt reporting webrtc CHECK crashes in
> > Chrome
> > > production?
> > 
> > No not that I can see. CHECK in Chromium doesn't do anything in particular
wrt
> > reporting crashes.
> 
> Not sure this matters, but the webrtc version just prints the stack trace to
> stderr:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> 
> while Chromium will log to its logfile. I suppose the stack traces are
uploaded
> in some other way though?

Yes, it's uploaded by other means. And afaik this doesn't affect that in any
way. Also, we don't really have a choice.

[Andrew MacDonald, 2015-09-15 01:51:04]

lgtm assuming no further concerns are brought up on your email thread.

I only looked at checks.h and checks.cc.

[tommi, 2015-09-15 13:29:05]

Thanks for doing the PSA round.  lgtm (and hope things go well with the >80
fix!)

[Henrik Grunell WebRTC, 2015-09-16 15:52:36]

The CQ bit was checked by henrikg@webrtc.org

[Henrik Grunell WebRTC, 2015-09-16 15:52:37]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
andrew@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1335923002/#ps100001 (title:
"Rebase.")

[commit-bot: I haz the power, 2015-09-16 15:52:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1335923002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1335923002/100001

[commit-bot: I haz the power, 2015-09-16 15:57:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-16 15:57:46]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/782)

[Henrik Grunell WebRTC, 2015-09-16 16:11:49]

The CQ bit was checked by henrikg@webrtc.org

[Henrik Grunell WebRTC, 2015-09-16 16:11:50]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
andrew@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1335923002/#ps120001 (title:
"Fixes.")

[commit-bot: I haz the power, 2015-09-16 16:11:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1335923002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1335923002/120001

[commit-bot: I haz the power, 2015-09-16 18:12:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-16 18:12:21]

Try jobs failed on following builders:
  android on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_drmemory_light on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[Henrik Grunell WebRTC, 2015-09-17 07:07:23]

The CQ bit was checked by henrikg@webrtc.org

[commit-bot: I haz the power, 2015-09-17 07:07:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1335923002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1335923002/120001

[commit-bot: I haz the power, 2015-09-17 07:10:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-17 07:10:21]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/798)

[Henrik Grunell WebRTC, 2015-09-17 07:22:09]

The CQ bit was checked by henrikg@webrtc.org

[Henrik Grunell WebRTC, 2015-09-17 07:22:10]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
andrew@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1335923002/#ps140001 (title:
"Rebase.")

[commit-bot: I haz the power, 2015-09-17 07:22:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1335923002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1335923002/140001

[commit-bot: I haz the power, 2015-09-17 07:24:42]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-09-17 07:25:01]

Patchset 8 (id:??) landed as
https://crrev.com/91d6edef35e7275879c30ce16ecb8b6dc73c6e4a
Cr-Commit-Position: refs/heads/master@{#9964}