Take ownership and close the platform file even if we fail to start logging.

Take ownership and close the platform file even if we fail to start logging.

Committed: https://crrev.com/43587e32e9aab498a6813074c6858e4672355106
Cr-Commit-Position: refs/heads/master@{#12944}

[terelius, 2016-05-25 16:15:57]

terelius@webrtc.org changed reviewers:
+ ivoc@webrtc.org, pbos@webrtc.org

[terelius, 2016-05-25 16:15:58]

Please take a look.

[ivoc, 2016-05-26 08:56:59]

Nice fix, see some small nits below.

https://codereview.webrtc.org/2015543002/diff/1/webrtc/call/rtc_event_log.cc
File webrtc/call/rtc_event_log.cc (right):

https://codereview.webrtc.org/2015543002/diff/1/webrtc/call/rtc_event_log.cc#...
webrtc/call/rtc_event_log.cc:197: LOG(LS_WARNING) << "Can't open file. Logging
not started.";
"Logging" sounds a bit vague to me, how about something like: RTC event log not
started.

https://codereview.webrtc.org/2015543002/diff/1/webrtc/call/rtc_event_log.cc#...
webrtc/call/rtc_event_log.cc:220: LOG(LS_WARNING) << "Can't open file. Logging
not started.";
Same here.

https://codereview.webrtc.org/2015543002/diff/1/webrtc/call/rtc_event_log.cc#...
webrtc/call/rtc_event_log.cc:229: LOG(LS_WARNING) << "Can't open file. Logging
not started.";
Same here.

https://codereview.webrtc.org/2015543002/diff/1/webrtc/call/rtc_event_log.cc#...
webrtc/call/rtc_event_log.cc:235: }
This is kind of unrelated to this CL, but I think it would be nice to have a
LOG(LS_INFO) message here to confirm that the logging has started successfully,
including the file name. WDYT?

[pbos-webrtc, 2016-05-26 12:05:18]

https://codereview.webrtc.org/2015543002/diff/1/webrtc/call/rtc_event_log.cc
File webrtc/call/rtc_event_log.cc (right):

https://codereview.webrtc.org/2015543002/diff/1/webrtc/call/rtc_event_log.cc#...
webrtc/call/rtc_event_log.cc:197: LOG(LS_WARNING) << "Can't open file. Logging
not started.";
All of these are errors and should log on LS_ERROR.

https://codereview.webrtc.org/2015543002/diff/1/webrtc/call/rtc_event_log.cc#...
webrtc/call/rtc_event_log.cc:235: }
On 2016/05/26 08:56:58, ivoc wrote:
> This is kind of unrelated to this CL, but I think it would be nice to have a
> LOG(LS_INFO) message here to confirm that the logging has started
successfully,
> including the file name. WDYT?

Agreed, that'd be useful.

[terelius, 2016-05-26 14:54:42]

https://codereview.webrtc.org/2015543002/diff/1/webrtc/call/rtc_event_log.cc
File webrtc/call/rtc_event_log.cc (right):

https://codereview.webrtc.org/2015543002/diff/1/webrtc/call/rtc_event_log.cc#...
webrtc/call/rtc_event_log.cc:197: LOG(LS_WARNING) << "Can't open file. Logging
not started.";
On 2016/05/26 08:56:58, ivoc wrote:
> "Logging" sounds a bit vague to me, how about something like: RTC event log
not
> started.

Done.

https://codereview.webrtc.org/2015543002/diff/1/webrtc/call/rtc_event_log.cc#...
webrtc/call/rtc_event_log.cc:197: LOG(LS_WARNING) << "Can't open file. Logging
not started.";
On 2016/05/26 12:05:18, pbos-webrtc wrote:
> All of these are errors and should log on LS_ERROR.

Done.

https://codereview.webrtc.org/2015543002/diff/1/webrtc/call/rtc_event_log.cc#...
webrtc/call/rtc_event_log.cc:220: LOG(LS_WARNING) << "Can't open file. Logging
not started.";
On 2016/05/26 08:56:59, ivoc wrote:
> Same here.

Done.

https://codereview.webrtc.org/2015543002/diff/1/webrtc/call/rtc_event_log.cc#...
webrtc/call/rtc_event_log.cc:229: LOG(LS_WARNING) << "Can't open file. Logging
not started.";
On 2016/05/26 08:56:59, ivoc wrote:
> Same here.

Done.

https://codereview.webrtc.org/2015543002/diff/1/webrtc/call/rtc_event_log.cc#...
webrtc/call/rtc_event_log.cc:235: }
On 2016/05/26 12:05:18, pbos-webrtc wrote:
> On 2016/05/26 08:56:58, ivoc wrote:
> > This is kind of unrelated to this CL, but I think it would be nice to have a
> > LOG(LS_INFO) message here to confirm that the logging has started
> successfully,
> > including the file name. WDYT?
> 
> Agreed, that'd be useful.

Logging the filename would be very useful, but we don't actually know it here.
We could print a message "WebRTC event logging has started", but apart from
debugging the event log itself, I don't see the utility of this message.

[pbos-webrtc, 2016-05-26 21:22:40]

lgtm

https://codereview.webrtc.org/2015543002/diff/40001/webrtc/call/rtc_event_log.cc
File webrtc/call/rtc_event_log.cc (right):

https://codereview.webrtc.org/2015543002/diff/40001/webrtc/call/rtc_event_log...
webrtc/call/rtc_event_log.cc:233: if (!message_queue_.Insert(&message)) {
Side note, should this ever happen? That seems scary.

[ivoc, 2016-05-27 07:46:32]

LGTM

[terelius, 2016-05-27 09:11:30]

The CQ bit was checked by terelius@webrtc.org

[commit-bot: I haz the power, 2016-05-27 09:11:36]

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

[commit-bot: I haz the power, 2016-05-27 09:22:55]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-27 09:22:58]

Description was changed from

==========
Take ownership and close the platform file even if we fail to start logging.
==========

to

==========
Take ownership and close the platform file even if we fail to start logging.

Committed: https://crrev.com/43587e32e9aab498a6813074c6858e4672355106
Cr-Commit-Position: refs/heads/master@{#12944}
==========

[commit-bot: I haz the power, 2016-05-27 09:22:59]

Patchset 3 (id:??) landed as
https://crrev.com/43587e32e9aab498a6813074c6858e4672355106
Cr-Commit-Position: refs/heads/master@{#12944}