chrome/browser/resources/media/webrtc_logs.js - Issue 1405373002: Fix WebRTC log list errors.

Unified Diff: chrome/browser/resources/media/webrtc_logs.js

Issue 1405373002: Fix WebRTC log list errors. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Updated log uploader unit test. Created 5 years, 2 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« no previous file with comments | « chrome/browser/media/webrtc_log_uploader_unittest.cc ('k') | chrome/browser/ui/webui/flash_ui.cc » ('j') | chrome/browser/ui/webui/media/webrtc_logs_ui.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: chrome/browser/resources/media/webrtc_logs.js

diff --git a/chrome/browser/resources/media/webrtc_logs.js b/chrome/browser/resources/media/webrtc_logs.js

index 21c8c120133008a6b3441955606e537a4fe32ca0..8ccf247cc02e1bc483b42b66294ba92e8fdda7f8 100644

--- a/chrome/browser/resources/media/webrtc_logs.js

+++ b/chrome/browser/resources/media/webrtc_logs.js

@@ -29,6 +29,10 @@ function updateWebRtcLogsList(uploads, version) {

var logBlock = document.createElement('div');

var title = document.createElement('h3');

+ // TODO(grunell): Use |capture_time| only. If empty, show "Unknown" or such.

+ // In october 2015, capture time was added to the log list, previously the

+ // local ID was used as capture time. It still needs to be backwards

+ // compatible for some time before changing here.

tommi (sloooow) - chröme 2015/10/16 18:24:48 Should the log include version information?

Henrik Grunell 2015/10/19 10:53:24 I assume you mean the log list, and update an old

tommi (sloooow) - chröme 2015/10/19 11:11:09 What I was getting at is that if we could say "ver

Henrik Grunell 2015/10/19 12:43:46 We'd have to convert the list info the new version

tommi (sloooow) - chröme 2015/10/19 12:51:42 What about changing capture_time in the C++ code t

Henrik Grunell 2015/10/19 12:58:32 I've already done that. :) Forgot to mention it la

On 2015/10/19 12:51:42, tommi wrote: > On 2015/10/19 12:43:46, Henrik Grunell wrote: > > On 2015/10/19 11:11:09, tommi wrote: > > > On 2015/10/19 10:53:24, Henrik Grunell wrote: > > > > On 2015/10/16 18:24:48, tommi wrote: > > > > > Should the log include version information? > > > > > > > > I assume you mean the log list, and update an old version to the new > format. > > > One > > > > problem is that the list is updated when writing a new entry, and if > reading > > > it > > > > here may happen before that. I'm not sure it's worth it to add a browser > > > startup > > > > timer for updating and refactor etc. Or did you mean just adapt what we > show > > > to > > > > the version? I don't see that being much better, but I'm open to that. > > > > > > What I was getting at is that if we could say "version 0 does not have the > > > capture_time, so we can assume it's not going to be there for that version. > > > Version 1 however does have it, so assume it's there". When writing the > file, > > > we always write the latest format. So yeah, basically we'd know for sure > > what's > > > in each version instead of guessing or having a period of testing what > fields > > > might be available. If we add one more, remove or modify, then things will > get > > > hairy if we just guess. > > > > We'd have to convert the list info the new version in that case to ensure all > > lines have the same version. Instead I did so that if no capture time, we fall > > back on local ID as capture time if within reasonable bounds, from when the > > feature was introduced until Now(). If it isn't, fall back on upload time. > > Within some time, a list should be in the new format anyway. Wdyt? > > What about changing capture_time in the C++ code to be the upload time and keep > the js code slightly simpler? > We could then move the version handling to there (C++ that is) if/when we need > it. Generally I'd prefer to have as few places as possible be aware of the > on-disk format but now it feels like we have two places.

I've already done that. :) Forgot to mention it last comment.

title.textContent =

loadTimeData.getStringF('webrtcLogHeaderFormat',

upload['capture_time'].length != 0 ?