Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(282)

Issue 218403004: Fix the timestamp generation for webrtc native log. (Closed)

Created:
6 years, 8 months ago by jiayl
Modified:
6 years, 8 months ago
CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, Henrik Grunell
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Fix the timestamp generation for webrtc native log. The timestamp generation is moved from libjingle/renderer to the browser. The renderer sends the absolute system time for each message in the IPC, which is used by WebRtcLoggingHandlerHost to calculate the timestamp relative to the logging started time. BUG=339478 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261472

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 8

Patch Set 4 : #

Total comments: 7

Patch Set 5 : fix tommi's comments #

Total comments: 3

Patch Set 6 : base::Time #

Patch Set 7 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+125 lines, -82 lines) Patch
M chrome/browser/media/webrtc_logging_handler_host.h View 1 2 3 4 3 chunks +7 lines, -2 lines 0 comments Download
M chrome/browser/media/webrtc_logging_handler_host.cc View 1 2 3 4 6 chunks +16 lines, -7 lines 0 comments Download
M chrome/chrome_common.gypi View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
A chrome/common/media/webrtc_logging_message_data.h View 1 2 3 4 5 1 chunk +26 lines, -0 lines 0 comments Download
A chrome/common/media/webrtc_logging_message_data.cc View 1 2 3 4 5 6 1 chunk +25 lines, -0 lines 0 comments Download
M chrome/common/media/webrtc_logging_messages.h View 1 2 3 4 1 chunk +8 lines, -4 lines 0 comments Download
M chrome/renderer/media/chrome_webrtc_log_message_delegate.h View 1 2 3 4 2 chunks +4 lines, -2 lines 0 comments Download
M chrome/renderer/media/chrome_webrtc_log_message_delegate.cc View 1 2 3 4 2 chunks +20 lines, -16 lines 0 comments Download
M chrome/renderer/media/mock_webrtc_logging_message_filter.h View 1 2 3 4 5 6 1 chunk +2 lines, -1 line 0 comments Download
M chrome/renderer/media/mock_webrtc_logging_message_filter.cc View 1 2 3 4 5 6 1 chunk +4 lines, -2 lines 0 comments Download
M chrome/renderer/media/webrtc_logging_message_filter.h View 1 2 3 4 2 chunks +3 lines, -1 line 0 comments Download
M chrome/renderer/media/webrtc_logging_message_filter.cc View 1 2 3 4 1 chunk +3 lines, -2 lines 0 comments Download
M content/renderer/media/webrtc_logging.cc View 1 2 3 4 2 chunks +4 lines, -29 lines 0 comments Download
M third_party/libjingle/overrides/talk/base/logging.h View 1 chunk +0 lines, -1 line 0 comments Download
M third_party/libjingle/overrides/talk/base/logging.cc View 3 chunks +1 line, -15 lines 0 comments Download

Messages

Total messages: 44 (0 generated)
jiayl
dcheng: webrtc_logging_messages.h ronghuawu: libjingle/overrides/* vrk: */media/* PTAL
6 years, 8 months ago (2014-03-31 21:47:19 UTC) #1
Ronghua Wu (Left Chromium)
grunell might be the better person for the override logging.h|cc changes.
6 years, 8 months ago (2014-03-31 21:51:01 UTC) #2
jiayl
On 2014/03/31 21:51:01, Ronghua Wu wrote: > grunell might be the better person for the ...
6 years, 8 months ago (2014-03-31 21:52:33 UTC) #3
Ronghua Wu (Left Chromium)
I can stamp after the review. On Mon, Mar 31, 2014 at 2:52 PM, <jiayl@chromium.org> ...
6 years, 8 months ago (2014-03-31 21:54:43 UTC) #4
jiayl
Ping reviewers.
6 years, 8 months ago (2014-04-01 17:19:47 UTC) #5
jiayl
6 years, 8 months ago (2014-04-01 21:16:00 UTC) #6
tommi (sloooow) - chröme
What about having a struct instead that has std::string and timestamp members? Managing this in ...
6 years, 8 months ago (2014-04-01 21:23:38 UTC) #7
jiayl
On 2014/04/01 21:23:38, tommi wrote: > What about having a struct instead that has std::string ...
6 years, 8 months ago (2014-04-01 22:40:27 UTC) #8
dcheng
https://codereview.chromium.org/218403004/diff/40001/chrome/browser/media/webrtc_logging_handler_host.cc File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/218403004/diff/40001/chrome/browser/media/webrtc_logging_handler_host.cc#newcode226 chrome/browser/media/webrtc_logging_handler_host.cc:226: base::Time::Now().ToInternalValue() / 1) Shouldn't we be using base::TimeTicks? base::Time::Now() ...
6 years, 8 months ago (2014-04-01 23:35:19 UTC) #9
jiayl
https://codereview.chromium.org/218403004/diff/40001/chrome/browser/media/webrtc_logging_handler_host.cc File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/218403004/diff/40001/chrome/browser/media/webrtc_logging_handler_host.cc#newcode226 chrome/browser/media/webrtc_logging_handler_host.cc:226: base::Time::Now().ToInternalValue() / On 2014/04/01 23:35:19, dcheng wrote: > 1) ...
6 years, 8 months ago (2014-04-01 23:50:06 UTC) #10
jiayl
PTAL. Thanks! https://codereview.chromium.org/218403004/diff/40001/chrome/browser/media/webrtc_logging_handler_host.cc File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/218403004/diff/40001/chrome/browser/media/webrtc_logging_handler_host.cc#newcode226 chrome/browser/media/webrtc_logging_handler_host.cc:226: base::Time::Now().ToInternalValue() / On 2014/04/01 23:50:06, jiayl wrote: ...
6 years, 8 months ago (2014-04-02 00:38:06 UTC) #11
dcheng
chrome/common/media/webrtc_logging_messages.h LGTM with one minor comment. https://codereview.chromium.org/218403004/diff/60001/chrome/browser/media/webrtc_logging_handler_host.h File chrome/browser/media/webrtc_logging_handler_host.h (right): https://codereview.chromium.org/218403004/diff/60001/chrome/browser/media/webrtc_logging_handler_host.h#newcode112 chrome/browser/media/webrtc_logging_handler_host.h:112: void AddLogMessageFromBrowser(const base::Time& ...
6 years, 8 months ago (2014-04-02 01:20:00 UTC) #12
tommi (sloooow) - chröme
https://codereview.chromium.org/218403004/diff/60001/chrome/browser/media/webrtc_logging_handler_host.cc File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/218403004/diff/60001/chrome/browser/media/webrtc_logging_handler_host.cc#newcode265 chrome/browser/media/webrtc_logging_handler_host.cc:265: const base::Time& timestamp, const std::string& message) { can we ...
6 years, 8 months ago (2014-04-02 15:17:58 UTC) #13
jiayl
sky: please review the new files in chrome/common/media/
6 years, 8 months ago (2014-04-02 16:22:49 UTC) #14
jiayl
PTAL https://codereview.chromium.org/218403004/diff/60001/chrome/browser/media/webrtc_logging_handler_host.cc File chrome/browser/media/webrtc_logging_handler_host.cc (right): https://codereview.chromium.org/218403004/diff/60001/chrome/browser/media/webrtc_logging_handler_host.cc#newcode265 chrome/browser/media/webrtc_logging_handler_host.cc:265: const base::Time& timestamp, const std::string& message) { On ...
6 years, 8 months ago (2014-04-02 16:23:59 UTC) #15
tommi (sloooow) - chröme
lgtm https://codereview.chromium.org/218403004/diff/80001/chrome/common/media/webrtc_logging_message_data.h File chrome/common/media/webrtc_logging_message_data.h (right): https://codereview.chromium.org/218403004/diff/80001/chrome/common/media/webrtc_logging_message_data.h#newcode20 chrome/common/media/webrtc_logging_message_data.h:20: std::string Format(const base::Time& start_time) const; Nice. thanks for ...
6 years, 8 months ago (2014-04-02 17:28:46 UTC) #16
jiayl
Ronghua, tommi has given lgtm on behalf of Henrik (OOO). PTAL.
6 years, 8 months ago (2014-04-02 17:49:11 UTC) #17
Ronghua Wu (Left Chromium)
Owner stamp on libjingle/overrides/*. LGTM
6 years, 8 months ago (2014-04-02 17:50:21 UTC) #18
dcheng
https://codereview.chromium.org/218403004/diff/80001/chrome/common/media/webrtc_logging_message_data.h File chrome/common/media/webrtc_logging_message_data.h (right): https://codereview.chromium.org/218403004/diff/80001/chrome/common/media/webrtc_logging_message_data.h#newcode20 chrome/common/media/webrtc_logging_message_data.h:20: std::string Format(const base::Time& start_time) const; Per my earlier comment, ...
6 years, 8 months ago (2014-04-02 17:52:08 UTC) #19
jiayl
https://codereview.chromium.org/218403004/diff/80001/chrome/common/media/webrtc_logging_message_data.h File chrome/common/media/webrtc_logging_message_data.h (right): https://codereview.chromium.org/218403004/diff/80001/chrome/common/media/webrtc_logging_message_data.h#newcode20 chrome/common/media/webrtc_logging_message_data.h:20: std::string Format(const base::Time& start_time) const; On 2014/04/02 17:52:09, dcheng ...
6 years, 8 months ago (2014-04-02 17:59:01 UTC) #20
sky
LGTM
6 years, 8 months ago (2014-04-02 20:00:06 UTC) #21
jiayl
The CQ bit was checked by jiayl@chromium.org
6 years, 8 months ago (2014-04-02 20:30:34 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/218403004/120001
6 years, 8 months ago (2014-04-02 20:32:35 UTC) #23
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-02 21:25:28 UTC) #24
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=292858
6 years, 8 months ago (2014-04-02 21:25:29 UTC) #25
jiayl
The CQ bit was checked by jiayl@chromium.org
6 years, 8 months ago (2014-04-02 21:39:00 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/218403004/140001
6 years, 8 months ago (2014-04-02 21:40:34 UTC) #27
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-02 21:51:02 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
6 years, 8 months ago (2014-04-02 21:51:03 UTC) #29
jiayl
The CQ bit was checked by jiayl@chromium.org
6 years, 8 months ago (2014-04-02 21:53:21 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/218403004/140001
6 years, 8 months ago (2014-04-02 21:53:48 UTC) #31
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-02 22:34:17 UTC) #32
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
6 years, 8 months ago (2014-04-02 22:34:18 UTC) #33
jiayl
The CQ bit was checked by jiayl@chromium.org
6 years, 8 months ago (2014-04-02 22:50:23 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/218403004/160001
6 years, 8 months ago (2014-04-02 22:53:10 UTC) #35
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-03 00:05:10 UTC) #36
commit-bot: I haz the power
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_dbg&number=167380
6 years, 8 months ago (2014-04-03 00:05:11 UTC) #37
jiayl
The CQ bit was checked by jiayl@chromium.org
6 years, 8 months ago (2014-04-03 00:18:38 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/218403004/170001
6 years, 8 months ago (2014-04-03 00:22:12 UTC) #39
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-03 02:01:14 UTC) #40
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=293017
6 years, 8 months ago (2014-04-03 02:01:15 UTC) #41
jiayl
The CQ bit was checked by jiayl@chromium.org
6 years, 8 months ago (2014-04-03 16:58:04 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/218403004/170001
6 years, 8 months ago (2014-04-03 16:58:17 UTC) #43
commit-bot: I haz the power
6 years, 8 months ago (2014-04-03 17:53:03 UTC) #44
Message was sent while issue was closed.
Change committed as 261472

Powered by Google App Engine
This is Rietveld 408576698