Bug 221371 - Add intermediate volume icon states between "mute" and "max"
Summary: Add intermediate volume icon states between "mute" and "max"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-03 18:17 PST by Devin Rousso
Modified: 2021-02-09 12:00 PST (History)
16 users (show)

See Also:


Attachments
Patch (115.00 KB, patch)
2021-02-03 19:38 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Video] after Patch is applied (inline) (492.98 KB, video/quicktime)
2021-02-03 19:38 PST, Devin Rousso
no flags Details
[Video] after Patch is applied (fullscreen) (168.13 KB, video/quicktime)
2021-02-03 19:38 PST, Devin Rousso
no flags Details
Patch (125.98 KB, patch)
2021-02-04 00:47 PST, Devin Rousso
eric.carlson: review+
eric.carlson: commit-queue+
Details | Formatted Diff | Diff
Patch (125.97 KB, patch)
2021-02-04 09:15 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Patch] test for EWS to tell me what `muteButton.iconName` is (885 bytes, patch)
2021-02-04 14:33 PST, Devin Rousso
hi: commit-queue-
Details | Formatted Diff | Diff
[Patch] test for EWS to tell me what `muteButton.iconName` is (1.43 KB, patch)
2021-02-04 14:35 PST, Devin Rousso
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
[Patch] test for EWS to tell me what `muteButton.iconName` is (1.43 KB, patch)
2021-02-04 16:22 PST, Devin Rousso
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
[Patch] test for EWS to tell me what `muteButton.iconName` is (3.09 KB, patch)
2021-02-04 21:38 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2021-02-03 18:17:54 PST
*    muted
*  0% <=  25%
* 25% <=  50%
* 50% <=  75%
* 75% <= 100%
Comment 1 Devin Rousso 2021-02-03 19:38:00 PST
Created attachment 419216 [details]
Patch
Comment 2 Devin Rousso 2021-02-03 19:38:49 PST
Created attachment 419217 [details]
[Video] after Patch is applied (inline)
Comment 3 Devin Rousso 2021-02-03 19:38:59 PST
Created attachment 419218 [details]
[Video] after Patch is applied (fullscreen)
Comment 4 Devin Rousso 2021-02-04 00:47:59 PST
Created attachment 419258 [details]
Patch
Comment 5 Eric Carlson 2021-02-04 09:01:24 PST
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/
Comment 6 Devin Rousso 2021-02-04 09:15:49 PST
Created attachment 419288 [details]
Patch
Comment 7 EWS 2021-02-04 10:01:41 PST
Committed r272375: <https://trac.webkit.org/changeset/272375>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419288 [details].
Comment 8 Radar WebKit Bug Importer 2021-02-04 10:02:16 PST
<rdar://problem/73986186>
Comment 9 Truitt Savell 2021-02-04 12:23:52 PST
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...
Comment 10 Devin Rousso 2021-02-04 14:33:55 PST
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 :/
Comment 11 Devin Rousso 2021-02-04 14:35:01 PST
Created attachment 419323 [details]
[Patch] test for EWS to tell me what `muteButton.iconName` is
Comment 12 Devin Rousso 2021-02-04 14:35:43 PST
reopening since EWS won't run for patches uploaded to resolved bugs :(
Comment 13 Devin Rousso 2021-02-04 16:22:49 PST
Created attachment 419336 [details]
[Patch] test for EWS to tell me what `muteButton.iconName` is
Comment 14 Devin Rousso 2021-02-04 21:38:17 PST
Created attachment 419362 [details]
[Patch] test for EWS to tell me what `muteButton.iconName` is
Comment 15 Devin Rousso 2021-02-04 23:41:24 PST
Comment on attachment 419362 [details]
[Patch] test for EWS to tell me what `muteButton.iconName` is

Committed r272404: <https://trac.webkit.org/changeset/272404>
Comment 16 Michael Catanzaro 2021-02-09 09:11:11 PST
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?
Comment 17 Devin Rousso 2021-02-09 11:47:00 PST
(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 :)
Comment 18 Michael Catanzaro 2021-02-09 11:54:16 PST
(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.
Comment 19 Devin Rousso 2021-02-09 12:00:21 PST
(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.