Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
183973021:
Add metrics to track the duration of tracks received over a PeerConnection. (Closed)
|
Created:
6 years, 9 months ago by Jói Modified:
6 years, 9 months ago Reviewers:
perkj_chrome, Alexei Svitkine (slow), jschuh, no longer working on chromium, jochen (gone - plz use gerrit) CC:
chromium-reviews, jar (doing other things), fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, wjia+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd metrics to track the duration of tracks received over a PeerConnection.
NOTRY=true
TBR=jochen@chromium.org
BUG=348616
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256780
Patch Set 1 #
Total comments: 4
Patch Set 2 : . #Patch Set 3 : Now logging in browser process. #
Total comments: 17
Patch Set 4 : Address review comments. #
Total comments: 8
Patch Set 5 : Lower similarity, fix .gypi file. #Patch Set 6 : Use pointer value as ID. #
Total comments: 1
Patch Set 7 : Only send messages if RenderThreadImpl::current() is available. #Patch Set 8 : Fixes based on more manual testing. #Patch Set 9 : No need to subscribe to notifications; BrowserMessageFilter is for one particular render process. #Patch Set 10 : Merge to LKGR. #Messages
Total messages: 42 (0 generated)
Hi Per and Shijing, Could you review and see what you think before I send this to asvitkine@ for the tools/metrics review? These observers seem to me to accurately match the lifetime of tracks, whether they are received right away in a PeerConnection, or added later, and whether they are removed only at the end of the PeerConnection or earlier. So this seems like an ideal choke point for received tracks. I've been discussing with niklase@ and we believe we also want to add metrics for sent tracks, since in hub-and-spoke topologies (like Hangouts) you might get multiple received tracks but only a single sent track (e.g. for video). Cheers, Jói
drive-by https://codereview.chromium.org/183973021/diff/1/content/renderer/media/remot... File content/renderer/media/remote_media_stream_impl.cc (right): https://codereview.chromium.org/183973021/diff/1/content/renderer/media/remot... content/renderer/media/remote_media_stream_impl.cc:103: UMA_HISTOGRAM_CUSTOM_TIMES(histogram_name, This is a bug - the name parameter to UMA macros must be a compile-time constant.
> This is a bug - the name parameter to UMA macros must be a compile-time
> constant.
I believe that only applies to usage of UserMetricsAction, as in
RecordAction(UserMetricsAction("FooBlat")), which needs to be
standardized this way so that tools/metrics/actions/extract_actions.py
can extract action names from the code. I don't think histograms are
done this way, instead we have histograms.xml.
The following code search reveals multiple usages where the name is
not a compile-time constant, so if this truly is a bug we'll need to
fix it in lots of places:
https://code.google.com/p/chromium/codesearch#search/&q=UMA_HISTOGRAM%5B%5E(%...
Cheers,
Jói
On Wed, Mar 5, 2014 at 8:44 AM, <fischman@chromium.org> wrote:
> drive-by
>
>
>
https://codereview.chromium.org/183973021/diff/1/content/renderer/media/remot...
> File content/renderer/media/remote_media_stream_impl.cc (right):
>
>
https://codereview.chromium.org/183973021/diff/1/content/renderer/media/remot...
> content/renderer/media/remote_media_stream_impl.cc:103:
> UMA_HISTOGRAM_CUSTOM_TIMES(histogram_name,
> This is a bug - the name parameter to UMA macros must be a compile-time
> constant.
>
> https://codereview.chromium.org/183973021/
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
This will track the lifetime from creation to maybe the end of the track. But what if the tab is closed and the fast shutdown path is used? https://codereview.chromium.org/183973021/diff/1/content/renderer/media/remot... File content/renderer/media/remote_media_stream_impl.cc (right): https://codereview.chromium.org/183973021/diff/1/content/renderer/media/remot... content/renderer/media/remote_media_stream_impl.cc:92: RemoteMediaStreamTrackObserver::~RemoteMediaStreamTrackObserver() { If you close a tab - which I guess is very common when you quit a call- the render process destructors don't run. Is that a problem for your history?
I don't think there is anything reliable we can do on the renderer side to track the duration when the fast shutdown path is invoked. I'm not sure it matters much since we're probably mostly interested in the distribution of call durations. However, if there is a reliable place on the browser side where we can do the same determination, that might be better. On Wed, Mar 5, 2014 at 9:39 AM, <perkj@chromium.org> wrote: > This will track the lifetime from creation to maybe the end of the track. > But > what if the tab is closed and the fast shutdown path is used? > > > > > > https://codereview.chromium.org/183973021/diff/1/content/renderer/media/remot... > File content/renderer/media/remote_media_stream_impl.cc (right): > > https://codereview.chromium.org/183973021/diff/1/content/renderer/media/remot... > content/renderer/media/remote_media_stream_impl.cc:92: > RemoteMediaStreamTrackObserver::~RemoteMediaStreamTrackObserver() { > If you close a tab - which I guess is very common when you quit a call- > the render process destructors don't run. Is that a problem for your > history? > > https://codereview.chromium.org/183973021/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/183973021/diff/1/content/renderer/media/remot... File content/renderer/media/remote_media_stream_impl.cc (right): https://codereview.chromium.org/183973021/diff/1/content/renderer/media/remot... content/renderer/media/remote_media_stream_impl.cc:103: UMA_HISTOGRAM_CUSTOM_TIMES(histogram_name, On 2014/03/05 08:44:27, Ami Fischman wrote: > This is a bug - the name parameter to UMA macros must be a compile-time > constant. The histogram macros cache the Histogram object after the first invocation at the call site, so while the name doesn't have to be a string literal (like it does for actions), you do need a separate macro callsite for each distinct histogram name you're logging.
OK, thanks Alexei. Could I do something like define my own local macro (UMA_HISTOGRAM_TIMES_16H) and then call that at two different call sites? Cheers, Jói On Wed, Mar 5, 2014 at 2:23 PM, <asvitkine@chromium.org> wrote: > > https://codereview.chromium.org/183973021/diff/1/content/renderer/media/remot... > File content/renderer/media/remote_media_stream_impl.cc (right): > > https://codereview.chromium.org/183973021/diff/1/content/renderer/media/remot... > content/renderer/media/remote_media_stream_impl.cc:103: > UMA_HISTOGRAM_CUSTOM_TIMES(histogram_name, > > On 2014/03/05 08:44:27, Ami Fischman wrote: >> >> This is a bug - the name parameter to UMA macros must be a > > compile-time >> >> constant. > > > The histogram macros cache the Histogram object after the first > invocation at the call site, so while the name doesn't have to be a > string literal (like it does for actions), you do need a separate macro > callsite for each distinct histogram name you're logging. > > https://codereview.chromium.org/183973021/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
That SGTM. On Wed, Mar 5, 2014 at 9:31 AM, Jói Sigurðsson <joi@chromium.org> wrote: > OK, thanks Alexei. > > Could I do something like define my own local macro > (UMA_HISTOGRAM_TIMES_16H) and then call that at two different call > sites? > > Cheers, > Jói > > > On Wed, Mar 5, 2014 at 2:23 PM, <asvitkine@chromium.org> wrote: > > > > > https://codereview.chromium.org/183973021/diff/1/content/renderer/media/remot... > > File content/renderer/media/remote_media_stream_impl.cc (right): > > > > > https://codereview.chromium.org/183973021/diff/1/content/renderer/media/remot... > > content/renderer/media/remote_media_stream_impl.cc:103: > > UMA_HISTOGRAM_CUSTOM_TIMES(histogram_name, > > > > On 2014/03/05 08:44:27, Ami Fischman wrote: > >> > >> This is a bug - the name parameter to UMA macros must be a > > > > compile-time > >> > >> constant. > > > > > > The histogram macros cache the Histogram object after the first > > invocation at the call site, so while the name doesn't have to be a > > string literal (like it does for actions), you do need a separate macro > > callsite for each distinct histogram name you're logging. > > > > https://codereview.chromium.org/183973021/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/183973021/diff/1/content/renderer/media/remot... File content/renderer/media/remote_media_stream_impl.cc (right): https://codereview.chromium.org/183973021/diff/1/content/renderer/media/remot... content/renderer/media/remote_media_stream_impl.cc:130: webkit_track_.source().setReadyState( Joi, I am not sure, but if you move the code from the destructor to this kEnded event, does it address Per's concern?
Per, could you take a look at the new approach of logging stuff in the browser process only? I'll send it to more reviewers once you're happy. Cheers, Jói
https://codereview.chromium.org/183973021/diff/40001/content/browser/renderer... File content/browser/renderer_host/media/media_track_lifetime_metrics_host.cc (right): https://codereview.chromium.org/183973021/diff/40001/content/browser/renderer... content/browser/renderer_host/media/media_track_lifetime_metrics_host.cc:62: ReportDuration(it->second.first, it->second.second); nit: can you use temp variables instead of second.first second.second. My memory fails to tell me what they mean. https://codereview.chromium.org/183973021/diff/40001/content/browser/renderer... content/browser/renderer_host/media/media_track_lifetime_metrics_host.cc:69: DCHECK(tracks_.find(id) == tracks_.end()); To make this a bit harder - the same track can theoretically be received in multiple mediastreams on the same PeerConnection. They will have the same id. So the unique identifier is ms id + track id. https://codereview.chromium.org/183973021/diff/40001/content/browser/renderer... content/browser/renderer_host/media/media_track_lifetime_metrics_host.cc:86: UMA_HISTOGRAM_TIMES_16H("WebRTC.ReceivedAudioTrackDuration", duration); Will we not have the same problem for local tracks and need to track the duration in the same way? ie- extend the identifier to be more than just bool audio? I guess we can also argue what it is we want to track. Is it the source duration or the track duration? I mean- you are supposed to be able to clone all sort of tracks, both local and remote and I guess you only want to track the duration of the original track or source. But starting talking about sources here might also be confusing... Are we only going to track the original - none cloned track? How about adding a comment about that somewhere and continue to have the UMA histogram called TrackDuration? https://codereview.chromium.org/183973021/diff/40001/content/common/media/med... File content/common/media/media_track_lifetime_metrics_messages.h (right): https://codereview.chromium.org/183973021/diff/40001/content/common/media/med... content/common/media/media_track_lifetime_metrics_messages.h:16: bool /* is_audio */) local and remote tracks ? https://codereview.chromium.org/183973021/diff/40001/content/content_browser.... File content/content_browser.gypi (right): https://codereview.chromium.org/183973021/diff/40001/content/content_browser.... content/content_browser.gypi:978: 'browser/renderer_host/media/media_track_lifetime_metrics_host.cc', humm- I think we call this media_stream_track etc in the renderer. How about media_stream_track_metrics_host.cc? https://codereview.chromium.org/183973021/diff/40001/content/renderer/media/r... File content/renderer/media/remote_media_stream_impl.cc (right): https://codereview.chromium.org/183973021/diff/40001/content/renderer/media/r... content/renderer/media/remote_media_stream_impl.cc:90: RenderThreadImpl::current()->Send(new MediaTrackLifetimeMetrics_AddTrack( Can we do this in a virtual method instead so I can write a unit test for the track observer that can't deal whith the static render thread and ipc messages? I am refactoring this class currently to support having the remote video tracks be a source to the chrome tracks as discussed. I can do that in my cl if you think that is the correct approach. https://codereview.chromium.org/183973021/diff/40001/content/renderer/media/r... content/renderer/media/remote_media_stream_impl.cc:91: webkit_track_.id().utf8(), To make this a bit harder - the same track can theoretically be received in multiple mediastreams on the same PeerConnection. They will have the same id. https://codereview.chromium.org/183973021/diff/40001/content/renderer/media/r... content/renderer/media/remote_media_stream_impl.cc:96: RenderThreadImpl::current()->Send(new MediaTrackLifetimeMetrics_RemoveTrack( dito https://codereview.chromium.org/183973021/diff/40001/ipc/ipc_message_start.h File ipc/ipc_message_start.h (right): https://codereview.chromium.org/183973021/diff/40001/ipc/ipc_message_start.h#... ipc/ipc_message_start.h:100: MediaTrackLifetimeMetricsHostMsgStart, Call this MediaStreamTrack something- http://dev.w3.org/2011/webrtc/editor/getusermedia.html
PTAL https://codereview.chromium.org/183973021/diff/40001/content/browser/renderer... File content/browser/renderer_host/media/media_track_lifetime_metrics_host.cc (right): https://codereview.chromium.org/183973021/diff/40001/content/browser/renderer... content/browser/renderer_host/media/media_track_lifetime_metrics_host.cc:62: ReportDuration(it->second.first, it->second.second); On 2014/03/11 08:05:21, perkj wrote: > nit: can you use temp variables instead of second.first second.second. My memory > fails to tell me what they mean. I added a temp IsAudioPlusTimestamp variable then I do audio_and_timestamp.first and audio_and_timestamp.second, that should be pretty clear. https://codereview.chromium.org/183973021/diff/40001/content/browser/renderer... content/browser/renderer_host/media/media_track_lifetime_metrics_host.cc:86: UMA_HISTOGRAM_TIMES_16H("WebRTC.ReceivedAudioTrackDuration", duration); On 2014/03/11 08:05:21, perkj wrote: > Will we not have the same problem for local tracks and need to track the > duration in the same way? > ie- extend the identifier to be more than just bool audio? > > I guess we can also argue what it is we want to track. Is it the source duration > or the track duration? > I mean- you are supposed to be able to clone all sort of tracks, both local and > remote and I guess you only want to track the duration of the original track or > source. But starting talking about sources here might also be confusing... Are > we only going to track the original - none cloned track? How about adding a > comment about that somewhere and continue to have the UMA histogram called > TrackDuration? I think we had settled on track duration in the design phase, but I agree with your concerns and I think it would be better to not count locally-cloned tracks. For now I suggest just documenting how this currently works, which I would do in histograms.xml. Do cloned tracks actually get reflected as RemoteMediaStreamTrackObserver interfaces in RemoteMediaStreamImpl? For sent tracks, I will expand this class when appropriate as I add that tracking. The distinction is not needed for now. https://codereview.chromium.org/183973021/diff/40001/content/common/media/med... File content/common/media/media_track_lifetime_metrics_messages.h (right): https://codereview.chromium.org/183973021/diff/40001/content/common/media/med... content/common/media/media_track_lifetime_metrics_messages.h:16: bool /* is_audio */) On 2014/03/11 08:05:21, perkj wrote: > local and remote tracks ? Makes sense to add that right away to avoid an additional roundtrip later through security review. Added. https://codereview.chromium.org/183973021/diff/40001/content/content_browser.... File content/content_browser.gypi (right): https://codereview.chromium.org/183973021/diff/40001/content/content_browser.... content/content_browser.gypi:978: 'browser/renderer_host/media/media_track_lifetime_metrics_host.cc', On 2014/03/11 08:05:21, perkj wrote: > humm- I think we call this media_stream_track etc in the renderer. > How about media_stream_track_metrics_host.cc? Done. https://codereview.chromium.org/183973021/diff/40001/content/renderer/media/r... File content/renderer/media/remote_media_stream_impl.cc (right): https://codereview.chromium.org/183973021/diff/40001/content/renderer/media/r... content/renderer/media/remote_media_stream_impl.cc:90: RenderThreadImpl::current()->Send(new MediaTrackLifetimeMetrics_AddTrack( On 2014/03/11 08:05:21, perkj wrote: > Can we do this in a virtual method instead so I can write a unit test for the > track observer that can't deal whith the static render thread and ipc messages? > I am refactoring this class currently to support having the remote video tracks > be a source to the chrome tracks as discussed. > > I can do that in my cl if you think that is the correct approach. Sure, sounds good. https://codereview.chromium.org/183973021/diff/40001/content/renderer/media/r... content/renderer/media/remote_media_stream_impl.cc:91: webkit_track_.id().utf8(), On 2014/03/11 08:05:21, perkj wrote: > To make this a bit harder - the same track can theoretically be received in > multiple mediastreams on the same PeerConnection. They will have the same id. I'm now using the MediaStream ID plus the track ID. https://codereview.chromium.org/183973021/diff/40001/content/renderer/media/r... content/renderer/media/remote_media_stream_impl.cc:96: RenderThreadImpl::current()->Send(new MediaTrackLifetimeMetrics_RemoveTrack( On 2014/03/11 08:05:21, perkj wrote: > dito Done. https://codereview.chromium.org/183973021/diff/40001/ipc/ipc_message_start.h File ipc/ipc_message_start.h (right): https://codereview.chromium.org/183973021/diff/40001/ipc/ipc_message_start.h#... ipc/ipc_message_start.h:100: MediaTrackLifetimeMetricsHostMsgStart, On 2014/03/11 08:05:21, perkj wrote: > Call this MediaStreamTrack something- > http://dev.w3.org/2011/webrtc/editor/getusermedia.html Done.
https://codereview.chromium.org/183973021/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/media_stream_track_metrics_host.h (right): https://codereview.chromium.org/183973021/diff/60001/content/browser/renderer... content/browser/renderer_host/media/media_stream_track_metrics_host.h:5: #ifndef CONTENT_BROWSER_RENDERER_HOST_MEDIA_MEDIA_STREAM_TRACK_METRICS_HOST_H_ nit : git mv is the reviewers friend I think.Or lowering the comparison bar when uploading the file. https://codereview.chromium.org/183973021/diff/60001/content/content_browser.... File content/content_browser.gypi (right): https://codereview.chromium.org/183973021/diff/60001/content/content_browser.... content/content_browser.gypi:1194: ontent/common/media/media_track_lifetime_metrics_messages. 'browser/speech/speech_recognizer_impl_android.h', error error https://codereview.chromium.org/183973021/diff/60001/content/renderer/media/r... File content/renderer/media/remote_media_stream_impl.cc (right): https://codereview.chromium.org/183973021/diff/60001/content/renderer/media/r... content/renderer/media/remote_media_stream_impl.cc:46: // A track might participate in different WebMediaStreams and will Can you please update the comment in metric_host.h as well to say what the id is in AddTrack. https://codereview.chromium.org/183973021/diff/60001/content/renderer/media/r... content/renderer/media/remote_media_stream_impl.cc:51: return track->stream().id().utf8() + " " + track->id().utf8(); oh- I did not now that there was such a back pointer. That is wrong and I am sure Tommy will remove it unless he just did yesterday. You can not have a back pointer since the same track can be in multiple streams now. I think you will have to provide the stream or the stream label to the RemoteMediaStreamTrackObserver when its created.
I'm working on a fix to use pointers as IDs, see below. Will ping again when I've uploaded that. https://codereview.chromium.org/183973021/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/media_stream_track_metrics_host.h (right): https://codereview.chromium.org/183973021/diff/60001/content/browser/renderer... content/browser/renderer_host/media/media_stream_track_metrics_host.h:5: #ifndef CONTENT_BROWSER_RENDERER_HOST_MEDIA_MEDIA_STREAM_TRACK_METRICS_HOST_H_ On 2014/03/12 07:12:40, perkj wrote: > nit : git mv is the reviewers friend I think.Or lowering the comparison bar when > uploading the file. I used git mv, indirectly. There are two problems: a) git mv is just git rm + mv + git add, and it does nothing to track moves. git only guesses at moves based on similarity of files. b) These are actually new files, from the source control system point of view. Rietveld does not try (AFAIK) to show you between-file deltas when files get moved to a new location between patch sets. I tried reuploading with --similarity=30, but it makes no difference, because of (b) I'm guessing. https://codereview.chromium.org/183973021/diff/60001/content/content_browser.... File content/content_browser.gypi (right): https://codereview.chromium.org/183973021/diff/60001/content/content_browser.... content/content_browser.gypi:1194: ontent/common/media/media_track_lifetime_metrics_messages. 'browser/speech/speech_recognizer_impl_android.h', On 2014/03/12 07:12:40, perkj wrote: > error error Ehemm :) Fixed. https://codereview.chromium.org/183973021/diff/60001/content/renderer/media/r... File content/renderer/media/remote_media_stream_impl.cc (right): https://codereview.chromium.org/183973021/diff/60001/content/renderer/media/r... content/renderer/media/remote_media_stream_impl.cc:46: // A track might participate in different WebMediaStreams and will On 2014/03/12 07:12:40, perkj wrote: > Can you please update the comment in metric_host.h as well to say what the id is > in AddTrack. Will do. https://codereview.chromium.org/183973021/diff/60001/content/renderer/media/r... content/renderer/media/remote_media_stream_impl.cc:51: return track->stream().id().utf8() + " " + track->id().utf8(); On 2014/03/12 07:12:40, perkj wrote: > oh- I did not now that there was such a back pointer. That is wrong and I am > sure Tommy will remove it unless he just did yesterday. You can not have a back > pointer since the same track can be in multiple streams now. I think you will > have to provide the stream or the stream label to the > RemoteMediaStreamTrackObserver when its created. Since all I really need is a unique identity per observer per renderer process (and note that the host on the browser side is per renderer process), I can simply use the value of the 'this' pointer. Will switch to that, it keeps things a lot simpler.
I am going to defer the review to Per :)
PTAL, pointer values now used as IDs.
looks good to me if you take care about the comment below. I think you can add tommi or some other owner. https://codereview.chromium.org/183973021/diff/100001/content/browser/rendere... File content/browser/renderer_host/media/media_stream_track_metrics_host.cc (right): https://codereview.chromium.org/183973021/diff/100001/content/browser/rendere... content/browser/renderer_host/media/media_stream_track_metrics_host.cc:81: ReportDuration(info.first, info.second); use |is_remote| ? or add a todo to do it later.
On 2014/03/12 10:21:34, perkj wrote: > looks good to me if you take care about the comment below. I think you can add > tommi or some other owner. > > owner stamp lgtm after Per is ok with the patch.
Thanks guys. There is already a DCHECK(is_remote) in the OnAddTrack implementation. Once I add sent tracks, I'll start using this parameter. Cheers, Jói On Wed, Mar 12, 2014 at 1:34 PM, <xians@chromium.org> wrote: > On 2014/03/12 10:21:34, perkj wrote: >> >> looks good to me if you take care about the comment below. I think you can >> add >> tommi or some other owner. > > > > > owner stamp lgtm after Per is ok with the patch. > > https://codereview.chromium.org/183973021/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
histograms lgtm
Per, I made a small change to only send IPC messages if RenderThreadImpl::current() is non-NULL, since it appears it is sometimes NULL in unit tests. In the process I also moved the sending to a virtual method that could be overridden. Could you take a quick look before I pull the commit trigger? Cheers, Jói
I made some further changes after manual testing showed me a couple of edge cases. Per, PTAL. On Wed, Mar 12, 2014 at 2:56 PM, <joi@chromium.org> wrote: > Per, I made a small change to only send IPC messages if > RenderThreadImpl::current() is non-NULL, since it appears it is sometimes > NULL > in unit tests. In the process I also moved the sending to a virtual method > that > could be overridden. > > Could you take a quick look before I pull the commit trigger? > > Cheers, > Jói > > > https://codereview.chromium.org/183973021/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Per, hold that review until I ping the thread again. Looks like there are some lifetime issues between RenderProcessHost and BrowserMessageFilters that it owns... but I think I know of a solution. On Wed, Mar 12, 2014 at 5:03 PM, Jói Sigurðsson <joi@chromium.org> wrote: > I made some further changes after manual testing showed me a couple of > edge cases. Per, PTAL. > > On Wed, Mar 12, 2014 at 2:56 PM, <joi@chromium.org> wrote: >> Per, I made a small change to only send IPC messages if >> RenderThreadImpl::current() is non-NULL, since it appears it is sometimes >> NULL >> in unit tests. In the process I also moved the sending to a virtual method >> that >> could be overridden. >> >> Could you take a quick look before I pull the commit trigger? >> >> Cheers, >> Jói >> >> >> https://codereview.chromium.org/183973021/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
OK, ready for a look now. On Wed, Mar 12, 2014 at 5:10 PM, Jói Sigurðsson <joi@chromium.org> wrote: > Per, hold that review until I ping the thread again. Looks like there > are some lifetime issues between RenderProcessHost and > BrowserMessageFilters that it owns... but I think I know of a > solution. > > On Wed, Mar 12, 2014 at 5:03 PM, Jói Sigurðsson <joi@chromium.org> wrote: >> I made some further changes after manual testing showed me a couple of >> edge cases. Per, PTAL. >> >> On Wed, Mar 12, 2014 at 2:56 PM, <joi@chromium.org> wrote: >>> Per, I made a small change to only send IPC messages if >>> RenderThreadImpl::current() is non-NULL, since it appears it is sometimes >>> NULL >>> in unit tests. In the process I also moved the sending to a virtual method >>> that >>> could be overridden. >>> >>> Could you take a quick look before I pull the commit trigger? >>> >>> Cheers, >>> Jói >>> >>> >>> https://codereview.chromium.org/183973021/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
+jschuh for IPC security review: content/common/media/media_stream_track_metrics_host_messages.h and ipc/ipc_message_start.h Cheers, Jói
lgtm on ipc security. (note to security: pod pairs in a map with insertion and removal messages)
lgtm
The CQ bit was checked by joi@chromium.org
The CQ bit was unchecked by joi@chromium.org
TBR=jochen@chromium.org for a trivial addition to RenderProcessHostImpl (one more BrowserMessageFilter).
The CQ bit was checked by joi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joi@chromium.org/183973021/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_clang_dbg
The CQ bit was checked by joi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joi@chromium.org/183973021/180001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_chromeos for step(s) app_list_unittests, ash_unittests, aura_unittests, base_unittests, browser_tests, cacheinvalidation_unittests, check_deps, check_deps2git, chromeos_unittests, components_unittests, content_browsertests, content_unittests, crypto_unittests, dbus_unittests, device_unittests, events_unittests, google_apis_unittests, gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, keyboard_unittests, media_unittests, net_unittests, ppapi_unittests, printing_unittests, sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
The CQ bit was checked by joi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joi@chromium.org/183973021/180001
Adding NOTRY=true as the errors seem to be the "max command line length" compile errors folks have been discussing. On Thu, Mar 13, 2014 at 4:35 AM, <commit-bot@chromium.org> wrote: > Retried try job too often on linux_chromeos for step(s) app_list_unittests, > ash_unittests, aura_unittests, base_unittests, browser_tests, > cacheinvalidation_unittests, check_deps, check_deps2git, chromeos_unittests, > components_unittests, content_browsertests, content_unittests, > crypto_unittests, > dbus_unittests, device_unittests, events_unittests, google_apis_unittests, > gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, > keyboard_unittests, media_unittests, net_unittests, ppapi_unittests, > printing_unittests, sandbox_linux_unittests, sql_unittests, sync_unit_tests, > unit_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... > > https://codereview.chromium.org/183973021/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Change committed as 256780 |
