Fix WebRTC log list errors.

Fix WebRTC log list errors.

We can no longer rely on the local ID being a time. This was changed when a Store() function was added to the extension API.

* Add capture_time to UploadList::UploadInfo. Rename other variables.
* Update UploadList::ParseLogEntries() to parse the new field.
* Write capture time to list when storing a WebRTC log locally or uploading.

BUG=540014
NOTRY=true

Committed: https://crrev.com/b452aa349bd1f30fa5e4c9664e65b37b8787a91d
Cr-Commit-Position: refs/heads/master@{#355747}

[Henrik Grunell, 2015-10-16 13:23:54]

grunell@chromium.org changed reviewers:
+ tommi@chromium.org

[Henrik Grunell, 2015-10-16 13:29:48]

grunell@chromium.org changed reviewers:
+ estade@chromium.org, rsesek@chromium.org, stuartmorgan@chromium.org

[Henrik Grunell, 2015-10-16 13:30:20]

tommi@: All.
estade@: chrome/browser/resources/media/webrtc_logs.js
         chrome/browser/ui/webui/*
rsesek@: components/crash/core/browser/crashes_ui_util.cc
stuartmorgan@: components/upload_list/*

[Evan Stade, 2015-10-16 16:37:20]

my files lgtm

https://codereview.chromium.org/1405373002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media/webrtc_logs_ui.cc (right):

https://codereview.chromium.org/1405373002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media/webrtc_logs_ui.cc:151: base::DictionaryValue*
upload = new base::DictionaryValue();
can you make this a scoped_ptr

https://codereview.chromium.org/1405373002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media/webrtc_logs_ui.cc:161: value_w =
base::TimeFormatFriendlyDateAndTime(i->capture_time);
nit: no curlies

[Robert Sesek, 2015-10-16 18:10:52]

components/crash LGTM

[tommi (sloooow) - chröme, 2015-10-16 18:24:48]

can you ping back when the bots are green?

https://codereview.chromium.org/1405373002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/media/webrtc_logs.js (right):

https://codereview.chromium.org/1405373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media/webrtc_logs.js:35: // compatible for some time
before changing here.
Should the log include version information?

[Henrik Grunell, 2015-10-19 10:52:20]

grunell@chromium.org changed reviewers:
+ thestig@chromium.org

[Henrik Grunell, 2015-10-19 10:53:24]

Now try bots are fine.

+thestig@ for review of chrome/browser/crash_upload_list_mac.cc

https://codereview.chromium.org/1405373002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/media/webrtc_logs.js (right):

https://codereview.chromium.org/1405373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media/webrtc_logs.js:35: // compatible for some time
before changing here.
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.

https://codereview.chromium.org/1405373002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/media/webrtc_logs_ui.cc (right):

https://codereview.chromium.org/1405373002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media/webrtc_logs_ui.cc:151: base::DictionaryValue*
upload = new base::DictionaryValue();
On 2015/10/16 16:37:20, Evan Stade wrote:
> can you make this a scoped_ptr

Done.

https://codereview.chromium.org/1405373002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/media/webrtc_logs_ui.cc:161: value_w =
base::TimeFormatFriendlyDateAndTime(i->capture_time);
On 2015/10/16 16:37:20, Evan Stade wrote:
> nit: no curlies

Done.

[tommi (sloooow) - chröme, 2015-10-19 11:11:09]

lgtm

https://codereview.chromium.org/1405373002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/media/webrtc_logs.js (right):

https://codereview.chromium.org/1405373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media/webrtc_logs.js:35: // compatible for some time
before changing here.
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.

[Henrik Grunell, 2015-10-19 12:43:46]

https://codereview.chromium.org/1405373002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/media/webrtc_logs.js (right):

https://codereview.chromium.org/1405373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media/webrtc_logs.js:35: // compatible for some time
before changing here.
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?

[tommi (sloooow) - chröme, 2015-10-19 12:51:42]

https://codereview.chromium.org/1405373002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/media/webrtc_logs.js (right):

https://codereview.chromium.org/1405373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media/webrtc_logs.js:35: // compatible for some time
before changing here.
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.

[Henrik Grunell, 2015-10-19 12:58:32]

https://codereview.chromium.org/1405373002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/media/webrtc_logs.js (right):

https://codereview.chromium.org/1405373002/diff/20001/chrome/browser/resource...
chrome/browser/resources/media/webrtc_logs.js:35: // compatible for some time
before changing here.
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.

[tommi (sloooow) - chröme, 2015-10-19 14:08:26]

https://codereview.chromium.org/1405373002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/media/webrtc_logs.js (right):

https://codereview.chromium.org/1405373002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media/webrtc_logs.js:34: upload['capture_time'].length
!= 0 ?
do you still need this check?

[Henrik Grunell, 2015-10-19 14:31:07]

https://codereview.chromium.org/1405373002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/media/webrtc_logs.js (right):

https://codereview.chromium.org/1405373002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/media/webrtc_logs.js:34: upload['capture_time'].length
!= 0 ?
On 2015/10/19 14:08:26, tommi wrote:
> do you still need this check?

Moved this check to c++ as well.

[tommi (sloooow) - chröme, 2015-10-19 15:25:56]

lgtm

[Lei Zhang, 2015-10-19 18:56:32]

On 2015/10/19 10:53:24, Henrik Grunell wrote:
> Now try bots are fine.
> 
> +thestig@ for review of chrome/browser/crash_upload_list_mac.cc

lgtm

[Henrik Grunell, 2015-10-20 05:54:34]

stuartmorgan@: ping for review of components/upload_list/*

[Henrik Grunell, 2015-10-20 05:57:21]

Description was changed from

==========
Fix WebRTC log list errors.

* Add capture_time to UploadList::UploadInfo. Rename other variables.
* Update UploadList::ParseLogEntries() to parse the new field.
* Write capture time to list when storing a WebRTC log locally or uploading.

BUG=540014
==========

to

==========
Fix WebRTC log list errors.

We can no longer rely on the local ID being a time. This was changed when a
Store() function was added to the extension API.

* Add capture_time to UploadList::UploadInfo. Rename other variables.
* Update UploadList::ParseLogEntries() to parse the new field.
* Write capture time to list when storing a WebRTC log locally or uploading.

BUG=540014
==========

[stuartmorgan, 2015-10-20 14:23:12]

https://codereview.chromium.org/1405373002/diff/120001/components/upload_list...
File components/upload_list/upload_list.h (right):

https://codereview.chromium.org/1405373002/diff/120001/components/upload_list...
components/upload_list/upload_list.h:24: //
upload_time,upload_id[,local_id,capture_time]
Should probably be:
  upload_time,upload_id[,local_id[,capture_time]]
since the format here suggests only 2 or 4 are valid, but files with 3 are
already out there.

https://codereview.chromium.org/1405373002/diff/120001/components/upload_list...
File components/upload_list/upload_list_unittest.cc (right):

https://codereview.chromium.org/1405373002/diff/120001/components/upload_list...
components/upload_list/upload_list_unittest.cc:51: // Add 3 more entries.
There's a ton of duplication in these tests now. Having multiple entries tested
over and over doesn't seem to meaningfully improve coverage; how about pulling
out a test that does multiple entry test as its primary purpose, and then having
all the others just test each format with one entry.

https://codereview.chromium.org/1405373002/diff/120001/components/upload_list...
components/upload_list/upload_list_unittest.cc:140: // This is the format that
WebRTC logs that only is stored locally are stored
s/is stored/are stored/

https://codereview.chromium.org/1405373002/diff/120001/components/upload_list...
components/upload_list/upload_list_unittest.cc:178: // This is the format that
WebRTC logs that is stored locally and have been
s/is stored/are stored/

[Henrik Grunell, 2015-10-21 08:33:40]

https://codereview.chromium.org/1405373002/diff/120001/components/upload_list...
File components/upload_list/upload_list.h (right):

https://codereview.chromium.org/1405373002/diff/120001/components/upload_list...
components/upload_list/upload_list.h:24: //
upload_time,upload_id[,local_id,capture_time]
On 2015/10/20 14:23:12, stuartmorgan wrote:
> Should probably be:
>   upload_time,upload_id[,local_id[,capture_time]]
> since the format here suggests only 2 or 4 are valid, but files with 3 are
> already out there.

Right, done.

https://codereview.chromium.org/1405373002/diff/120001/components/upload_list...
File components/upload_list/upload_list_unittest.cc (right):

https://codereview.chromium.org/1405373002/diff/120001/components/upload_list...
components/upload_list/upload_list_unittest.cc:51: // Add 3 more entries.
On 2015/10/20 14:23:12, stuartmorgan wrote:
> There's a ton of duplication in these tests now. Having multiple entries
tested
> over and over doesn't seem to meaningfully improve coverage; how about pulling
> out a test that does multiple entry test as its primary purpose, and then
having
> all the others just test each format with one entry.

Agree. Done.

https://codereview.chromium.org/1405373002/diff/120001/components/upload_list...
components/upload_list/upload_list_unittest.cc:140: // This is the format that
WebRTC logs that only is stored locally are stored
On 2015/10/20 14:23:12, stuartmorgan wrote:
> s/is stored/are stored/

Done.

https://codereview.chromium.org/1405373002/diff/120001/components/upload_list...
components/upload_list/upload_list_unittest.cc:178: // This is the format that
WebRTC logs that is stored locally and have been
On 2015/10/20 14:23:12, stuartmorgan wrote:
> s/is stored/are stored/

Done.

[Henrik Grunell, 2015-10-21 08:38:02]

The CQ bit was checked by grunell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-21 08:38:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1405373002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1405373002/160001

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

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

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

Dry run: This issue passed the CQ dry run.

[Henrik Grunell, 2015-10-22 09:42:41]

Stuart: ptal.

[stuartmorgan, 2015-10-22 14:54:42]

lgtm

[Henrik Grunell, 2015-10-22 14:56:40]

The CQ bit was checked by grunell@chromium.org

[Henrik Grunell, 2015-10-22 14:56:40]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
estade@chromium.org, thestig@chromium.org, tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/1405373002/#ps160001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-10-22 14:57:06]

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

[commit-bot: I haz the power, 2015-10-22 16:32:54]

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

[commit-bot: I haz the power, 2015-10-22 16:32:54]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Henrik Grunell, 2015-10-23 07:16:24]

Description was changed from

==========
Fix WebRTC log list errors.

We can no longer rely on the local ID being a time. This was changed when a
Store() function was added to the extension API.

* Add capture_time to UploadList::UploadInfo. Rename other variables.
* Update UploadList::ParseLogEntries() to parse the new field.
* Write capture time to list when storing a WebRTC log locally or uploading.

BUG=540014
==========

to

==========
Fix WebRTC log list errors.

We can no longer rely on the local ID being a time. This was changed when a
Store() function was added to the extension API.

* Add capture_time to UploadList::UploadInfo. Rename other variables.
* Update UploadList::ParseLogEntries() to parse the new field.
* Write capture time to list when storing a WebRTC log locally or uploading.

BUG=540014
NOTRY=true
==========

[Henrik Grunell, 2015-10-23 07:16:31]

The CQ bit was checked by grunell@chromium.org

[commit-bot: I haz the power, 2015-10-23 07:16:41]

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

[commit-bot: I haz the power, 2015-10-23 07:21:19]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-10-23 07:22:12]

Patchset 9 (id:??) landed as
https://crrev.com/b452aa349bd1f30fa5e4c9664e65b37b8787a91d
Cr-Commit-Position: refs/heads/master@{#355747}