Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
314113008:
Oilpan: have MediaController weakly refer to its media elements. (Closed)
|
Created:
6 years, 6 months ago by sof Modified:
6 years, 6 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, philipj_slow, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionOilpan: have MediaController weakly refer to its media elements.
Simplify finalization of 'slaved' media elements by not requiring them
to unregister with their MediaController, but have it (the controller)
weakly refer to them instead.
Previously, a RefPtr<> was kept on the media element to make that
unregistration safe during finalization. This is no longer needed, and
a simpler Member reference from the media element is used instead.
R=haraken@chromium.org,ager@chromium.org,acolwell@chromium.org
BUG=357163
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175930
Patch Set 1 #
Total comments: 5
Patch Set 2 : Use add() #
Total comments: 3
Patch Set 3 : Add FIXME (crbug.com/383072) #
Messages
Total messages: 19 (0 generated)
Please take a look. Just carrying over the MediaController change made in https://codereview.chromium.org/302093011/
LGTM https://codereview.chromium.org/314113008/diff/1/Source/core/html/MediaContro... File Source/core/html/MediaController.h (right): https://codereview.chromium.org/314113008/diff/1/Source/core/html/MediaContro... Source/core/html/MediaController.h:111: typedef WillBeHeapLinkedHashSet<RawPtrWillBeWeakMember<HTMLMediaElement> > MediaElementSequence; I'm not sure if the order matters, so probably you can just use a HeapHashSet. However, it would be better to keep the current behavior by using a HeapLinkedHashSet.
https://codereview.chromium.org/314113008/diff/1/Source/core/html/MediaContro... File Source/core/html/MediaController.h (right): https://codereview.chromium.org/314113008/diff/1/Source/core/html/MediaContro... Source/core/html/MediaController.h:111: typedef WillBeHeapLinkedHashSet<RawPtrWillBeWeakMember<HTMLMediaElement> > MediaElementSequence; On 2014/06/06 07:27:21, haraken wrote: > > I'm not sure if the order matters, so probably you can just use a HeapHashSet. > However, it would be better to keep the current behavior by using a > HeapLinkedHashSet. "they must be processed in the order that they were last associated with the MediaController." says the spec, so I don't think that would be correct. (http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element... )
https://codereview.chromium.org/314113008/diff/1/Source/core/html/MediaContro... File Source/core/html/MediaController.h (right): https://codereview.chromium.org/314113008/diff/1/Source/core/html/MediaContro... Source/core/html/MediaController.h:111: typedef WillBeHeapLinkedHashSet<RawPtrWillBeWeakMember<HTMLMediaElement> > MediaElementSequence; On 2014/06/06 07:32:45, sof wrote: > On 2014/06/06 07:27:21, haraken wrote: > > > > I'm not sure if the order matters, so probably you can just use a HeapHashSet. > > However, it would be better to keep the current behavior by using a > > HeapLinkedHashSet. > > "they must be processed in the order that they were last associated with the > MediaController." says the spec, so I don't think that would be correct. > > (http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element... > ) oh, thanks!
LGTM This keep the current behavior. I can't really figure out if this could have been a strong relationship though. Any thoughts on that? Is there a case where it is important that an HTMLMediaElement is allowed to die independently from the MediaController it is slaved on? https://codereview.chromium.org/314113008/diff/1/Source/core/html/MediaContro... File Source/core/html/MediaController.cpp (right): https://codereview.chromium.org/314113008/diff/1/Source/core/html/MediaContro... Source/core/html/MediaController.cpp:77: m_mediaElements.appendOrMoveToLast(element); You can just use add here. There is an assert above that it is not contained.
On 2014/06/06 08:49:08, Mads Ager (chromium) wrote: > LGTM > > This keep the current behavior. I can't really figure out if this could have > been a strong relationship though. Any thoughts on that? Is there a case where > it is important that an HTMLMediaElement is allowed to die independently from > the MediaController it is slaved on? > It's a good question; I can't locate anything either on whether a slaved element must be kept alive until the controller has reached a specific state or not.
While https://codereview.chromium.org/302093011/#msg21 sounded positive wrt this change, I will wait for acolwell's approval/feedback here. https://codereview.chromium.org/314113008/diff/1/Source/core/html/MediaContro... File Source/core/html/MediaController.cpp (right): https://codereview.chromium.org/314113008/diff/1/Source/core/html/MediaContro... Source/core/html/MediaController.cpp:77: m_mediaElements.appendOrMoveToLast(element); On 2014/06/06 08:49:08, Mads Ager (chromium) wrote: > You can just use add here. There is an assert above that it is not contained. Done.
looks good. Just one question. https://codereview.chromium.org/314113008/diff/20001/Source/core/html/MediaCo... File Source/core/html/MediaController.h (right): https://codereview.chromium.org/314113008/diff/20001/Source/core/html/MediaCo... Source/core/html/MediaController.h:111: typedef WillBeHeapLinkedHashSet<RawPtrWillBeWeakMember<HTMLMediaElement> > MediaElementSequence; I'm a little fuzzy on these Oilpan types. If you use this, does this mean that media elements can disappear from this list if the JavaScript application doesn't have any references to the elements and they aren't in the document, but JavaScript DOES have a reference to this controller? If so, then I think that might be bad. Say I have several audio elements I want to control with the same controller, but don't really need them in the document. Conceptually I should be able to just hold onto the controller and still control the media elements eventhough they have gone out of scope. I think the web developer might be surprised if these elements disappear.
https://codereview.chromium.org/314113008/diff/20001/Source/core/html/MediaCo... File Source/core/html/MediaController.h (right): https://codereview.chromium.org/314113008/diff/20001/Source/core/html/MediaCo... Source/core/html/MediaController.h:111: typedef WillBeHeapLinkedHashSet<RawPtrWillBeWeakMember<HTMLMediaElement> > MediaElementSequence; > I'm a little fuzzy on these Oilpan types. If you use this, does this mean that > media elements can disappear from this list if the JavaScript application > doesn't have any references to the elements and they aren't in the document, but > JavaScript DOES have a reference to this controller? If so, then I think that > might be bad. > > Say I have several audio elements I want to control with the same controller, > but don't really need them in the document. Conceptually I should be able to > just hold onto the controller and still control the media elements eventhough > they have gone out of scope. I think the web developer might be surprised if > these elements disappear. Your understanding is correct. If you want to keep the media elements alive while the controller is alive, we can use HeapLinkedHashSet<Member<HTMLMediaElement>>. In fact, we _do_ want to do that because we don't want to use weak members as much as possible. However, as far as I understand the current code, the media elements are not kept alive while the controller is alive, so we used HeapLinkedHashSet<WeakMember<HTMLMediaElement>> to keep the current behavior. In short, if you're happy with keeping the media elements alive while the controller is alive, we're very happy to make the change :)
https://codereview.chromium.org/314113008/diff/20001/Source/core/html/MediaCo... File Source/core/html/MediaController.h (right): https://codereview.chromium.org/314113008/diff/20001/Source/core/html/MediaCo... Source/core/html/MediaController.h:111: typedef WillBeHeapLinkedHashSet<RawPtrWillBeWeakMember<HTMLMediaElement> > MediaElementSequence; On 2014/06/07 00:04:50, acolwell wrote: > I'm a little fuzzy on these Oilpan types. If you use this, does this mean that > media elements can disappear from this list if the JavaScript application > doesn't have any references to the elements and they aren't in the document, but > JavaScript DOES have a reference to this controller? If so, then I think that > might be bad. > > Say I have several audio elements I want to control with the same controller, > but don't really need them in the document. Conceptually I should be able to > just hold onto the controller and still control the media elements eventhough > they have gone out of scope. I think the web developer might be surprised if > these elements disappear. > Comments #5 and #6 touch on what the spec promises to be the behavior -- we're not able to locate much on a slaved element's guaranteed lifetime (other than the lifetime of such an audio element is mentioned elsewhere as needing to stay around when detached from the document, if still playing.) Might it be better to wait with doing this change until/if Oilpan is the default? Otherwise you have behavioral differences across the setting of enable_oilpan.
On 2014/06/07 05:33:30, sof wrote: > https://codereview.chromium.org/314113008/diff/20001/Source/core/html/MediaCo... > File Source/core/html/MediaController.h (right): > > https://codereview.chromium.org/314113008/diff/20001/Source/core/html/MediaCo... > Source/core/html/MediaController.h:111: typedef > WillBeHeapLinkedHashSet<RawPtrWillBeWeakMember<HTMLMediaElement> > > MediaElementSequence; > On 2014/06/07 00:04:50, acolwell wrote: > > I'm a little fuzzy on these Oilpan types. If you use this, does this mean that > > media elements can disappear from this list if the JavaScript application > > doesn't have any references to the elements and they aren't in the document, > but > > JavaScript DOES have a reference to this controller? If so, then I think that > > might be bad. > > > > Say I have several audio elements I want to control with the same controller, > > but don't really need them in the document. Conceptually I should be able to > > just hold onto the controller and still control the media elements eventhough > > they have gone out of scope. I think the web developer might be surprised if > > these elements disappear. > > > > Comments #5 and #6 touch on what the spec promises to be the behavior -- we're > not able to locate much on a slaved element's guaranteed lifetime (other than > the lifetime of such an audio element is mentioned elsewhere as needing to stay > around when detached from the document, if still playing.) > > Might it be better to wait with doing this change until/if Oilpan is the > default? Otherwise you have behavioral differences across the setting of > enable_oilpan. Defer (and add a FIXME for now), or address here?
On 2014/06/09 20:00:30, sof wrote: > On 2014/06/07 05:33:30, sof wrote: > > > https://codereview.chromium.org/314113008/diff/20001/Source/core/html/MediaCo... > > File Source/core/html/MediaController.h (right): > > > > > https://codereview.chromium.org/314113008/diff/20001/Source/core/html/MediaCo... > > Source/core/html/MediaController.h:111: typedef > > WillBeHeapLinkedHashSet<RawPtrWillBeWeakMember<HTMLMediaElement> > > > MediaElementSequence; > > On 2014/06/07 00:04:50, acolwell wrote: > > > I'm a little fuzzy on these Oilpan types. If you use this, does this mean > that > > > media elements can disappear from this list if the JavaScript application > > > doesn't have any references to the elements and they aren't in the document, > > but > > > JavaScript DOES have a reference to this controller? If so, then I think > that > > > might be bad. > > > > > > Say I have several audio elements I want to control with the same > controller, > > > but don't really need them in the document. Conceptually I should be able to > > > just hold onto the controller and still control the media elements > eventhough > > > they have gone out of scope. I think the web developer might be surprised if > > > these elements disappear. > > > > > > > Comments #5 and #6 touch on what the spec promises to be the behavior -- we're > > not able to locate much on a slaved element's guaranteed lifetime (other than > > the lifetime of such an audio element is mentioned elsewhere as needing to > stay > > around when detached from the document, if still playing.) > > > > Might it be better to wait with doing this change until/if Oilpan is the > > default? Otherwise you have behavioral differences across the setting of > > enable_oilpan. > > Defer (and add a FIXME for now), or address here? I think it's OK to land this CL with a FIXME and address the issue in a separate CL.
On 2014/06/09 23:14:14, haraken wrote: > On 2014/06/09 20:00:30, sof wrote: > > On 2014/06/07 05:33:30, sof wrote: > > > > > > https://codereview.chromium.org/314113008/diff/20001/Source/core/html/MediaCo... > > > File Source/core/html/MediaController.h (right): > > > > > > > > > https://codereview.chromium.org/314113008/diff/20001/Source/core/html/MediaCo... > > > Source/core/html/MediaController.h:111: typedef > > > WillBeHeapLinkedHashSet<RawPtrWillBeWeakMember<HTMLMediaElement> > > > > MediaElementSequence; > > > On 2014/06/07 00:04:50, acolwell wrote: > > > > I'm a little fuzzy on these Oilpan types. If you use this, does this mean > > that > > > > media elements can disappear from this list if the JavaScript application > > > > doesn't have any references to the elements and they aren't in the > document, > > > but > > > > JavaScript DOES have a reference to this controller? If so, then I think > > that > > > > might be bad. > > > > > > > > Say I have several audio elements I want to control with the same > > controller, > > > > but don't really need them in the document. Conceptually I should be able > to > > > > just hold onto the controller and still control the media elements > > eventhough > > > > they have gone out of scope. I think the web developer might be surprised > if > > > > these elements disappear. > > > > > > > > > > Comments #5 and #6 touch on what the spec promises to be the behavior -- > we're > > > not able to locate much on a slaved element's guaranteed lifetime (other > than > > > the lifetime of such an audio element is mentioned elsewhere as needing to > > stay > > > around when detached from the document, if still playing.) > > > > > > Might it be better to wait with doing this change until/if Oilpan is the > > > default? Otherwise you have behavioral differences across the setting of > > > enable_oilpan. > > > > Defer (and add a FIXME for now), or address here? > > I think it's OK to land this CL with a FIXME and address the issue in a separate > CL. I agree with that. This change keeps the current behavior. Let's add a FIXME and file a bug report so we remember to revisit it?
On 2014/06/10 06:12:04, Mads Ager (chromium) wrote: > On 2014/06/09 23:14:14, haraken wrote: > > On 2014/06/09 20:00:30, sof wrote: > > > On 2014/06/07 05:33:30, sof wrote: > > > > > > > > > > https://codereview.chromium.org/314113008/diff/20001/Source/core/html/MediaCo... > > > > File Source/core/html/MediaController.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/314113008/diff/20001/Source/core/html/MediaCo... > > > > Source/core/html/MediaController.h:111: typedef > > > > WillBeHeapLinkedHashSet<RawPtrWillBeWeakMember<HTMLMediaElement> > > > > > MediaElementSequence; > > > > On 2014/06/07 00:04:50, acolwell wrote: > > > > > I'm a little fuzzy on these Oilpan types. If you use this, does this > mean > > > that > > > > > media elements can disappear from this list if the JavaScript > application > > > > > doesn't have any references to the elements and they aren't in the > > document, > > > > but > > > > > JavaScript DOES have a reference to this controller? If so, then I think > > > that > > > > > might be bad. > > > > > > > > > > Say I have several audio elements I want to control with the same > > > controller, > > > > > but don't really need them in the document. Conceptually I should be > able > > to > > > > > just hold onto the controller and still control the media elements > > > eventhough > > > > > they have gone out of scope. I think the web developer might be > surprised > > if > > > > > these elements disappear. > > > > > > > > > > > > > Comments #5 and #6 touch on what the spec promises to be the behavior -- > > we're > > > > not able to locate much on a slaved element's guaranteed lifetime (other > > than > > > > the lifetime of such an audio element is mentioned elsewhere as needing to > > > stay > > > > around when detached from the document, if still playing.) > > > > > > > > Might it be better to wait with doing this change until/if Oilpan is the > > > > default? Otherwise you have behavioral differences across the setting of > > > > enable_oilpan. > > > > > > Defer (and add a FIXME for now), or address here? > > > > I think it's OK to land this CL with a FIXME and address the issue in a > separate > > CL. > > I agree with that. This change keeps the current behavior. Let's add a FIXME and > file a bug report so we remember to revisit it? Let's do that, but I will still wait for approval from media folks about this change before going ahead.
lgtm
On 2014/06/10 14:58:31, acolwell wrote: > lgtm Thanks all for the reviews. Created http://crbug.com/383072 + added a FIXME.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/314113008/40001
Message was sent while issue was closed.
Change committed as 175930 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
