Bug 221371 – Add intermediate volume icon states between "mute" and "max"

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Devin Rousso]

*    muted
*  0% <=  25%
* 25% <=  50%
* 50% <=  75%
* 75% <= 100%

[Devin Rousso]

Created attachment 419216 [details]
Patch

[Devin Rousso]

Created attachment 419217 [details]
[Video] after Patch is applied (inline)

[Devin Rousso]

Created attachment 419218 [details]
[Video] after Patch is applied (fullscreen)

[Devin Rousso]

Created attachment 419258 [details]
Patch

[Eric Carlson]

Comment on attachment 419258 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419258&action=review

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=221371

s/add/Add/

[Devin Rousso]

Created attachment 419288 [details]
Patch

[EWS]

Committed r272375: <https://trac.webkit.org/changeset/272375>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419288 [details].

[Radar WebKit Bug Importer]

<rdar://problem/73986186>

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/272375/webkit

broke media/modern-media-controls/mute-button/mute-button.html on Mac 

history:
https://results.webkit.org/?suite=layout-tests&test=media%2Fmodern-media-controls%2Fmute-button%2Fmute-button.html

Diff:
--- /Volumes/Data/slave/bigsur-release-tests-wk2/build/layout-test-results/media/modern-media-controls/mute-button/mute-button-expected.txt
+++ /Volumes/Data/slave/bigsur-release-tests-wk2/build/layout-test-results/media/modern-media-controls/mute-button/mute-button-actual.txt
@@ -5,7 +5,7 @@
 
 PASS muteButton.element.localName is "button"
 PASS muteButton.element.classList.contains("mute") is true
-PASS muteButton.iconName is Icons.VolumeMutedRTL
+FAIL muteButton.iconName should be [object Object]. Was [object Object].
 PASS muteButton.muted is true
 PASS muteButton.image.element.style.webkitMaskImage.includes("macOS/VolumeMuted-RTL.svg") became true
 Unmuting...

[Devin Rousso]

Created attachment 419322 [details]
[Patch] test for EWS to tell me what `muteButton.iconName` is

running `media/modern-media-controls/mute-button/mute-button.html` passes on my machine, so not really sure what's up with EWS :/

[Devin Rousso]

Created attachment 419323 [details]
[Patch] test for EWS to tell me what `muteButton.iconName` is

[Devin Rousso]

reopening since EWS won't run for patches uploaded to resolved bugs :(

[Devin Rousso]

Created attachment 419336 [details]
[Patch] test for EWS to tell me what `muteButton.iconName` is

[Devin Rousso]

Created attachment 419362 [details]
[Patch] test for EWS to tell me what `muteButton.iconName` is

[Devin Rousso]

Comment on attachment 419362 [details]
[Patch] test for EWS to tell me what `muteButton.iconName` is

Committed r272404: <https://trac.webkit.org/changeset/272404>

[Michael Catanzaro]

This introduced a new warning:

[1385/2409] Building CXX object Source...ources/UnifiedSource-950a39b6-10.cpp.o
In file included from DerivedSources/WebCore/unified-sources/UnifiedSource-950a39b6-10.cpp:2:
../../Source/WebCore/html/HTMLMediaElement.cpp:7159:5: warning: "ENGINEERING_BUILD" is not defined, evaluates to 0 [-Wundef]
 7159 | #if ENGINEERING_BUILD || !defined(NDEBUG)
      |     ^~~~~~~~~~~~~~~~~

I will land this simple fix:

#if (defined(ENGINEERING_BUILD) && ENGINEERING_BUILD) || !defined(NDEBUG)

But it's worth mentioning that non-Apple ports already have ENABLE(DEVELOPER_MODE), which smells like exactly the same thing under a different name. So maybe you could use that instead?

[Devin Rousso]

(In reply to Michael Catanzaro from comment #16)
> This introduced a new warning:
> 
> [1385/2409] Building CXX object
> Source...ources/UnifiedSource-950a39b6-10.cpp.o
> In file included from
> DerivedSources/WebCore/unified-sources/UnifiedSource-950a39b6-10.cpp:2:
> ../../Source/WebCore/html/HTMLMediaElement.cpp:7159:5: warning:
> "ENGINEERING_BUILD" is not defined, evaluates to 0 [-Wundef]
>  7159 | #if ENGINEERING_BUILD || !defined(NDEBUG)
>       |     ^~~~~~~~~~~~~~~~~
> 
> I will land this simple fix:
> 
> #if (defined(ENGINEERING_BUILD) && ENGINEERING_BUILD) || !defined(NDEBUG)
> 
> But it's worth mentioning that non-Apple ports already have ENABLE(DEVELOPER_MODE), which smells like exactly the same thing under a different name. So maybe you could use that instead?
Oh poop!  Sorry about that.  Changing it to `ENABLE(DEVELOPER_MODE)` sounds fine to me :)

[Michael Catanzaro]

(In reply to Devin Rousso from comment #17)
> Changing it to `ENABLE(DEVELOPER_MODE)` sounds fine to me :)

That needs help from an XCode developer... I don't dare attempt this myself. ;)

In the meantime, I'll land the tiny fixup in bug #221613.

[Devin Rousso]

(In reply to Michael Catanzaro from comment #18)
> (In reply to Devin Rousso from comment #17)
> > Changing it to `ENABLE(DEVELOPER_MODE)` sounds fine to me :)
> 
> That needs help from an XCode developer... I don't dare attempt this myself. ;)
I gotchu :P <https://webkit.org/b/221621> ([Cocoa] rename `ENGINEERING_BUILD` to `ENABLE_DEVELOPER_MODE` to match other platforms)

> In the meantime, I'll land the tiny fixup in bug #221613.