Bug 164414 – [Modern Media Controls] UI Library: macOS fullscreen controls

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

[Antoine Quint]

This covers the JS class to instantiate the media controls for fullscreen media playback on macOS.

[Antoine Quint]

<rdar://problem/27989474>

[Antoine Quint]

Created attachment 293886 [details]
Patch

[Build Bot]

Comment on attachment 293886 [details]
Patch

Attachment 293886 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2461809

New failing tests:
media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-buttons-containers-styles.html

[Build Bot]

Created attachment 293888 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 293886 [details]
Patch

Attachment 293886 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2461838

New failing tests:
media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-buttons-containers-styles.html

[Build Bot]

Created attachment 293889 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Antoine Quint]

Created attachment 293890 [details]
Patch

[Build Bot]

Comment on attachment 293890 [details]
Patch

Attachment 293890 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2462233

New failing tests:
media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-buttons-containers-styles.html

[Build Bot]

Created attachment 293896 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 293890 [details]
Patch

Attachment 293890 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2462234

New failing tests:
media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-buttons-containers-styles.html

[Build Bot]

Created attachment 293897 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Antoine Quint]

Created attachment 294048 [details]
Patch

[Dean Jackson]

Comment on attachment 294048 [details]
Patch

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

> Source/WebCore/Modules/modern-media-controls/controls/icon-service.js:46
> -const IconsWithFullScreenVariants = [Icons.Airplay, Icons.Tracks, Icons.Pause, Icons.Play];
> +const IconsWithFullscreenVariants = [Icons.Airplay, Icons.Tracks, Icons.Pause, Icons.Play];

Are we consistent with this?

> Source/WebCore/Modules/modern-media-controls/controls/macos-fullscreen-media-controls.css:40
> +    height: 50px;
> +
> +    transform: translateX(-50%);
> +
> +    overflow: hidden;
> +
> +    height: 75px;
> +
> +    border-radius: 8px;

Why

so

many

blank

lines?

> Source/WebCore/Modules/modern-media-controls/controls/macos-fullscreen-media-controls.css:60
> +/* Make center container centered */

Is this comment necessary? Many of them in this file are a bit basic.

> Source/WebCore/Modules/modern-media-controls/controls/macos-fullscreen-media-controls.js:53
> +            padding: 27,
> +            margin: 27

Do we need constants for these magic numbers?

> Source/WebCore/Modules/modern-media-controls/controls/macos-fullscreen-media-controls.js:90
> +        const numberOfEnabledButtons = this._rightContainer.buttons.filter( button => button.enabled ).length;

Why the ( space )?

> Source/WebCore/Modules/modern-media-controls/controls/macos-fullscreen-media-controls.js:103
> +        this.timeControl.width = 457;

And this completely magic number. I think we need them in a single place with some explanation.

[Build Bot]

Comment on attachment 294048 [details]
Patch

Attachment 294048 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2469511

New failing tests:
media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-buttons-containers-styles.html

[Build Bot]

Created attachment 294090 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Antoine Quint]

Created attachment 294152 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 294152 [details]
Patch

Rejecting attachment 294152 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 294152, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
LayoutTests
:040000 040000 e63c76f6906fc629c923a52c2e7614b1a0c2e920 52c81ff1568926095327c90adcba5094847f8074 M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/2484219

[Antoine Quint]

https://trac.webkit.org/changeset/208448