Replace the easy cases of VERIFY usage.

Replace the easy cases of VERIFY usage.

BUG=webrtc:6424
Review-Url: https://codereview.webrtc.org/2652653012
Cr-Commit-Position: refs/heads/master@{#16370}
Committed: https://chromium.googlesource.com/external/webrtc/+/7ce109acd37ff1981e6b65eb96500700513743a5

[nisse-webrtc, 2017-01-26 12:06:03]

Description was changed from

==========
Replace the esy cases of VERIFY usage.

BUG=webrtc:6424
==========

to

==========
Replace the easy cases of VERIFY usage.

BUG=webrtc:6424
==========

[nisse-webrtc, 2017-01-26 12:08:43]

nisse@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[nisse-webrtc, 2017-01-26 12:08:44]

PTAL. No sed scripting for these, I think they all have to be replaced
case-by-case.

[kwiberg-webrtc, 2017-01-30 10:12:49]

lgtm, but see comment

https://codereview.webrtc.org/2652653012/diff/1/webrtc/pc/peerconnection.cc
File webrtc/pc/peerconnection.cc (right):

https://codereview.webrtc.org/2652653012/diff/1/webrtc/pc/peerconnection.cc#n...
webrtc/pc/peerconnection.cc:1017: RTC_DCHECK(observer != nullptr);
RTC_DCHECK_NE? Or will that not compile?

(You could also check just "observer", with implicit pointer->bool conversion,
but I'll leave that style choice up to you.)

Are you sure these will never trigger? Otherwise we'll get crashes here that we
didn't get before.

[nisse-webrtc, 2017-01-30 10:58:41]

https://codereview.webrtc.org/2652653012/diff/1/webrtc/pc/peerconnection.cc
File webrtc/pc/peerconnection.cc (right):

https://codereview.webrtc.org/2652653012/diff/1/webrtc/pc/peerconnection.cc#n...
webrtc/pc/peerconnection.cc:1017: RTC_DCHECK(observer != nullptr);
On 2017/01/30 10:12:49, kwiberg-webrtc wrote:
> RTC_DCHECK_NE? Or will that not compile?

It fails to compile, with errors related to << and MakeCheckOpString.

> (You could also check just "observer", with implicit pointer->bool conversion,
> but I'll leave that style choice up to you.)

I'll change to the more concise implicit bool conversion.

> Are you sure these will never trigger? Otherwise we'll get crashes here that
we
> didn't get before.

I'm not sure of anything... At least, there's nothing in the comments in
peerconnectioninterface.h to indicate that the observers are optional. Who
should I ask?

[nisse-webrtc, 2017-01-30 13:01:44]

nisse@webrtc.org changed reviewers:
+ deadbeef@webrtc.org

[nisse-webrtc, 2017-01-30 13:01:44]

Taylor, do you know if the peerconnection methods that take an observer argument
need to tolerate a NULL observer? 

Before this cl, NULL observer would crash in builds where  VERIFY was active,
but just log a message and return without doing anything in a release build.

[Taylor Brandstetter, 2017-01-30 15:48:02]

On 2017/01/30 13:01:44, nisse-webrtc wrote:
> Taylor, do you know if the peerconnection methods that take an observer
argument
> need to tolerate a NULL observer? 
> 
> Before this cl, NULL observer would crash in builds where  VERIFY was active,
> but just log a message and return without doing anything in a release build.

Since this is part of the surface level of the API, I think it should behave
well with bad arguments, so it should just log and error and return.

[nisse-webrtc, 2017-01-30 16:17:30]

On 2017/01/30 15:48:02, Taylor Brandstetter wrote:
> On 2017/01/30 13:01:44, nisse-webrtc wrote:
> > Taylor, do you know if the peerconnection methods that take an observer
> argument
> > need to tolerate a NULL observer? 
> > 
> > Before this cl, NULL observer would crash in builds where  VERIFY was
active,
> > but just log a message and return without doing anything in a release build.
> 
> Since this is part of the surface level of the API, I think it should behave
> well with bad arguments, so it should just log and error and return.

Thanks. I intend to make that change and land tomorrow, if that's ok with you?
(It's getting late in the afternoon here).

I.e, just change the original 

  if (!VERIFY(observer != nullptr))

to 

  if (!observer)

[Taylor Brandstetter, 2017-01-30 16:40:36]

On 2017/01/30 16:17:30, nisse-webrtc wrote:
> On 2017/01/30 15:48:02, Taylor Brandstetter wrote:
> > On 2017/01/30 13:01:44, nisse-webrtc wrote:
> > > Taylor, do you know if the peerconnection methods that take an observer
> > argument
> > > need to tolerate a NULL observer? 
> > > 
> > > Before this cl, NULL observer would crash in builds where  VERIFY was
> active,
> > > but just log a message and return without doing anything in a release
build.
> > 
> > Since this is part of the surface level of the API, I think it should behave
> > well with bad arguments, so it should just log and error and return.
> 
> Thanks. I intend to make that change and land tomorrow, if that's ok with you?
> (It's getting late in the afternoon here).
> 
> I.e, just change the original 
> 
>   if (!VERIFY(observer != nullptr))
> 
> to 
> 
>   if (!observer)

Yep, sounds good.

[nisse-webrtc, 2017-01-31 07:57:58]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 07:58:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[nisse-webrtc, 2017-01-31 07:58:32]

On 2017/01/30 16:40:36, Taylor Brandstetter wrote:
> On 2017/01/30 16:17:30, nisse-webrtc wrote:
> > I.e, just change the original 
> > 
> >   if (!VERIFY(observer != nullptr))
> > 
> > to 
> > 
> >   if (!observer)
> 
> Yep, sounds good.

Done now. Intending to land soon.

[commit-bot: I haz the power, 2017-01-31 08:34:39]

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

[commit-bot: I haz the power, 2017-01-31 08:34:40]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-01-31 08:55:35]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-01-31 08:55:35]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2652653012/#ps40001 (title:
"Let Peerconnection methods accept NULL observer.")

[commit-bot: I haz the power, 2017-01-31 08:55:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-31 08:57:57]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485852935328580,
"parent_rev": "96a9fa0291d0a0024ffb43c8277be389e076a607", "commit_rev":
"7ce109acd37ff1981e6b65eb96500700513743a5"}

[commit-bot: I haz the power, 2017-01-31 08:57:59]

Description was changed from

==========
Replace the easy cases of VERIFY usage.

BUG=webrtc:6424
==========

to

==========
Replace the easy cases of VERIFY usage.

BUG=webrtc:6424

Review-Url: https://codereview.webrtc.org/2652653012
Cr-Commit-Position: refs/heads/master@{#16370}
Committed:
https://chromium.googlesource.com/external/webrtc/+/7ce109acd37ff1981e6b65eb9...
==========

[commit-bot: I haz the power, 2017-01-31 08:58:00]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/7ce109acd37ff1981e6b65eb9...