Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
946503003:
Add HTMLMediaElement.waitingforkey event (Closed)
|
Created:
5 years, 10 months ago by jrummell Modified:
5 years, 10 months ago CC:
blink-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, philipj_slow Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAdd HTMLMediaElement.waitingforkey event
Spec: https://w3c.github.io/encrypted-media/#queue-waitingforkey
BUG=337975
TEST=none yet, need Chromium changes to call it.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190611
Patch Set 1 #
Total comments: 6
Patch Set 2 : rename #
Total comments: 3
Patch Set 3 : rename again #
Messages
Total messages: 20 (4 generated)
jrummell@chromium.org changed reviewers: + ddorwin@chromium.org, sandersd@chromium.org
PTAL.
Really just a naming question. LG other than that. https://codereview.chromium.org/946503003/diff/1/Source/modules/encryptedmedi... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h (right): https://codereview.chromium.org/946503003/diff/1/Source/modules/encryptedmedi... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:43: DEFINE_STATIC_ATTRIBUTE_EVENT_LISTENER(waitingforkey); There isn't an attribute for this event. https://codereview.chromium.org/946503003/diff/1/Source/modules/encryptedmedi... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:50: static void playbackResumedAfterKey(HTMLMediaElement&); "AfterKey" sounds weird. noLongerWaitingForKey? It may be that we got the KeyWaitingFor, but this may also be called on a seek or something I imagine. Of course, maybe Blink should be handling that logic. https://codereview.chromium.org/946503003/diff/1/Source/modules/encryptedmedi... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:85: bool m_waitingForKey; m_isW...
jrummell@chromium.org changed reviewers: + dcheng@chromium.org
Updated. +dcheng@ for OWNERS review. https://codereview.chromium.org/946503003/diff/1/Source/modules/encryptedmedi... File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h (right): https://codereview.chromium.org/946503003/diff/1/Source/modules/encryptedmedi... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:43: DEFINE_STATIC_ATTRIBUTE_EVENT_LISTENER(waitingforkey); On 2015/02/20 04:45:28, ddorwin wrote: > There isn't an attribute for this event. Done. https://codereview.chromium.org/946503003/diff/1/Source/modules/encryptedmedi... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:50: static void playbackResumedAfterKey(HTMLMediaElement&); On 2015/02/20 04:45:28, ddorwin wrote: > "AfterKey" sounds weird. > > noLongerWaitingForKey? > > It may be that we got the KeyWaitingFor, but this may also be called on a seek > or something I imagine. Of course, maybe Blink should be handling that logic. Done. https://codereview.chromium.org/946503003/diff/1/Source/modules/encryptedmedi... Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.h:85: bool m_waitingForKey; On 2015/02/20 04:45:28, ddorwin wrote: > m_isW... Done.
https://codereview.chromium.org/946503003/diff/20001/public/platform/WebMedia... File public/platform/WebMediaPlayerClient.h (right): https://codereview.chromium.org/946503003/diff/20001/public/platform/WebMedia... public/platform/WebMediaPlayerClient.h:86: virtual void noLongerWaitingForKey() = 0; I feel like these could be better named. In the past, we've tended to use verbs for method names. It seems WebMediaPlayerClient has diverged from this convention somewhat, but I think it reads a bit more clearly. Maybe something like: WebMediaPlayerClient::waitForKey() WebMediaPlayerClient::didGetKey() or something else (I'm sure someone can come up with better names). The reason I didn't just use the spec verbs is because (to me) it's not clear that wait for key and resume playback are immediately related.
https://codereview.chromium.org/946503003/diff/20001/public/platform/WebMedia... File public/platform/WebMediaPlayerClient.h (right): https://codereview.chromium.org/946503003/diff/20001/public/platform/WebMedia... public/platform/WebMediaPlayerClient.h:86: virtual void noLongerWaitingForKey() = 0; On 2015/02/20 18:19:20, dcheng wrote: > I feel like these could be better named. In the past, we've tended to use verbs > for method names. It seems WebMediaPlayerClient has diverged from this > convention somewhat, but I think it reads a bit more clearly. > > Maybe something like: > WebMediaPlayerClient::waitForKey() > WebMediaPlayerClient::didGetKey() > > or something else (I'm sure someone can come up with better names). The reason I > didn't just use the spec verbs is because (to me) it's not clear that wait for > key and resume playback are immediately related. I'm okay with waitingForKey(), because like the other methods here it is named after the associated event. noLongerWaitingForKey() is harder to justify, but at least it's clear in context.
On 2015/02/20 at 20:17:04, sandersd wrote: > https://codereview.chromium.org/946503003/diff/20001/public/platform/WebMedia... > File public/platform/WebMediaPlayerClient.h (right): > > https://codereview.chromium.org/946503003/diff/20001/public/platform/WebMedia... > public/platform/WebMediaPlayerClient.h:86: virtual void noLongerWaitingForKey() = 0; > On 2015/02/20 18:19:20, dcheng wrote: > > I feel like these could be better named. In the past, we've tended to use verbs > > for method names. It seems WebMediaPlayerClient has diverged from this > > convention somewhat, but I think it reads a bit more clearly. > > > > Maybe something like: > > WebMediaPlayerClient::waitForKey() > > WebMediaPlayerClient::didGetKey() > > > > or something else (I'm sure someone can come up with better names). The reason I > > didn't just use the spec verbs is because (to me) it's not clear that wait for > > key and resume playback are immediately related. > > I'm okay with waitingForKey(), because like the other methods here it is named after the associated event. As previously mentioned, we've been a bit lax about this convention. That said, I don't think existing violations of the convention should be used to justify continuing this trend =) > noLongerWaitingForKey() is harder to justify, but at least it's clear in context. I don't agree. Some things are named after the associated event, some aren't. Half of the methods are verbs (repaint, setWebLayer, requestFullscreen), but some are nouns (encrypted, keyError, keyMessage). It's hard for someone who's less familiar with the code to understand exactly what's going on. One might expect noLongerWaitingForKey to fire some sort of event too, but obviously it doesn't. Is this called only once we successfully get a key? Or is it called in other situations? etc. These are all questions a more descriptive name could help with. At a bare minimum, something like: WebMediaPlayerClient::willWaitForKey() WebMediaPlayerClient::didGetKey() would better match the conventions of the Blink API. But there might be an even better name.
lgtm https://codereview.chromium.org/946503003/diff/20001/public/platform/WebMedia... File public/platform/WebMediaPlayerClient.h (right): https://codereview.chromium.org/946503003/diff/20001/public/platform/WebMedia... public/platform/WebMediaPlayerClient.h:86: virtual void noLongerWaitingForKey() = 0; On 2015/02/20 20:17:04, sandersd wrote: > On 2015/02/20 18:19:20, dcheng wrote: > > I feel like these could be better named. In the past, we've tended to use > verbs > > for method names. It seems WebMediaPlayerClient has diverged from this > > convention somewhat, but I think it reads a bit more clearly. > > > > Maybe something like: > > WebMediaPlayerClient::waitForKey() > > WebMediaPlayerClient::didGetKey() > > > > or something else (I'm sure someone can come up with better names). The reason > I > > didn't just use the spec verbs is because (to me) it's not clear that wait for > > key and resume playback are immediately related. > > I'm okay with waitingForKey(), because like the other methods here it is named > after the associated event. noLongerWaitingForKey() is harder to justify, but at > least it's clear in context. It's definitely not "waitForKey()" because Blink doesn't do any waiting. These should probably be onFoo or fireFoo, but consistency with the other methods is higher priority. noLongerWaitingForKey() is odd, but I can't think of another name that doesn't expose implementation (i.e. clearWaitingForKeyFlag()).
On 2015/02/20 21:01:06, dcheng wrote: > On 2015/02/20 at 20:17:04, sandersd wrote: > > > https://codereview.chromium.org/946503003/diff/20001/public/platform/WebMedia... > > File public/platform/WebMediaPlayerClient.h (right): > > > > > https://codereview.chromium.org/946503003/diff/20001/public/platform/WebMedia... > > public/platform/WebMediaPlayerClient.h:86: virtual void > noLongerWaitingForKey() = 0; > > On 2015/02/20 18:19:20, dcheng wrote: > > > I feel like these could be better named. In the past, we've tended to use > verbs > > > for method names. It seems WebMediaPlayerClient has diverged from this > > > convention somewhat, but I think it reads a bit more clearly. > > > > > > Maybe something like: > > > WebMediaPlayerClient::waitForKey() > > > WebMediaPlayerClient::didGetKey() > > > > > > or something else (I'm sure someone can come up with better names). The > reason I > > > didn't just use the spec verbs is because (to me) it's not clear that wait > for > > > key and resume playback are immediately related. > > > > I'm okay with waitingForKey(), because like the other methods here it is named > after the associated event. > > As previously mentioned, we've been a bit lax about this convention. That said, > I don't think existing violations of the convention should be used to justify > continuing this trend =) > > > noLongerWaitingForKey() is harder to justify, but at least it's clear in > context. > > I don't agree. Some things are named after the associated event, some aren't. > Half of the methods are verbs (repaint, setWebLayer, requestFullscreen), but > some are nouns (encrypted, keyError, keyMessage). It's hard for someone who's > less familiar with the code to understand exactly what's going on. One might > expect noLongerWaitingForKey to fire some sort of event too, but obviously it > doesn't. Is this called only once we successfully get a key? Or is it called in > other situations? etc. These are all questions a more descriptive name could > help with. > > At a bare minimum, something like: > WebMediaPlayerClient::willWaitForKey() > WebMediaPlayerClient::didGetKey() > would better match the conventions of the Blink API. But there might be an even > better name. The first is *currently* waiting, not will or did. The current name seems fine and consistent with the other event methods. "encrypted", "keyerror", and "keymessage" are all event names. As I mentioned, these should probably be "on" or something like that, but we don't have that convention. The second method could be "didResumePlayback[BlockedByKey]" or something like that. This tells you exactly when to call it.
On 2015/02/20 at 21:11:24, ddorwin wrote: > On 2015/02/20 21:01:06, dcheng wrote: > > On 2015/02/20 at 20:17:04, sandersd wrote: > > > > > https://codereview.chromium.org/946503003/diff/20001/public/platform/WebMedia... > > > File public/platform/WebMediaPlayerClient.h (right): > > > > > > > > https://codereview.chromium.org/946503003/diff/20001/public/platform/WebMedia... > > > public/platform/WebMediaPlayerClient.h:86: virtual void > > noLongerWaitingForKey() = 0; > > > On 2015/02/20 18:19:20, dcheng wrote: > > > > I feel like these could be better named. In the past, we've tended to use > > verbs > > > > for method names. It seems WebMediaPlayerClient has diverged from this > > > > convention somewhat, but I think it reads a bit more clearly. > > > > > > > > Maybe something like: > > > > WebMediaPlayerClient::waitForKey() > > > > WebMediaPlayerClient::didGetKey() > > > > > > > > or something else (I'm sure someone can come up with better names). The > > reason I > > > > didn't just use the spec verbs is because (to me) it's not clear that wait > > for > > > > key and resume playback are immediately related. > > > > > > I'm okay with waitingForKey(), because like the other methods here it is named > > after the associated event. > > > > As previously mentioned, we've been a bit lax about this convention. That said, > > I don't think existing violations of the convention should be used to justify > > continuing this trend =) > > > > > noLongerWaitingForKey() is harder to justify, but at least it's clear in > > context. > > > > I don't agree. Some things are named after the associated event, some aren't. > > Half of the methods are verbs (repaint, setWebLayer, requestFullscreen), but > > some are nouns (encrypted, keyError, keyMessage). It's hard for someone who's > > less familiar with the code to understand exactly what's going on. One might > > expect noLongerWaitingForKey to fire some sort of event too, but obviously it > > doesn't. Is this called only once we successfully get a key? Or is it called in > > other situations? etc. These are all questions a more descriptive name could > > help with. > > > > At a bare minimum, something like: > > WebMediaPlayerClient::willWaitForKey() > > WebMediaPlayerClient::didGetKey() > > would better match the conventions of the Blink API. But there might be an even > > better name. > > The first is *currently* waiting, not will or did. didPausePlaybackForKey? I don't know exactly what the right name should be, other than the current name is not OK. > The current name seems fine and consistent with the other event methods. "encrypted", "keyerror", and "keymessage" are all event names. As I mentioned, these should probably be "on" or something like that, but we don't have that convention. Again, I'm not seeking consistency within WebMediaPlayerClient. I'm looking for consistency with the rest of the Blink API. Blink doesn't really have a strong convention of onFoo. The benefit isn't just consistency though--it also helps people looking at the Blink API get a sense of when/why things are called, by having more descriptive names. > > The second method could be "didResumePlayback[BlockedByKey]" or something like that. This tells you exactly when to call it. This seems fine.
On 2015/02/20 21:21:02, dcheng wrote: > On 2015/02/20 at 21:11:24, ddorwin wrote: > > On 2015/02/20 21:01:06, dcheng wrote: > > > On 2015/02/20 at 20:17:04, sandersd wrote: > > > > > > > > https://codereview.chromium.org/946503003/diff/20001/public/platform/WebMedia... > > > > File public/platform/WebMediaPlayerClient.h (right): > > > > > > > > > > > > https://codereview.chromium.org/946503003/diff/20001/public/platform/WebMedia... > > > > public/platform/WebMediaPlayerClient.h:86: virtual void > > > noLongerWaitingForKey() = 0; > > > > On 2015/02/20 18:19:20, dcheng wrote: > > > > > I feel like these could be better named. In the past, we've tended to > use > > > verbs > > > > > for method names. It seems WebMediaPlayerClient has diverged from this > > > > > convention somewhat, but I think it reads a bit more clearly. > > > > > > > > > > Maybe something like: > > > > > WebMediaPlayerClient::waitForKey() > > > > > WebMediaPlayerClient::didGetKey() > > > > > > > > > > or something else (I'm sure someone can come up with better names). The > > > reason I > > > > > didn't just use the spec verbs is because (to me) it's not clear that > wait > > > for > > > > > key and resume playback are immediately related. > > > > > > > > I'm okay with waitingForKey(), because like the other methods here it is > named > > > after the associated event. > > > > > > As previously mentioned, we've been a bit lax about this convention. That > said, > > > I don't think existing violations of the convention should be used to > justify > > > continuing this trend =) > > > > > > > noLongerWaitingForKey() is harder to justify, but at least it's clear in > > > context. > > > > > > I don't agree. Some things are named after the associated event, some > aren't. > > > Half of the methods are verbs (repaint, setWebLayer, requestFullscreen), but > > > some are nouns (encrypted, keyError, keyMessage). It's hard for someone > who's > > > less familiar with the code to understand exactly what's going on. One might > > > expect noLongerWaitingForKey to fire some sort of event too, but obviously > it > > > doesn't. Is this called only once we successfully get a key? Or is it called > in > > > other situations? etc. These are all questions a more descriptive name could > > > help with. > > > > > > At a bare minimum, something like: > > > WebMediaPlayerClient::willWaitForKey() > > > WebMediaPlayerClient::didGetKey() > > > would better match the conventions of the Blink API. But there might be an > even > > > better name. > > > > The first is *currently* waiting, not will or did. > > didPausePlaybackForKey? Pause is a specific state. How about didBlockPlayback[Waiting]ForKey or didBlockPlaybackForNeededKey? Then the other one would be didResumePlaybackBlockedFor[Needed]Key. > > I don't know exactly what the right name should be, other than the current name > is not OK. > > > The current name seems fine and consistent with the other event methods. > "encrypted", "keyerror", and "keymessage" are all event names. As I mentioned, > these should probably be "on" or something like that, but we don't have that > convention. > > Again, I'm not seeking consistency within WebMediaPlayerClient. I'm looking for > consistency with the rest of the Blink API. Blink doesn't really have a strong > convention of onFoo. The benefit isn't just consistency though--it also helps > people looking at the Blink API get a sense of when/why things are called, by > having more descriptive names. > > > > > The second method could be "didResumePlayback[BlockedByKey]" or something like > that. This tells you exactly when to call it. > > This seems fine.
On 2015/02/20 at 21:33:56, ddorwin wrote: > On 2015/02/20 21:21:02, dcheng wrote: > > On 2015/02/20 at 21:11:24, ddorwin wrote: > > > On 2015/02/20 21:01:06, dcheng wrote: > > > > On 2015/02/20 at 20:17:04, sandersd wrote: > > > > > > > > > > > https://codereview.chromium.org/946503003/diff/20001/public/platform/WebMedia... > > > > > File public/platform/WebMediaPlayerClient.h (right): > > > > > > > > > > > > > > > > https://codereview.chromium.org/946503003/diff/20001/public/platform/WebMedia... > > > > > public/platform/WebMediaPlayerClient.h:86: virtual void > > > > noLongerWaitingForKey() = 0; > > > > > On 2015/02/20 18:19:20, dcheng wrote: > > > > > > I feel like these could be better named. In the past, we've tended to > > use > > > > verbs > > > > > > for method names. It seems WebMediaPlayerClient has diverged from this > > > > > > convention somewhat, but I think it reads a bit more clearly. > > > > > > > > > > > > Maybe something like: > > > > > > WebMediaPlayerClient::waitForKey() > > > > > > WebMediaPlayerClient::didGetKey() > > > > > > > > > > > > or something else (I'm sure someone can come up with better names). The > > > > reason I > > > > > > didn't just use the spec verbs is because (to me) it's not clear that > > wait > > > > for > > > > > > key and resume playback are immediately related. > > > > > > > > > > I'm okay with waitingForKey(), because like the other methods here it is > > named > > > > after the associated event. > > > > > > > > As previously mentioned, we've been a bit lax about this convention. That > > said, > > > > I don't think existing violations of the convention should be used to > > justify > > > > continuing this trend =) > > > > > > > > > noLongerWaitingForKey() is harder to justify, but at least it's clear in > > > > context. > > > > > > > > I don't agree. Some things are named after the associated event, some > > aren't. > > > > Half of the methods are verbs (repaint, setWebLayer, requestFullscreen), but > > > > some are nouns (encrypted, keyError, keyMessage). It's hard for someone > > who's > > > > less familiar with the code to understand exactly what's going on. One might > > > > expect noLongerWaitingForKey to fire some sort of event too, but obviously > > it > > > > doesn't. Is this called only once we successfully get a key? Or is it called > > in > > > > other situations? etc. These are all questions a more descriptive name could > > > > help with. > > > > > > > > At a bare minimum, something like: > > > > WebMediaPlayerClient::willWaitForKey() > > > > WebMediaPlayerClient::didGetKey() > > > > would better match the conventions of the Blink API. But there might be an > > even > > > > better name. > > > > > > The first is *currently* waiting, not will or did. > > > > didPausePlaybackForKey? > > Pause is a specific state. How about didBlockPlayback[Waiting]ForKey or didBlockPlaybackForNeededKey? > > Then the other one would be didResumePlaybackBlockedFor[Needed]Key. Sure, this seems fine. Thanks for helping with the names. > > > > > I don't know exactly what the right name should be, other than the current name > > is not OK. > > > > > The current name seems fine and consistent with the other event methods. > > "encrypted", "keyerror", and "keymessage" are all event names. As I mentioned, > > these should probably be "on" or something like that, but we don't have that > > convention. > > > > Again, I'm not seeking consistency within WebMediaPlayerClient. I'm looking for > > consistency with the rest of the Blink API. Blink doesn't really have a strong > > convention of onFoo. The benefit isn't just consistency though--it also helps > > people looking at the Blink API get a sense of when/why things are called, by > > having more descriptive names. > > > > > > > > The second method could be "didResumePlayback[BlockedByKey]" or something like > > that. This tells you exactly when to call it. > > > > This seems fine.
New patchsets have been uploaded after l-g-t-m from ddorwin@chromium.org
Methods renamed to didBlockPlaybackWaitingForKey() and didResumePlaybackBlockedForKey().
lgtm
lgtm
The CQ bit was checked by jrummell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/946503003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=190611 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
