165132 2016-11-28 21:44:32 -0800 HTMLMediaElement::setVolume should updateIsPlayingMedia 2016-11-29 11:47:31 -0800 1 1 1 Unclassified WebKit Media WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 barraclough barraclough oldest_to_newest 1254205 0 barraclough 2016-11-28 21:44:32 -0800 HTMLMediaElement::mediaState takes the volume into account when determining whether media is playing (audio is not considered to be playing if volume is 0). As such, and change to the volume may require mediaState to be recomputed. 1254206 1 295581 barraclough 2016-11-28 21:47:33 -0800 Created attachment 295581 Fix 1254285 2 295581 darin 2016-11-29 08:58:54 -0800 Comment on attachment 295581 Fix View in context: https://bugs.webkit.org/attachment.cgi?id=295581&action=review Might be OK, but what about in loadResource, mediaPlayerVolumeChanged, mediaVolumeDidChange, pageMutedStateDidChange, and setShouldDuck? Maybe this should go into the updateVolume function instead? I’ll say r=me but please consider that. > Source/WebCore/ChangeLog:9 > + is playing (audio is not considered to be playing if volume is 0). As such, and change to typo "and" for "any" > Source/WebCore/html/HTMLMediaElement.cpp:3247 > m_volumeInitialized = true; Follow-up idea: We should make m_volume be std::optional<double> instead of using a double plus a boolean. > Source/WebCore/html/HTMLMediaElement.cpp:3255 > +#if ENABLE(MEDIA_SESSION) > + document().updateIsPlayingMedia(m_elementID); > +#else > + document().updateIsPlayingMedia(); > +#endif Follow-up idea: We should create a helper member function to do this so we don’t have to repeat this five-line sequence in five different places in the class. Can be inline if we like. 1254336 3 barraclough 2016-11-29 10:58:36 -0800 (In reply to comment #2) > Comment on attachment 295581 [details] > Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=295581&action=review > > Might be OK, but what about in loadResource, mediaPlayerVolumeChanged, > mediaVolumeDidChange, pageMutedStateDidChange, and setShouldDuck? Maybe this > should go into the updateVolume function instead? > > I’ll say r=me but please consider that. Have made this change. > > Source/WebCore/ChangeLog:9 > > + is playing (audio is not considered to be playing if volume is 0). As such, and change to > > typo "and" for "any" > > > Source/WebCore/html/HTMLMediaElement.cpp:3247 > > m_volumeInitialized = true; > > Follow-up idea: We should make m_volume be std::optional<double> instead of > using a double plus a boolean. Will look at this as a followup. > > Source/WebCore/html/HTMLMediaElement.cpp:3255 > > +#if ENABLE(MEDIA_SESSION) > > + document().updateIsPlayingMedia(m_elementID); > > +#else > > + document().updateIsPlayingMedia(); > > +#endif > > Follow-up idea: We should create a helper member function to do this so we > don’t have to repeat this five-line sequence in five different places in the > class. Can be inline if we like. If we want to clean up here, with a plan to keep the MEDIA_SESSION code, then I think we could just remove the ifdef & keep the version that passes the argument. (In the callee I think it's unused anyway if MEDIA_SESSION is compiled out.) Discussed with Jer & he recommends holding off on cleanup here for now - thinks MEDIA_SESSION code paths probably need to be removed, and can more cleanly be completely removed as is. 1254372 4 barraclough 2016-11-29 11:47:31 -0800 Committed revision 209084. 295581 2016-11-28 21:47:33 -0800 2016-11-29 08:58:54 -0800 Fix 165132.1.patch text/plain 1551 barraclough SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIwOTA1OSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBACisyMDE2LTExLTI4ICBHYXZpbiBC YXJyYWNsb3VnaCAgPGJhcnJhY2xvdWdoQGFwcGxlLmNvbT4KKworICAgICAgICBIVE1MTWVkaWFF bGVtZW50OjpzZXRWb2x1bWUgc2hvdWxkIHVwZGF0ZUlzUGxheWluZ01lZGlhCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNjUxMzIKKworICAgICAgICBS ZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBIVE1MTWVkaWFFbGVtZW50Ojpt ZWRpYVN0YXRlIHRha2VzIHRoZSB2b2x1bWUgaW50byBhY2NvdW50IHdoZW4gZGV0ZXJtaW5pbmcg d2hldGhlciBtZWRpYQorICAgICAgICBpcyBwbGF5aW5nIChhdWRpbyBpcyBub3QgY29uc2lkZXJl ZCB0byBiZSBwbGF5aW5nIGlmIHZvbHVtZSBpcyAwKS4gQXMgc3VjaCwgYW5kIGNoYW5nZSB0bwor ICAgICAgICB0aGUgdm9sdW1lIG1heSByZXF1aXJlIG1lZGlhU3RhdGUgdG8gYmUgcmVjb21wdXRl ZC4KKworCisgICAgICAgICogaHRtbC9IVE1MTWVkaWFFbGVtZW50LmNwcDoKKyAgICAgICAgKFdl YkNvcmU6OkhUTUxNZWRpYUVsZW1lbnQ6OnNldFZvbHVtZSk6CisKIDIwMTYtMTEtMjggIEppZXdl biBUYW4gIDxqaWV3ZW5fdGFuQGFwcGxlLmNvbT4KIAogICAgICAgICBBU1NFUlRJT04gRkFJTEVE OiBtX3NjcmlwdEV4ZWN1dGlvbkNvbnRleHQtPmlzQ29udGV4dFRocmVhZCgpIHNlZW4gd2l0aCBM YXlvdXRUZXN0IGNyeXB0by9zdWJ0bGUvcnNhLW9hZXAtZ2VuZXJhdGUta2V5LWVuY3J5cHQtZGVj cnlwdC5odG1sCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxNZWRpYUVsZW1lbnQuY3Bw Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2h0bWwvSFRNTE1lZGlhRWxlbWVudC5jcHAJ KHJldmlzaW9uIDIwOTAzNCkKKysrIFNvdXJjZS9XZWJDb3JlL2h0bWwvSFRNTE1lZGlhRWxlbWVu dC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTMyNDcsNiArMzI0NywxMiBAQCBFeGNlcHRpb25Pcjx2 b2lkPiBIVE1MTWVkaWFFbGVtZW50OjpzZXRWCiAgICAgbV92b2x1bWVJbml0aWFsaXplZCA9IHRy dWU7CiAgICAgdXBkYXRlVm9sdW1lKCk7CiAgICAgc2NoZWR1bGVFdmVudChldmVudE5hbWVzKCku dm9sdW1lY2hhbmdlRXZlbnQpOworCisjaWYgRU5BQkxFKE1FRElBX1NFU1NJT04pCisgICAgZG9j dW1lbnQoKS51cGRhdGVJc1BsYXlpbmdNZWRpYShtX2VsZW1lbnRJRCk7CisjZWxzZQorICAgIGRv Y3VtZW50KCkudXBkYXRlSXNQbGF5aW5nTWVkaWEoKTsKKyNlbmRpZgogI2VuZGlmCiAgICAgcmV0 dXJuIHsgfTsKIH0K