media-internals: Store updates when cannot update.

media-internals: Store MediaLog events for future use.

Developed based on watk@'s CL with modifications.

With this CL, opening chrome://media-internals after HTML5 media playback has
happened is working.

Technical details:
- In the browser process, always save up to 128 latest media log events per
  render process. If we hit the limit, old logs will be discarded.
- When chrome://media-internals UI is opened, all saved logs will be sent to the
  UI.
- When a tab is closed, all saved logs associated with that tab is discarded.

Test shows that 128 log entries should be sufficient for 4 youtube video
playbacks (with minor seeking activities).

BUG=497286
TEST=Manually tested.

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

[xhwang, 2015-06-09 01:02:28]

xhwang@chromium.org changed reviewers:
+ dalecurtis@chromium.org, watk@chromium.org

[xhwang, 2015-06-09 01:02:29]

This works for most cases, but it has a problem. Currently I am not using
different maps to store the updates. So when a tab is closed, I am not removing
the logs associated with that tab from the |pending_updates_|. As a result, if a
user opens media-internals UI, the player in the closed tab will still appear,
and will not disappear.

Note that currently players associated with a tab will disappear immediately if
a tab is closed.

I actually found it useful keeping the logs even after the tab is closed.
Sometimes I accidentally closed a tab after I see a playback error, and as such
I lost all the logs which could've be useful.

WDYT on this?

[DaleCurtis, 2015-06-09 16:41:33]

You should expire that data, consider how large it might be by the end of a
session and that it could linger across incognito tabs.

In terms of memory usage this seems like it might add up to ~256k of memory
usage (assuming 64 character, 2 byte strings)?  In practice, with heavy
browsing, what kind of memory usage are you seeing?

https://codereview.chromium.org/1169983003/diff/1/content/browser/media/media...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/1169983003/diff/1/content/browser/media/media...
content/browser/media/media_internals.cc:619: static const size_t kMaxNumUpdates
= 2048;
Usually just const at function scope.

[xhwang, 2015-06-09 20:37:51]

Improvements over the last CL:
- Store events instead of updates to minimize memory usage.
- Use per process storage so that we could expire data when a process is
terminated.

Now the memory increase is about 8k per render process if we do play HTML5 video
in that process, and hit the 128 events limit (using the 64 char assumption). (I
don't find an easy way to actually measure the size of DictionaryValue though.)

[xhwang, 2015-06-09 20:37:58]

https://codereview.chromium.org/1169983003/diff/1/content/browser/media/media...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/1169983003/diff/1/content/browser/media/media...
content/browser/media/media_internals.cc:619: static const size_t kMaxNumUpdates
= 2048;
On 2015/06/09 16:41:32, DaleCurtis wrote:
> Usually just const at function scope.

Done.

[xhwang, 2015-06-09 20:38:25]

xhwang@chromium.org changed reviewers:
+ wolenetz@chromium.org
- watk@chromium.org

[xhwang, 2015-06-09 20:39:21]

s/watk/wolenetz since watk is OOO.

I updated this CL and it's in a better shape now. PTAL!

[xhwang, 2015-06-09 20:43:37]

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
content/browser/media/media_internals.cc:513: SaveEvent(render_process_id,
event);
Note that now we always SaveEvent() to keep the list up-to-date.

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
content/browser/media/media_internals.cc:553: }
We don't clear the list here, so that refreshing the UI, or opening the second
UI works pretty nicely.

[DaleCurtis, 2015-06-09 20:54:00]

lgtm % nits and 1 q.

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
content/browser/media/media_internals.cc:415: // Post the task to the IO thread
to avoid race in updating renderer_info_ map
Shouldn't you always post to fix the race you're talking about here?

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
content/browser/media/media_internals.cc:477: // TODO(dalecurtis): This is
technically not correct.  TimeTicks "can't" be
We should probably fix this at some point too. I think we want to record the
time of the first event and use that as a base time ticks value to subtract from
each given event time.

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
content/browser/media/media_internals.cc:553: }
On 2015/06/09 20:43:37, xhwang wrote:
> We don't clear the list here, so that refreshing the UI, or opening the second
> UI works pretty nicely.

Add this as a comment? :)

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
content/browser/media/media_internals.cc:641: return;
{} for multi-line if.

[wolenetz, 2015-06-09 22:35:24]

lgtm % nit and 1 q, too :)

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
content/browser/media/media_internals.cc:645: if (pending_events.size() >=
kMaxNumEvents)
We should notify the user that we have front-truncated the eventlog. If
complicated, please add a TODO()+bug for some later CL.

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
File content/browser/media/media_internals.h (right):

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
content/browser/media/media_internals.h:108: // Saves |event| so that they can
be sent when there are UpdateCallback
Do I understand correctly that GetEverything()'s call to SendHistorical..() is
when an UpdateCallback is registered? Or is there some other meaning to this
comment that I'm missing?

[xhwang, 2015-06-09 22:41:10]

comments addressed

[xhwang, 2015-06-09 22:41:19]

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
content/browser/media/media_internals.cc:415: // Post the task to the IO thread
to avoid race in updating renderer_info_ map
On 2015/06/09 20:54:00, DaleCurtis wrote:
> Shouldn't you always post to fix the race you're talking about here? 

This is not new code, I moved it from the old
MediaInternals::MediaInternalsUMAHandler::Observe().

Here we are actually always posting from UI thread to IO thread.

Updated the comment a bit to make it more clear (I hope).

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
content/browser/media/media_internals.cc:477: // TODO(dalecurtis): This is
technically not correct.  TimeTicks "can't" be
On 2015/06/09 20:54:00, DaleCurtis wrote:
> We should probably fix this at some point too. I think we want to record the
> time of the first event and use that as a base time ticks value to subtract
from
> each given event time.

Acknowledged.

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
content/browser/media/media_internals.cc:553: }
On 2015/06/09 20:54:00, DaleCurtis wrote:
> On 2015/06/09 20:43:37, xhwang wrote:
> > We don't clear the list here, so that refreshing the UI, or opening the
second
> > UI works pretty nicely.
> 
> Add this as a comment? :)

Done.

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
content/browser/media/media_internals.cc:641: return;
On 2015/06/09 20:54:00, DaleCurtis wrote:
> {} for multi-line if.

Done.

[xhwang, 2015-06-09 22:51:26]

comments addressed

[xhwang, 2015-06-09 22:51:35]

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
File content/browser/media/media_internals.cc (right):

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
content/browser/media/media_internals.cc:645: if (pending_events.size() >=
kMaxNumEvents)
On 2015/06/09 22:35:24, wolenetz wrote:
> We should notify the user that we have front-truncated the eventlog. If
> complicated, please add a TODO()+bug for some later CL.

Done.

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
File content/browser/media/media_internals.h (right):

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
content/browser/media/media_internals.h:108: // Saves |event| so that they can
be sent when there are UpdateCallback
On 2015/06/09 22:35:24, wolenetz wrote:
> Do I understand correctly that GetEverything()'s call to SendHistorical..() is
> when an UpdateCallback is registered? Or is there some other meaning to this
> comment that I'm missing?

When a media-internals UI is opened, a UpdateCallback will be registered. Also,
it'll call GetEverything(). So yes they happen at almost the same time. But I
agree this is implementation detail and may cause confusion.

Simplified the comment.

[wolenetz, 2015-06-09 22:54:38]

lgtm2 Thanks!

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
File content/browser/media/media_internals.h (right):

https://codereview.chromium.org/1169983003/diff/20001/content/browser/media/m...
content/browser/media/media_internals.h:108: // Saves |event| so that they can
be sent when there are UpdateCallback
On 2015/06/09 22:51:35, xhwang wrote:
> On 2015/06/09 22:35:24, wolenetz wrote:
> > Do I understand correctly that GetEverything()'s call to SendHistorical..()
is
> > when an UpdateCallback is registered? Or is there some other meaning to this
> > comment that I'm missing?
> 
> When a media-internals UI is opened, a UpdateCallback will be registered.
Also,
> it'll call GetEverything(). So yes they happen at almost the same time. But I
> agree this is implementation detail and may cause confusion.
> 
> Simplified the comment.

Acknowledged.

[xhwang, 2015-06-09 23:24:27]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-06-09 23:24:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/1169983003/#ps50001
(title: "comments addressed")

[commit-bot: I haz the power, 2015-06-09 23:24:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1169983003/50001

[commit-bot: I haz the power, 2015-06-10 00:45:40]

Committed patchset #4 (id:50001)

[commit-bot: I haz the power, 2015-06-10 00:46:46]

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