Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
738193002:
Update BufferedDataSource class documentation. (Closed)
|
Created:
6 years, 1 month ago by no sievers Modified:
6 years, 1 month ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionUpdate BufferedDataSource class documentation.
This class must be deleted on the main thread.
BUG=356540
Committed: https://crrev.com/fc72523a689093962b4f7032747fa2e5346c6490
Cr-Commit-Position: refs/heads/master@{#304936}
Patch Set 1 #
Total comments: 6
Patch Set 2 : address comments #
Total comments: 2
Patch Set 3 : #Patch Set 4 : #
Messages
Total messages: 15 (2 generated)
sievers@chromium.org changed reviewers: + dalecurtis@chromium.org, wez@chromium.org
ptal
lgtm % update. Do all the tests pass after this? https://codereview.chromium.org/738193002/diff/1/media/blink/buffered_data_so... File media/blink/buffered_data_source.cc (right): https://codereview.chromium.org/738193002/diff/1/media/blink/buffered_data_so... media/blink/buffered_data_source.cc:105: DCHECK(host_); DCHECK constructed on render_task_runner_ too?
https://codereview.chromium.org/738193002/diff/1/media/blink/buffered_data_so... File media/blink/buffered_data_source.cc (right): https://codereview.chromium.org/738193002/diff/1/media/blink/buffered_data_so... media/blink/buffered_data_source.cc:105: DCHECK(host_); On 2014/11/19 21:14:10, DaleCurtis wrote: > DCHECK constructed on render_task_runner_ too? It's not required in terms of the WeakPtr factory, since the thread binding only happens when pointers are invalidated or dereferenced. So in that regard it's ok to create it on another thread as long as you then only post to it on the main thread.
https://codereview.chromium.org/738193002/diff/1/media/blink/buffered_data_so... File media/blink/buffered_data_source.cc (right): https://codereview.chromium.org/738193002/diff/1/media/blink/buffered_data_so... media/blink/buffered_data_source.cc:105: DCHECK(host_); On 2014/11/19 21:16:21, sievers wrote: > On 2014/11/19 21:14:10, DaleCurtis wrote: > > DCHECK constructed on render_task_runner_ too? > > It's not required in terms of the WeakPtr factory, since the thread binding only > happens when pointers are invalidated or dereferenced. > So in that regard it's ok to create it on another thread as long as you then > only post to it on the main thread. If you want to allow this object to be created on any thread, but then used & destroyed on the render thread then the class comment needs to say so. If there's no actual use for that property right now then I'd suggest the more restrictive create+destroy from render thread requirement be stipulated, and a DCHECK here. https://codereview.chromium.org/738193002/diff/1/media/blink/buffered_data_so... File media/blink/buffered_data_source.h (right): https://codereview.chromium.org/738193002/diff/1/media/blink/buffered_data_so... media/blink/buffered_data_source.h:47: // deleted on the main thread. It doesn't look like it actually exposes WeakPtrs; they seem to be an internal implementation detail, in which case I'd suggest simply stating that this class is designed to be created and destroyed from the render thread, although it may be accessed by other threads while alive. How are you making sure that the accesses from other threads are synchronized with this object's teardown, though? e.g. how do you know that SetBitrate or Stop won't be called on one thread while this object is being torn down, or after it has been torn down, on the render thread?
https://codereview.chromium.org/738193002/diff/1/media/blink/buffered_data_so... File media/blink/buffered_data_source.cc (right): https://codereview.chromium.org/738193002/diff/1/media/blink/buffered_data_so... media/blink/buffered_data_source.cc:105: DCHECK(host_); On 2014/11/19 21:38:55, Wez wrote: > On 2014/11/19 21:16:21, sievers wrote: > > On 2014/11/19 21:14:10, DaleCurtis wrote: > > > DCHECK constructed on render_task_runner_ too? > > > > It's not required in terms of the WeakPtr factory, since the thread binding > only > > happens when pointers are invalidated or dereferenced. > > So in that regard it's ok to create it on another thread as long as you then > > only post to it on the main thread. > > If you want to allow this object to be created on any thread, but then used & > destroyed on the render thread then the class comment needs to say so. > > If there's no actual use for that property right now then I'd suggest the more > restrictive create+destroy from render thread requirement be stipulated, and a > DCHECK here. Done. https://codereview.chromium.org/738193002/diff/1/media/blink/buffered_data_so... File media/blink/buffered_data_source.h (right): https://codereview.chromium.org/738193002/diff/1/media/blink/buffered_data_so... media/blink/buffered_data_source.h:47: // deleted on the main thread. On 2014/11/19 21:38:55, Wez wrote: > It doesn't look like it actually exposes WeakPtrs; they seem to be an internal > implementation detail, in which case I'd suggest simply stating that this class > is designed to be created and destroyed from the render thread, although it may > be accessed by other threads while alive. > Done.
https://codereview.chromium.org/738193002/diff/20001/media/blink/buffered_dat... File media/blink/buffered_data_source.h (right): https://codereview.chromium.org/738193002/diff/20001/media/blink/buffered_dat... media/blink/buffered_data_source.h:45: // BufferedDataSource must be created and destroyed on the main thread. "main thread" doesn't have any meaning here, does it? I think you mean "on the supplied |task_runner|" - if so then you can actually get rid of the |task_runner| parameter and use ThreadTaskRunnerHandle::Get() to retrieve it in the ctor.
https://codereview.chromium.org/738193002/diff/20001/media/blink/buffered_dat... File media/blink/buffered_data_source.h (right): https://codereview.chromium.org/738193002/diff/20001/media/blink/buffered_dat... media/blink/buffered_data_source.h:45: // BufferedDataSource must be created and destroyed on the main thread. On 2014/11/19 21:45:13, Wez wrote: > "main thread" doesn't have any meaning here, does it? > > I think you mean "on the supplied |task_runner|" - if so then you can actually > get rid of the |task_runner| parameter and use ThreadTaskRunnerHandle::Get() to > retrieve it in the ctor. buffered_data_source_unittest.cc passes in another message loop.
On 2014/11/19 21:52:29, sievers wrote: > https://codereview.chromium.org/738193002/diff/20001/media/blink/buffered_dat... > File media/blink/buffered_data_source.h (right): > > https://codereview.chromium.org/738193002/diff/20001/media/blink/buffered_dat... > media/blink/buffered_data_source.h:45: // BufferedDataSource must be created and > destroyed on the main thread. > On 2014/11/19 21:45:13, Wez wrote: > > "main thread" doesn't have any meaning here, does it? > > > > I think you mean "on the supplied |task_runner|" - if so then you can actually > > get rid of the |task_runner| parameter and use ThreadTaskRunnerHandle::Get() > to > > retrieve it in the ctor. > > buffered_data_source_unittest.cc passes in another message loop. Right, but presumably the same requirement applies, that that is the message loop on which the object is created & destroyed. (That's what your new DCHECKs enforce).
On 2014/11/19 22:16:40, Wez wrote: > On 2014/11/19 21:52:29, sievers wrote: > > > https://codereview.chromium.org/738193002/diff/20001/media/blink/buffered_dat... > > File media/blink/buffered_data_source.h (right): > > > > > https://codereview.chromium.org/738193002/diff/20001/media/blink/buffered_dat... > > media/blink/buffered_data_source.h:45: // BufferedDataSource must be created > and > > destroyed on the main thread. > > On 2014/11/19 21:45:13, Wez wrote: > > > "main thread" doesn't have any meaning here, does it? > > > > > > I think you mean "on the supplied |task_runner|" - if so then you can > actually > > > get rid of the |task_runner| parameter and use ThreadTaskRunnerHandle::Get() > > to > > > retrieve it in the ctor. > > > > buffered_data_source_unittest.cc passes in another message loop. > > Right, but presumably the same requirement applies, that that is the message > loop on which the object is created & destroyed. (That's what your new DCHECKs > enforce). Comment updated.
lgtm
The CQ bit was checked by sievers@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/738193002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/fc72523a689093962b4f7032747fa2e5346c6490 Cr-Commit-Position: refs/heads/master@{#304936} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
