Bug 120895 – [Mac] Implement the media controls in JavaScript.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[Mac] Implement the media controls in JavaScript.

[Jer Noble]

Created attachment 210801 [details]
Patch

[Jer Noble]

Created attachment 210811 [details]
Patch

[WebKit Commit Bot]

Attachment 210811 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/FeatureDefines.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/Configurations/FeatureDefines.xcconfig', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp', u'Source/WebCore/Modules/mediacontrols/MediaControlsHost.h', u'Source/WebCore/Modules/mediacontrols/MediaControlsHost.idl', u'Source/WebCore/Modules/mediacontrols/mediaControlsApple.css', u'Source/WebCore/Modules/mediacontrols/mediaControlsApple.js', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/ScriptObject.cpp', u'Source/WebCore/bindings/js/ScriptObject.h', u'Source/WebCore/css/CSSDefaultStyleSheets.cpp', u'Source/WebCore/css/fullscreenQuickTime.css', u'Source/WebCore/css/mediaControlsQuickTime.css', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaElement.h', u'Source/WebCore/rendering/RenderTheme.h', u'Source/WebCore/rendering/RenderThemeMac.h', u'Source/WebCore/rendering/RenderThemeMac.mm', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/Configurations/FeatureDefines.xcconfig', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Configurations/FeatureDefines.xcconfig']" exit_code: 1
Source/WebCore/ChangeLog:135:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 210811 [details]
Patch

Attachment 210811 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1719126

[Build Bot]

Comment on attachment 210811 [details]
Patch

Attachment 210811 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1701957

[Jer Noble]

Created attachment 210825 [details]
Patch

[WebKit Commit Bot]

Attachment 210825 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/FeatureDefines.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/Configurations/FeatureDefines.xcconfig', u'Source/WebCore/DerivedSources.cpp', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp', u'Source/WebCore/Modules/mediacontrols/MediaControlsHost.h', u'Source/WebCore/Modules/mediacontrols/MediaControlsHost.idl', u'Source/WebCore/Modules/mediacontrols/mediaControlsApple.css', u'Source/WebCore/Modules/mediacontrols/mediaControlsApple.js', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/ScriptObject.cpp', u'Source/WebCore/bindings/js/ScriptObject.h', u'Source/WebCore/css/CSSDefaultStyleSheets.cpp', u'Source/WebCore/css/fullscreenQuickTime.css', u'Source/WebCore/css/mediaControlsQuickTime.css', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaElement.h', u'Source/WebCore/rendering/RenderTheme.h', u'Source/WebCore/rendering/RenderThemeMac.h', u'Source/WebCore/rendering/RenderThemeMac.mm', u'Source/WebCore/rendering/RenderThemeWin.cpp', u'Source/WebCore/rendering/RenderThemeWin.h', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/Configurations/FeatureDefines.xcconfig', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Configurations/FeatureDefines.xcconfig']" exit_code: 1
Source/WebCore/ChangeLog:138:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Comment on attachment 210811 [details]
Patch

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

This looks great, modulo my minor complaints and the build errors, but I would like to have more eyes on it.

> Source/WebCore/ChangeLog:81
> +        (Controller.prototype.onfullscreenchange): Reconfure the controls.

Typo: Reconfure

> Source/WebCore/ChangeLog:207
> +        (WebCore::HTMLMediaElement::ensureMediaControlsInjectedScript): Inject the media controls script into the vm.
> +        (WebCore::HTMLMediaElement::didAddUserAgentShadowRoot): Inject the MediaControlsHost into the VM and call

Is it "VM" or "vm"? Should be consistent.

> Source/WebCore/Modules/mediacontrols/MediaControlsHost.h:67
> +

Nit: extra blank line.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:309
> +        var currentTime = this.controls.currentTime = this.controls.currentTime = document.createElement('div');

? currentTime = this.controls.currentTime = this.controls.currentTime
I assume this is a typo

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:446
> +        this.controls.statusDisplay.innerText = 'Loading';

All of these status strings should be localized, but it is fine to do them in a follow up bug.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:591
> +        return this.video.paused || this.video.ended || this.video.readyState < HTMLMediaElement.HAVE_METADATA;

?? this.video.readyState *<* HTMLMediaElement.HAVE_METADATA

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:632
> +        this.controls.thumbnail.style.left = percent*100 + '%';

Nit: there should be a space around the "*"

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:635
> +        var track;

This is unused.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:645
> +                    this.controls.thumbnailImage.src = cue.text;
> +                    return;

It would be more efficient to calculate the position and show the container here so it only happens when there is something to show.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:722
> +        console.log('seekBackFaster: nextRate() = ' + this.nextRate());

I assume this shouldn't be here?

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:747
> +        console.log('seekForwardFaster: nextRate() = ' + this.nextRate());

Ditto

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:775
> +        return (time < 0 ? '-' : '' ) + String('00'+intMinutes).slice(-2) + ":" + String('00'+intSeconds).slice(-2)

Nit: you need a space around the "+"

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:860
> +        for (var i = 0; i < this.video.textTracks.length; ++i) {
> +            var track = this.video.textTracks[i];
> +            if (this.trackHasThumbnails(track)) {
> +                this.controls.thumbnailTrack.classList.remove('hidden');
> +                return;
> +            }

Would it be more efficient to do this when we know there something to show?

[Dean Jackson]

Comment on attachment 210825 [details]
Patch

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

"Big ups" to Jer for taking on this task! It's looking good. I can't wait for this.

> Source/WebCore/ChangeLog:174
> +        (::-webkit-media-controls):
> +        (audio::-webkit-media-controls-panel):
> +        (video::-webkit-media-controls-panel):
> +        (video::-webkit-media-controls-panel:hover):
> +        (audeo::-webkit-media-controls-panel button):
> +        (audeo::-webkit-media-controls-panel button:active):
> +        (audio::-webkit-media-controls-rewind-button):
> +        (audio::-webkit-media-controls-play-button):
> +        (audio::-webkit-media-controls-play-button.paused):
> +        (audeo::-webkit-media-controls-panel .mute-box):
> +        (video::-webkit-media-controls-volume-max-button):
> +        (audio::-webkit-media-controls-panel .volume-box):
> +        (audio::-webkit-media-controls-panel .volume-box:active):
> +        (video::-webkit-media-controls-volume-slider):
> +        (audio::-webkit-media-controls-volume-slider::-webkit-slider-thumb):
> +        (audio::-webkit-media-controls-volume-slider::-webkit-slider-thumb:active::-webkit-slider-thumb):
> +        (video::-webkit-media-controls-volume-min-button):
> +        (audio::-webkit-media-controls-toggle-closed-captions-button):
> +        (audio::-webkit-media-controls-closed-captions-container):
> +        (audio::-webkit-media-controls-closed-captions-container .list):
> +        (audio::-webkit-media-controls-closed-captions-container h3):
> +        (audio::-webkit-media-controls-closed-captions-container ul):
> +        (audio::-webkit-media-controls-closed-captions-container li):
> +        (audio::-webkit-media-controls-closed-captions-container li:hover):
> +        (audio::-webkit-media-controls-closed-captions-container li.selected::before):
> +        (audio::-webkit-media-controls-closed-captions-container li.selected:hover::before):
> +        (audio::-webkit-media-controls-fullscreen-button):
> +        (audio::-webkit-media-controls-status-display):
> +        (audio::-webkit-media-controls-timeline):
> +        (audio::-webkit-media-controls-timeline::-webkit-slider-thumb):
> +        (audio::-webkit-media-controls-timeline:active::-webkit-slider-thumb,):
> +        (audio::-webkit-media-controls-time-remaining-display):
> +        (audio::-webkit-media-controls-timeline-container):
> +        (audio::-webkit-media-controls-panel .thumbnail-track):
> +        (audio::-webkit-media-controls-panel .thumbnail):

I say remove all this junk!

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:62
> +    background-image: -webkit-linear-gradient(top,
> +    rgba(0,  0,  0,  .92) 0,
> +    rgba(0,  0,  0,  .92) 1px,
> +    rgba(89, 89, 89, .92) 1px,
> +    rgba(89, 89, 89, .92) 2px,
> +    rgba(60, 60, 60, .92) 2px,
> +    rgba(35, 35, 35, .92) 12px,
> +    rgba(30, 30, 30, .92) 12px,
> +    rgba(30, 30, 30, .92) 13px,
> +    rgba(25, 25, 25, .92) 13px,
> +    rgba(17, 17, 17, .92) 100%
> +    ) !important;

Do you need to make this !important?

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:69
> +    -webkit-transition: opacity 0.25s linear;

No need for prefix.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:87
> +    display:block;

Nit: space needed.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:107
> +    background-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 15 17"><linearGradient id="a" x2="0" y2="100%" gradientUnits="userSpaceOnUse"><stop offset="0" stop-color="#D8D8D8" /><stop offset="0.44444" stop-color="#D8D8D8" /><stop offset="0.44444" stop-color="#D0D0D0" /><stop offset="0.55555" stop-color="#D0D0D0" /><stop offset="0.55555" stop-color="#C8C8C8" /><stop offset="1" stop-color="#D0D0D0" /></linearGradient><path d="m 7.9131,2 0,-1.548 -2.586,2.155 0,-2.171 -2.582,2.208 2.582,2.175 0,-2.139 2.586,2.155 0,-1.276 c 3.138,0.129 5.491,2.681 5.543,5.838 l -1.031,0 0.016,0.215 1.015,0 c -0.06,3.19 -2.629,5.765 -5.819,5.833 l 0,-1.018 -0.214,0 0,1.021 c -3.21,-0.047 -5.801,-2.631 -5.862,-5.836 l 1.045,0 -0.016,-0.215 -1.045,0 c -0.052,-0.288 -0.318,-0.654 -0.766,-0.654 -0.538,0 -0.755,0.484 -0.755,0.75 0,4.146 3.331,7.506 7.476,7.506 4.146,0 7.506,-3.36 7.506,-7.506 0,-4.059 -3.066,-7.357 -7.093,-7.493" style="fill:url(#a)" /><path d="m 5.1729,11.0518 c -0.38,0 -0.668,-0.129 -0.945,-0.366 -0.083,-0.071 -0.186,-0.134 -0.338,-0.134 -0.277,0 -0.511,0.238 -0.511,0.521 0,0.154 0.083,0.301 0.179,0.383 0.394,0.346 0.911,0.563 1.601,0.563 1.077,0 1.739,-0.681 1.739,-1.608 l 0,-0.013 c 0,-0.911 -0.641,-1.265 -1.296,-1.376 l 0.945,-0.919 c 0.193,-0.19 0.317,-0.337 0.317,-0.604 0,-0.294 -0.228,-0.477 -0.538,-0.477 l -2.354,0 c -0.248,0 -0.455,0.21 -0.455,0.464 0,0.253 0.207,0.463 0.455,0.463 l 1.485,0 -0.939,0.961 c -0.166,0.169 -0.228,0.295 -0.228,0.444 0,0.25 0.207,0.463 0.455,0.463 l 0.166,0 c 0.594,0 0.945,0.222 0.945,0.624 l 0,0.012 c 0,0.367 -0.282,0.599 -0.683,0.599" style="fill:url(#a)" /><path d="m 10.354,9.5342 c 0,0.876 -0.379,1.525 -0.979,1.525 -0.599,0 -0.992,-0.655 -0.992,-1.539 l 0,-0.012 c 0,-0.884 0.388,-1.527 0.979,-1.527 0.592,0 0.992,0.663 0.992,1.539 l 0,0.014 z m -0.979,-2.512 c -1.197,0 -2.008,1.097 -2.008,2.498 l 0,0.014 c 0,1.401 0.792,2.484 1.995,2.484 1.205,0 2.01,-1.097 2.01,-2.498 l 0,-0.012 c 0,-1.402 -0.805,-2.486 -1.997,-2.486" style="fill:url(#a)" /></svg>');

Nit: You shouldn't need the spaces before /> (and elsewhere in this file)

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:167
> +    background-image: -webkit-gradient(
> +        linear,
> +        left top,
> +        right top,
> +        color-stop(0, rgba(17, 17, 17,  0.92)),
> +        color-stop(1, rgba(42, 42, 42, 0.92))
> +    );

Use linear-gradient (no prefix)

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:208
> +    background-image: -webkit-gradient(
> +        linear,
> +        left top,
> +        left bottom,
> +        color-stop(0,  rgba(15, 15, 15, .85)),
> +        color-stop(0.5, rgba(23, 23, 23, .85)),
> +        color-stop(1,  rgba(15, 15, 15, .85))
> +    );

Ditto.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:227
> +    background-image: -webkit-gradient(
> +        linear,
> +        left top,
> +        right top,
> +        color-stop(0, rgba(99, 99, 99, 1)),
> +        color-stop(1, rgba(144, 144, 144, 1))
> +    );

Ditto.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:228
> +    -webkit-box-shadow: inset -1px 0 0 rgba(255, 255, 255, .5), 0 1px rgba(255, 255, 255, .14);

No prefix needed.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:240
> +    background-image: -webkit-gradient(
> +        linear,
> +        left bottom,
> +        right top,
> +        color-stop(0, rgba(160, 160, 160, 1)),
> +        color-stop(1, rgba(221, 221, 221, 1))
> +    );

As above.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:266
> +    max-width: -webkit-calc(100% - 48px); /* right + 10px */
> +    max-height: -webkit-calc(100% - 39px); /* bottom + 10px */

Awesome!

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:320
> +    border-top: 1px solid rgba(0, 0, 0, 0);
> +    border-bottom: 1px solid rgba(0, 0, 0, 0);

Nit: transparent keyword.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:393
> +    background-image: -webkit-gradient(
> +        linear,
> +        left top,
> +        left bottom,
> +        color-stop(0, rgba(7,  7,  7,  0.3)),
> +        color-stop(1, rgba(37, 37, 37, 0.629))
> +    );

linear-gradient as above.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:394
> +    -webkit-box-flex: 1;

This rule seems to have both styles of flexbox rules (-webkit-flex is above). I think you can remove this one.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:395
> +    -webkit-box-shadow: inset -1px 0 0 rgba(0, 0, 0, .68), 0 1px rgba(255, 255, 255, .08);

Nit: no prefix needed.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:411
> +    background-image: -webkit-gradient(
> +        linear,
> +        left bottom,
> +        right top,
> +        color-stop(0, rgba(99,  99,  99,  1)),
> +        color-stop(1, rgba(144, 144, 144, 1))
> +    );

As above.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:424
> +    background-image: -webkit-gradient(
> +        linear,
> +        left bottom,
> +        right top,
> +        color-stop(0, rgba(160, 160, 160, 1)),
> +        color-stop(1, rgba(221, 221, 221, 1))
> +    );

ditto.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:504
> +    video:-webkit-full-screen::-webkit-media-controls-panel {

Nit: indentation.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:520
> +    background: -webkit-linear-gradient(top,
> +    rgba(45, 45, 45, .97) 0,
> +    rgba(30, 30, 30, .97) 19px,
> +    rgba(25, 25, 25, .97) 19px,
> +    rgba(25, 25, 25, .97) 20px,
> +    rgba(19, 19, 19, .97) 20px,
> +    rgba(12, 12, 12, .97) 100%
> +    ) !important;

As above, and another !important that I don't understand.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:522
> +    -webkit-box-shadow: 

Nit: prefix

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:529
> +    -webkit-transition: opacity 0.3s linear !important;

Nit prefix.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:557
> +    background-image: -webkit-linear-gradient(top,
> +    rgba(16, 16, 16, .300) 0,
> +    rgba(9,  9,  9,  .629) 100%
> +    );

I think this is the last one :)

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:565
> +    -webkit-transform: rotateZ(270deg);

Is there a reason you used rotateZ rather than rotate? Did you want this to be composited?

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:646
> +    right: -webkit-calc(50% - 183px); /* 183px is 221px (half the media controller's width) minus 38px (the right position of the captions icon). */
> +    max-width: -webkit-calc(50% + 173px); /* right + 10px */
> +    max-height: -webkit-calc(100% - 124px); /* bottom + 10px */

Will be cool when we can use these expressions directly.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:26
> +};

Nit: No semicolon here, although keep it if you move to the form I prefer, which is var Controller = function(root, video, host)...

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:60
> +        this.video.addEventListener('loadstart', this, false);
> +        this.video.addEventListener('error', this, false);
> +        this.video.addEventListener('abort', this, false);
> +        this.video.addEventListener('suspend', this, false);
> +        this.video.addEventListener('progress', this, false);
> +        this.video.addEventListener('stalled', this, false);
> +        this.video.addEventListener('waiting', this, false);
> +
> +        /* readiness state changes */
> +        this.video.addEventListener('emptied', this, false);
> +        this.video.addEventListener('loadedmetadata', this, false);
> +        this.video.addEventListener('loadeddata', this, false);
> +        this.video.addEventListener('canplay', this, false);
> +        this.video.addEventListener('canplaythrough', this, false);
> +
> +        /* time */
> +        this.video.addEventListener('timeupdate', this, false);
> +        this.video.addEventListener('durationchange', this, false);
> +        this.video.addEventListener('playing', this, false);
> +        this.video.addEventListener('play', this, false);
> +        this.video.addEventListener('pause', this, false);
> +        this.video.addEventListener('ratechange', this, false);
> +
> +        /* volume */
> +        this.video.addEventListener('volumechange', this, false);

For all these, and "webkitfullscreenchange" below, I suggest something like this:

Controller.HANDLED_EVENTS = [
  "loadstart",
  "error", ....
]

Then Controller.prototype.addListeners does:

Controller.HANDLED_EVENTS.forEach(function (eventName) { this.video.addEventListener(eventName, this, false); }.bind(this));

or a for loop which might be nicer (and avoids the bind).

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:84
> +        this.video.removeEventListener('loadstart', this, false);
> +        this.video.removeEventListener('error', this, false);
> +        this.video.removeEventListener('abort', this, false);
> +        this.video.removeEventListener('suspend', this, false);
> +        this.video.removeEventListener('progress', this, false);
> +        this.video.removeEventListener('stalled', this, false);
> +        this.video.removeEventListener('waiting', this, false);

And similarly for all these.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:120
> +            if (event.target == this.video) {

Use ===

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:146
> +                case 'loadstart':
> +                    this.onloadstart(event);
> +                    break;
> +                case 'error':
> +                    this.onerror(event);
> +                    break;
> +                case 'abort':
> +                    this.onabort(event);
> +                    break;
> +                case 'suspend':
> +                    this.onsuspend(event);
> +                    break;
> +                case 'progress':
> +                    this.onprogress(event);
> +                    break;
> +                case 'stalled':
> +                    this.onstalled(event);
> +                    break;
> +                case 'waiting':
> +                    this.onwaiting(event);
> +                    break;
> +                case 'emptied':
> +                    this.onreadystatechange(event);
> +                    break;

OK, so remember that Array you made above with all the event names? Why not be even trickier?
Controller.HANDLED_EVENTS = [
 { name: 'loadstart' },
...
 { name: 'emptied', handler: 'readystatechange' }
...
];

So basically if the object has a handler property, you call this["on" + eventDescriptor.handler](event), otherwise the default is this["on" + eventDescriptor.name](event);

That way you can get rid of this entire switch statement.

Another nit: I suggest the functions get camel casing, and use "handle" rather than "on" e.g. handleWaiting and handleReadyStateChange.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:184
> +            } else if (event.target == this.video.textTracks) {

Use ===

There are many more instances below, so I won't mention it again. Basically, in JS if there is any chance that both sides of the conditional could be null or undefined, you should use strict equality. It's unlikely in this case (since event.target should be non-null), but it is a good habit to get into when checking object equality.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:263
> +        } catch(e) {
> +            console.log(e);
> +        }

This is a little scary. What could go wrong? Remember there isn't necessarily a console at this point.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:285
> +        rewindButton.addEventListener('click', this);

All the addEventListeners in this method are missing their 3rd parameter.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:358
> +        volume.step = .01;

Super nit: you used 0.01 above and .01 here. I'm not sure which is official WK style.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:375
> +        if (type == this.controlsType)

Another case for ===

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:392
> +        for (item in this.controls) {
> +            var control = this.controls[item];
> +            if (control && control.parentNode)
> +                control.parentNode.removeChild(control);
> +        }

I tend to avoid for/in loops:
1. You still have to get the element from the object in the next line.
2. Iterating over an object's properties can sometimes give unexpected answers.

I'd take a look at what the WI code typically uses. Maybe that is a for/i=0 style, or forEach (which would work really well here since you wouldn't need to bind(this))

this.controls.forEach(function(control) { if (control && control.parentNode....

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:441
> +        this.controls.panel.style.bottom = '50px';
> +        this.controls.panel.style.left = ((this.base.offsetWidth - this.controls.panel.offsetWidth) / 2) + 'px';

Is there any reason this is here rather than the CSS?

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:551
> +    isFullScreen: function isFullScreen()
> +    {
> +        return document.webkitCurrentFullScreenElement == this.video;

And here's an example of where == could trip you up!

If document.webkitCurrentFullScreenElement is null or undefined, and this.video is null or undefined, then according to this method we're in full screen.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:570
> +        this.hideTimer = setTimeout(this.hideControls.bind(this), 4000);

I think we should define 4s somewhere on the Controller object.

Controller.HIDE_CONTROLS_DELAY = 4 * 1000;

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:584
> +        var newTime = Math.max(
> +                               this.video.startTime,
> +                               this.video.currentTime - 30,

Similarly with the 30s rewind amount.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:702
> +    maximumSeekRate: 8,
> +    seekDelay: 1500,

If these are really constants, they should be on the Controller itself, not the prototype.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:710
> +    PlayAfterSeeking: 0,
> +    PauseAfterSeeking: 1,

Ditto.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:722
> +        console.log('seekBackFaster: nextRate() = ' + this.nextRate());

Remove this line.

(Or maybe you want a "global" log method to make inspection easier?)

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:747
> +        console.log('seekForwardFaster: nextRate() = ' + this.nextRate());

Ditto.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:775
> +        return (time < 0 ? '-' : '' ) + String('00'+intMinutes).slice(-2) + ":" + String('00'+intSeconds).slice(-2)

Nit: Spacing.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:785
> +            this.controls.panel.classList.add('paused');
> +            this.controls.playButton.classList.add('paused');
> +        } else {
> +            this.controls.panel.classList.remove('paused');
> +            this.controls.playButton.classList.remove('paused');

You can move this into classList.toggle('paused')

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:790
> +            this.hideTimer = setTimeout(this.hideControls.bind(this), 1000);

Ooooh. There is a different hiding delay.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:802
> +    showControls: function showControls()
> +    {
> +        this.controls.panel.classList.add('show');
> +    },
> +
> +    hideControls: function hideControls()
> +    {
> +        this.controls.panel.classList.remove('show');
> +    },

You could combine these into a property with a custom setter.

get controlsVisible()
{
   return this.controls.panel.classList.contains("show");
}

set controlsVisible(newVisible)
{
  if (newVisible)
    this.controls.panel.classList.add("show");
  else
    this.controls.panel.classList.remove("show");
}

Then elsewhere in the code you just controls.controlsVisible = false;

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:846
> +    setStatusHidden: function setStatusHidden(hidden)
> +    {
> +        if (hidden) {
> +            this.controls.statusDisplay.classList.add('hidden');
> +            this.controls.currentTime.classList.remove('hidden');
> +            this.controls.timeline.classList.remove('hidden');
> +            this.controls.remainingTime.classList.remove('hidden');
> +        } else {
> +            this.controls.statusDisplay.classList.remove('hidden');
> +            this.controls.currentTime.classList.add('hidden');
> +            this.controls.timeline.classList.add('hidden');
> +            this.controls.remainingTime.classList.add('hidden');
> +        }
> +    },

Similarly this could be a custom setter on a "statusHidden" property too. There may be places where you can do this above too - I only just thought of it.

[Jer Noble]

(In reply to comment #8)
> (From update of attachment 210811 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210811&action=review
> 
> This looks great, modulo my minor complaints and the build errors, but I would like to have more eyes on it.
> 
> > Source/WebCore/ChangeLog:81
> > +        (Controller.prototype.onfullscreenchange): Reconfure the controls.
> 
> Typo: Reconfure

Fixed.

> > Source/WebCore/ChangeLog:207
> > +        (WebCore::HTMLMediaElement::ensureMediaControlsInjectedScript): Inject the media controls script into the vm.
> > +        (WebCore::HTMLMediaElement::didAddUserAgentShadowRoot): Inject the MediaControlsHost into the VM and call
> 
> Is it "VM" or "vm"? Should be consistent.

The class is VM, the instance in vm. I'll make it consistent.

> > Source/WebCore/Modules/mediacontrols/MediaControlsHost.h:67
> > +
> 
> Nit: extra blank line.

DELETE'D!

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:309
> > +        var currentTime = this.controls.currentTime = this.controls.currentTime = document.createElement('div');
> 
> ? currentTime = this.controls.currentTime = this.controls.currentTime
> I assume this is a typo

It is.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:446
> > +        this.controls.statusDisplay.innerText = 'Loading';
> 
> All of these status strings should be localized, but it is fine to do them in a follow up bug.

Sure; this was on my TODO list, but I wasn't sure how to localize string from within JavaScript.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:591
> > +        return this.video.paused || this.video.ended || this.video.readyState < HTMLMediaElement.HAVE_METADATA;
> 
> ?? this.video.readyState *<* HTMLMediaElement.HAVE_METADATA

Whoops! :)  >.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:632
> > +        this.controls.thumbnail.style.left = percent*100 + '%';
> 
> Nit: there should be a space around the "*"

Fixed.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:635
> > +        var track;
> 
> This is unused.

Removed.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:645
> > +                    this.controls.thumbnailImage.src = cue.text;
> > +                    return;
> 
> It would be more efficient to calculate the position and show the container here so it only happens when there is something to show.

Well, if we bailed out early if there was nothing to show, the thumbnail slider would appear to get "stuck" if it had previously shown.  I'd also rather not have the slider fade in-and-out if a given thumbnail track has discontinuities.

Though we could bail out early if the timeline was entirely hidden.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:722
> > +        console.log('seekBackFaster: nextRate() = ' + this.nextRate());
> 
> I assume this shouldn't be here?

Yep! Printf-style debugging.  I'll remove it.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:747
> > +        console.log('seekForwardFaster: nextRate() = ' + this.nextRate());
> 
> Ditto

Ditto.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:775
> > +        return (time < 0 ? '-' : '' ) + String('00'+intMinutes).slice(-2) + ":" + String('00'+intSeconds).slice(-2)
> 
> Nit: you need a space around the "+"

Added.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:860
> > +        for (var i = 0; i < this.video.textTracks.length; ++i) {
> > +            var track = this.video.textTracks[i];
> > +            if (this.trackHasThumbnails(track)) {
> > +                this.controls.thumbnailTrack.classList.remove('hidden');
> > +                return;
> > +            }
> 
> Would it be more efficient to do this when we know there something to show?

Unfortunately, there isn't any event thrown when cues are added to a track, so even if there aren't any cues now, there may be ones later, but we'll never be notified if that changes.

[Jer Noble]

(In reply to comment #9)
> (From update of attachment 210825 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210825&action=review
> 
> "Big ups" to Jer for taking on this task! It's looking good. I can't wait for this.
> 
> > Source/WebCore/ChangeLog:174
> > +        (::-webkit-media-controls):
> > +        (audio::-webkit-media-controls-panel):
> > +        (video::-webkit-media-controls-panel):
> > +        (video::-webkit-media-controls-panel:hover):
> > +        (audeo::-webkit-media-controls-panel button):
> > +        (audeo::-webkit-media-controls-panel button:active):
> > +        (audio::-webkit-media-controls-rewind-button):
> > +        (audio::-webkit-media-controls-play-button):
> > +        (audio::-webkit-media-controls-play-button.paused):
> > +        (audeo::-webkit-media-controls-panel .mute-box):
> > +        (video::-webkit-media-controls-volume-max-button):
> > +        (audio::-webkit-media-controls-panel .volume-box):
> > +        (audio::-webkit-media-controls-panel .volume-box:active):
> > +        (video::-webkit-media-controls-volume-slider):
> > +        (audio::-webkit-media-controls-volume-slider::-webkit-slider-thumb):
> > +        (audio::-webkit-media-controls-volume-slider::-webkit-slider-thumb:active::-webkit-slider-thumb):
> > +        (video::-webkit-media-controls-volume-min-button):
> > +        (audio::-webkit-media-controls-toggle-closed-captions-button):
> > +        (audio::-webkit-media-controls-closed-captions-container):
> > +        (audio::-webkit-media-controls-closed-captions-container .list):
> > +        (audio::-webkit-media-controls-closed-captions-container h3):
> > +        (audio::-webkit-media-controls-closed-captions-container ul):
> > +        (audio::-webkit-media-controls-closed-captions-container li):
> > +        (audio::-webkit-media-controls-closed-captions-container li:hover):
> > +        (audio::-webkit-media-controls-closed-captions-container li.selected::before):
> > +        (audio::-webkit-media-controls-closed-captions-container li.selected:hover::before):
> > +        (audio::-webkit-media-controls-fullscreen-button):
> > +        (audio::-webkit-media-controls-status-display):
> > +        (audio::-webkit-media-controls-timeline):
> > +        (audio::-webkit-media-controls-timeline::-webkit-slider-thumb):
> > +        (audio::-webkit-media-controls-timeline:active::-webkit-slider-thumb,):
> > +        (audio::-webkit-media-controls-time-remaining-display):
> > +        (audio::-webkit-media-controls-timeline-container):
> > +        (audio::-webkit-media-controls-panel .thumbnail-track):
> > +        (audio::-webkit-media-controls-panel .thumbnail):
> 
> I say remove all this junk!

Gone!

Most of the below was copied over from mediaControlsQuickTime.css and fullscreenQuickTime.css, which explains some of the prefixes.  I'll absolutely clean them out however.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:62
> > +    background-image: -webkit-linear-gradient(top,
> > +    rgba(0,  0,  0,  .92) 0,
> > +    rgba(0,  0,  0,  .92) 1px,
> > +    rgba(89, 89, 89, .92) 1px,
> > +    rgba(89, 89, 89, .92) 2px,
> > +    rgba(60, 60, 60, .92) 2px,
> > +    rgba(35, 35, 35, .92) 12px,
> > +    rgba(30, 30, 30, .92) 12px,
> > +    rgba(30, 30, 30, .92) 13px,
> > +    rgba(25, 25, 25, .92) 13px,
> > +    rgba(17, 17, 17, .92) 100%
> > +    ) !important;
> 
> Do you need to make this !important?

Nope!

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:69
> > +    -webkit-transition: opacity 0.25s linear;
> 
> No need for prefix.

Removed.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:87
> > +    display:block;
> 
> Nit: space needed.

Added.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:107
> > +    background-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 15 17"><linearGradient id="a" x2="0" y2="100%" gradientUnits="userSpaceOnUse"><stop offset="0" stop-color="#D8D8D8" /><stop offset="0.44444" stop-color="#D8D8D8" /><stop offset="0.44444" stop-color="#D0D0D0" /><stop offset="0.55555" stop-color="#D0D0D0" /><stop offset="0.55555" stop-color="#C8C8C8" /><stop offset="1" stop-color="#D0D0D0" /></linearGradient><path d="m 7.9131,2 0,-1.548 -2.586,2.155 0,-2.171 -2.582,2.208 2.582,2.175 0,-2.139 2.586,2.155 0,-1.276 c 3.138,0.129 5.491,2.681 5.543,5.838 l -1.031,0 0.016,0.215 1.015,0 c -0.06,3.19 -2.629,5.765 -5.819,5.833 l 0,-1.018 -0.214,0 0,1.021 c -3.21,-0.047 -5.801,-2.631 -5.862,-5.836 l 1.045,0 -0.016,-0.215 -1.045,0 c -0.052,-0.288 -0.318,-0.654 -0.766,-0.654 -0.538,0 -0.755,0.484 -0.755,0.75 0,4.146 3.331,7.506 7.476,7.506 4.146,0 7.506,-3.36 7.506,-7.506 0,-4.059 -3.066,-7.357 -7.093,-7.493" style="fill:url(#a)" /><path d="m 5.1729,11.0518 c -0.38,0 -0.668,-0.129 -0.945,-0.366 -0.083,-0.071 -0.186,-0.134 -0.338,-0.134 -0.277,0 -0.511,0.238 -0.511,0.521 0,0.154 0.083,0.301 0.179,0.383 0.394,0.346 0.911,0.563 1.601,0.563 1.077,0 1.739,-0.681 1.739,-1.608 l 0,-0.013 c 0,-0.911 -0.641,-1.265 -1.296,-1.376 l 0.945,-0.919 c 0.193,-0.19 0.317,-0.337 0.317,-0.604 0,-0.294 -0.228,-0.477 -0.538,-0.477 l -2.354,0 c -0.248,0 -0.455,0.21 -0.455,0.464 0,0.253 0.207,0.463 0.455,0.463 l 1.485,0 -0.939,0.961 c -0.166,0.169 -0.228,0.295 -0.228,0.444 0,0.25 0.207,0.463 0.455,0.463 l 0.166,0 c 0.594,0 0.945,0.222 0.945,0.624 l 0,0.012 c 0,0.367 -0.282,0.599 -0.683,0.599" style="fill:url(#a)" /><path d="m 10.354,9.5342 c 0,0.876 -0.379,1.525 -0.979,1.525 -0.599,0 -0.992,-0.655 -0.992,-1.539 l 0,-0.012 c 0,-0.884 0.388,-1.527 0.979,-1.527 0.592,0 0.992,0.663 0.992,1.539 l 0,0.014 z m -0.979,-2.512 c -1.197,0 -2.008,1.097 -2.008,2.498 l 0,0.014 c 0,1.401 0.792,2.484 1.995,2.484 1.205,0 2.01,-1.097 2.01,-2.498 l 0,-0.012 c 0,-1.402 -0.805,-2.486 -1.997,-2.486" style="fill:url(#a)" /></svg>');
> 
> Nit: You shouldn't need the spaces before /> (and elsewhere in this file)

Bad habit from my long, long ago IE supporting days.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:167
> > +    background-image: -webkit-gradient(
> > +        linear,
> > +        left top,
> > +        right top,
> > +        color-stop(0, rgba(17, 17, 17,  0.92)),
> > +        color-stop(1, rgba(42, 42, 42, 0.92))
> > +    );
> 
> Use linear-gradient (no prefix)

Sure thing.  I've converted many other instances to linear-gradient, and I'll get the rest.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:208
> > +    background-image: -webkit-gradient(
> > +        linear,
> > +        left top,
> > +        left bottom,
> > +        color-stop(0,  rgba(15, 15, 15, .85)),
> > +        color-stop(0.5, rgba(23, 23, 23, .85)),
> > +        color-stop(1,  rgba(15, 15, 15, .85))
> > +    );
> 
> Ditto.

Ditto.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:227
> > +    background-image: -webkit-gradient(
> > +        linear,
> > +        left top,
> > +        right top,
> > +        color-stop(0, rgba(99, 99, 99, 1)),
> > +        color-stop(1, rgba(144, 144, 144, 1))
> > +    );
> 
> Ditto.

Ditto.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:228
> > +    -webkit-box-shadow: inset -1px 0 0 rgba(255, 255, 255, .5), 0 1px rgba(255, 255, 255, .14);
> 
> No prefix needed.

Removed.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:240
> > +    background-image: -webkit-gradient(
> > +        linear,
> > +        left bottom,
> > +        right top,
> > +        color-stop(0, rgba(160, 160, 160, 1)),
> > +        color-stop(1, rgba(221, 221, 221, 1))
> > +    );
> 
> As above.

Ditto.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:266
> > +    max-width: -webkit-calc(100% - 48px); /* right + 10px */
> > +    max-height: -webkit-calc(100% - 39px); /* bottom + 10px */
> 
> Awesome!

This is Eric's doing.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:320
> > +    border-top: 1px solid rgba(0, 0, 0, 0);
> > +    border-bottom: 1px solid rgba(0, 0, 0, 0);
> 
> Nit: transparent keyword.

Replaced.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:393
> > +    background-image: -webkit-gradient(
> > +        linear,
> > +        left top,
> > +        left bottom,
> > +        color-stop(0, rgba(7,  7,  7,  0.3)),
> > +        color-stop(1, rgba(37, 37, 37, 0.629))
> > +    );
> 
> linear-gradient as above.

DItto.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:394
> > +    -webkit-box-flex: 1;
> 
> This rule seems to have both styles of flexbox rules (-webkit-flex is above). I think you can remove this one.

Sure thing.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:395
> > +    -webkit-box-shadow: inset -1px 0 0 rgba(0, 0, 0, .68), 0 1px rgba(255, 255, 255, .08);
> 
> Nit: no prefix needed.

Ditto.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:411
> > +    background-image: -webkit-gradient(
> > +        linear,
> > +        left bottom,
> > +        right top,
> > +        color-stop(0, rgba(99,  99,  99,  1)),
> > +        color-stop(1, rgba(144, 144, 144, 1))
> > +    );
> 
> As above.

Ditto.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:424
> > +    background-image: -webkit-gradient(
> > +        linear,
> > +        left bottom,
> > +        right top,
> > +        color-stop(0, rgba(160, 160, 160, 1)),
> > +        color-stop(1, rgba(221, 221, 221, 1))
> > +    );
> 
> ditto.

Ditto.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:504
> > +    video:-webkit-full-screen::-webkit-media-controls-panel {
> 
> Nit: indentation.

Fixed.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:520
> > +    background: -webkit-linear-gradient(top,
> > +    rgba(45, 45, 45, .97) 0,
> > +    rgba(30, 30, 30, .97) 19px,
> > +    rgba(25, 25, 25, .97) 19px,
> > +    rgba(25, 25, 25, .97) 20px,
> > +    rgba(19, 19, 19, .97) 20px,
> > +    rgba(12, 12, 12, .97) 100%
> > +    ) !important;
> 
> As above, and another !important that I don't understand.

So full screen properties have gotten !important in the past because pages will inadvertently screw with styles which make the controls "look bad" in full screen. Whereas that's usually fine in inline controls, we've previously taken a hard line on pages messing with the look of our full-screen controls.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:522
> > +    -webkit-box-shadow: 
> 
> Nit: prefix

Fixed.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:529
> > +    -webkit-transition: opacity 0.3s linear !important;
> 
> Nit prefix.

Ditto.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:557
> > +    background-image: -webkit-linear-gradient(top,
> > +    rgba(16, 16, 16, .300) 0,
> > +    rgba(9,  9,  9,  .629) 100%
> > +    );
> 
> I think this is the last one :)

Ditto.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:565
> > +    -webkit-transform: rotateZ(270deg);
> 
> Is there a reason you used rotateZ rather than rotate? Did you want this to be composited?

I do.  Slider thumbs are small enough that compositing them is relatively inexpensive, it keeps the entire slider from repainting for every change event. Repainting is particularly expensive with blurs and shadows, 

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:646
> > +    right: -webkit-calc(50% - 183px); /* 183px is 221px (half the media controller's width) minus 38px (the right position of the captions icon). */
> > +    max-width: -webkit-calc(50% + 173px); /* right + 10px */
> > +    max-height: -webkit-calc(100% - 124px); /* bottom + 10px */
> 
> Will be cool when we can use these expressions directly.

Indeed.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:26
> > +};
> 
> Nit: No semicolon here, although keep it if you move to the form I prefer, which is var Controller = function(root, video, host)...

So, I used to agree with you.  However, I've found that when debugging, that kind of form will lead to "anonymous function" being put into the stack trace, rather than the function's name.  If you want to have it both ways, you can do: "var Controller = function Controller(root, video, host)", but that seems overly verbose. 

Also, when this file is processed, all the newlines are removed, and so semicolons which are not technically necessary become absolutely necessary.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:60
> > +        this.video.addEventListener('loadstart', this, false);
> > +        this.video.addEventListener('error', this, false);
> > +        this.video.addEventListener('abort', this, false);
> > +        this.video.addEventListener('suspend', this, false);
> > +        this.video.addEventListener('progress', this, false);
> > +        this.video.addEventListener('stalled', this, false);
> > +        this.video.addEventListener('waiting', this, false);
> > +
> > +        /* readiness state changes */
> > +        this.video.addEventListener('emptied', this, false);
> > +        this.video.addEventListener('loadedmetadata', this, false);
> > +        this.video.addEventListener('loadeddata', this, false);
> > +        this.video.addEventListener('canplay', this, false);
> > +        this.video.addEventListener('canplaythrough', this, false);
> > +
> > +        /* time */
> > +        this.video.addEventListener('timeupdate', this, false);
> > +        this.video.addEventListener('durationchange', this, false);
> > +        this.video.addEventListener('playing', this, false);
> > +        this.video.addEventListener('play', this, false);
> > +        this.video.addEventListener('pause', this, false);
> > +        this.video.addEventListener('ratechange', this, false);
> > +
> > +        /* volume */
> > +        this.video.addEventListener('volumechange', this, false);
> 
> For all these, and "webkitfullscreenchange" below, I suggest something like this:
> 
> Controller.HANDLED_EVENTS = [
>   "loadstart",
>   "error", ....
> ]
> 
> Then Controller.prototype.addListeners does:
> 
> Controller.HANDLED_EVENTS.forEach(function (eventName) { this.video.addEventListener(eventName, this, false); }.bind(this));
> 
> or a for loop which might be nicer (and avoids the bind).

Too clever by half. forEach() takes a second parameter and will bind 'this' to it during the callback. ;)

I'd also name it HANDLED_VIDEO_EVENTS, since it applies only to the video element.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:84
> > +        this.video.removeEventListener('loadstart', this, false);
> > +        this.video.removeEventListener('error', this, false);
> > +        this.video.removeEventListener('abort', this, false);
> > +        this.video.removeEventListener('suspend', this, false);
> > +        this.video.removeEventListener('progress', this, false);
> > +        this.video.removeEventListener('stalled', this, false);
> > +        this.video.removeEventListener('waiting', this, false);
> 
> And similarly for all these.

Ditto.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:120
> > +            if (event.target == this.video) {
> 
> Use ===

Done.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:146
> > +                case 'loadstart':
> > +                    this.onloadstart(event);
> > +                    break;
> > +                case 'error':
> > +                    this.onerror(event);
> > +                    break;
> > +                case 'abort':
> > +                    this.onabort(event);
> > +                    break;
> > +                case 'suspend':
> > +                    this.onsuspend(event);
> > +                    break;
> > +                case 'progress':
> > +                    this.onprogress(event);
> > +                    break;
> > +                case 'stalled':
> > +                    this.onstalled(event);
> > +                    break;
> > +                case 'waiting':
> > +                    this.onwaiting(event);
> > +                    break;
> > +                case 'emptied':
> > +                    this.onreadystatechange(event);
> > +                    break;
> 
> OK, so remember that Array you made above with all the event names? Why not be even trickier?
> Controller.HANDLED_EVENTS = [
>  { name: 'loadstart' },
> ...
>  { name: 'emptied', handler: 'readystatechange' }
> ...
> ];
> 
> So basically if the object has a handler property, you call this["on" + eventDescriptor.handler](event), otherwise the default is this["on" + eventDescriptor.name](event);
>
> That way you can get rid of this entire switch statement.
> 
> Another nit: I suggest the functions get camel casing, and use "handle" rather than "on" e.g. handleWaiting and handleReadyStateChange.

Wouldn't this be better as a plain object in that case?  I.e.:

Controller.HANDLED_VIDEO_EVENTS = {
  loadstart: 'handleLoadStart',
  ...
  emptied: 'handleReadyStateChange',
}

Then you could do:

if (event.target === this.video) {
  var handlerName = this.HANDLED_VIDEO_EVENTS[event.name];
  var handler = this[handlerName];
  handler.call(this, event);
}

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:184
> > +            } else if (event.target == this.video.textTracks) {
> 
> Use ===
> 
> There are many more instances below, so I won't mention it again. Basically, in JS if there is any chance that both sides of the conditional could be null or undefined, you should use strict equality. It's unlikely in this case (since event.target should be non-null), but it is a good habit to get into when checking object equality.

Changed.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:263
> > +        } catch(e) {
> > +            console.log(e);
> > +        }
> 
> This is a little scary. What could go wrong? Remember there isn't necessarily a console at this point.

If an exception is thrown and nobody explicitly cleans the exception up from within WebCore (by calling ExecState::clearException()), then this can cause an ASSERT crash the next time that VM (the one owned by the HTMLMediaElement for use by controls) is accessed.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:285
> > +        rewindButton.addEventListener('click', this);
> 
> All the addEventListeners in this method are missing their 3rd parameter.

Added.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:358
> > +        volume.step = .01;
> 
> Super nit: you used 0.01 above and .01 here. I'm not sure which is official WK style.

Huh.  How about we assume that .01 is correct. :)

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:375
> > +        if (type == this.controlsType)
> 
> Another case for ===

I don't think it matters for literals.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:392
> > +        for (item in this.controls) {
> > +            var control = this.controls[item];
> > +            if (control && control.parentNode)
> > +                control.parentNode.removeChild(control);
> > +        }
> 
> I tend to avoid for/in loops:
> 1. You still have to get the element from the object in the next line.
> 2. Iterating over an object's properties can sometimes give unexpected answers.
> 
> I'd take a look at what the WI code typically uses. Maybe that is a for/i=0 style, or forEach (which would work really well here since you wouldn't need to bind(this))
> 
> this.controls.forEach(function(control) { if (control && control.parentNode....

this.controls is an Object, so it doesn't have a forEach.  I'd have to do this.controls.keys.forEach(), and then i'd have to get the element again anyway, which gets us back to square 1.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:441
> > +        this.controls.panel.style.bottom = '50px';
> > +        this.controls.panel.style.left = ((this.base.offsetWidth - this.controls.panel.offsetWidth) / 2) + 'px';
> 
> Is there any reason this is here rather than the CSS?

I'm going to add dragging support, which will require me to fiddle with .bottom and .left during mousemove events.  I guess I could do something like: 'left: -webkit-calc(50% - 220px)' to start, then replace it with absolute pixel values later.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:551
> > +    isFullScreen: function isFullScreen()
> > +    {
> > +        return document.webkitCurrentFullScreenElement == this.video;
> 
> And here's an example of where == could trip you up!
> 
> If document.webkitCurrentFullScreenElement is null or undefined, and this.video is null or undefined, then according to this method we're in full screen.

But this.video will never be null or undefined.  I'll change it anyway. :)

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:570
> > +        this.hideTimer = setTimeout(this.hideControls.bind(this), 4000);
> 
> I think we should define 4s somewhere on the Controller object.
> 
> Controller.HIDE_CONTROLS_DELAY = 4 * 1000;

OK.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:584
> > +        var newTime = Math.max(
> > +                               this.video.startTime,
> > +                               this.video.currentTime - 30,
> 
> Similarly with the 30s rewind amount.

OK.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:702
> > +    maximumSeekRate: 8,
> > +    seekDelay: 1500,
> 
> If these are really constants, they should be on the Controller itself, not the prototype.

Wouldn't that screw with inheritance?  What if a port wants to extend Controller and increase the maximumSeekRate?  They'd have to re-write handleSeekBackMouseDown, etc, instead of just modifying the prototype.

I guess these are more akin to defaults than constants.  Same as the HIDE_CONTROLS_DELAY above.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:710
> > +    PlayAfterSeeking: 0,
> > +    PauseAfterSeeking: 1,
> 
> Ditto.

These are constants (or enums) however.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:722
> > +        console.log('seekBackFaster: nextRate() = ' + this.nextRate());
> 
> Remove this line.
> 
> (Or maybe you want a "global" log method to make inspection easier?)

No that was incidental printf-debug statements left in.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:747
> > +        console.log('seekForwardFaster: nextRate() = ' + this.nextRate());
> 
> Ditto.

Ditto.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:775
> > +        return (time < 0 ? '-' : '' ) + String('00'+intMinutes).slice(-2) + ":" + String('00'+intSeconds).slice(-2)
> 
> Nit: Spacing.

Fixed.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:785
> > +            this.controls.panel.classList.add('paused');
> > +            this.controls.playButton.classList.add('paused');
> > +        } else {
> > +            this.controls.panel.classList.remove('paused');
> > +            this.controls.playButton.classList.remove('paused');
> 
> You can move this into classList.toggle('paused')

Not really. I don't think that logic works.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:790
> > +            this.hideTimer = setTimeout(this.hideControls.bind(this), 1000);
> 
> Ooooh. There is a different hiding delay.

Whoops!  There shouldn't be.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:802
> > +    showControls: function showControls()
> > +    {
> > +        this.controls.panel.classList.add('show');
> > +    },
> > +
> > +    hideControls: function hideControls()
> > +    {
> > +        this.controls.panel.classList.remove('show');
> > +    },
> 
> You could combine these into a property with a custom setter.
> 
> get controlsVisible()
> {
>    return this.controls.panel.classList.contains("show");
> }
> 
> set controlsVisible(newVisible)
> {
>   if (newVisible)
>     this.controls.panel.classList.add("show");
>   else
>     this.controls.panel.classList.remove("show");
> }
> 
> Then elsewhere in the code you just controls.controlsVisible = false;

While that's very cool, that means I couldn't pass showControls as a parameter to setTimeout any more, and would have to wrap it in a function instead.  So I don't believe it saves anything.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:846
> > +    setStatusHidden: function setStatusHidden(hidden)
> > +    {
> > +        if (hidden) {
> > +            this.controls.statusDisplay.classList.add('hidden');
> > +            this.controls.currentTime.classList.remove('hidden');
> > +            this.controls.timeline.classList.remove('hidden');
> > +            this.controls.remainingTime.classList.remove('hidden');
> > +        } else {
> > +            this.controls.statusDisplay.classList.remove('hidden');
> > +            this.controls.currentTime.classList.add('hidden');
> > +            this.controls.timeline.classList.add('hidden');
> > +            this.controls.remainingTime.classList.add('hidden');
> > +        }
> > +    },
> 
> Similarly this could be a custom setter on a "statusHidden" property too. There may be places where you can do this above too - I only just thought of it.

Similarly, I don't see a lot of value in treating 'statusHidden' as a virtual property.  Perhaps if I was planning to expose it to external clients, maybe.  But this code is entirely self contained.

[Antoine Quint]

Couple more things:

1. I *think* we support calc() unprefixed
2. If you use forEach() (do it, it's awesome), the second arg is "this" so you don't have to use a .bind() call

Awesome that you guys are making this happen!

[Eric Carlson]

(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 210825 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=210825&action=review
> > 
> 
> > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:266
> > > +    max-width: -webkit-calc(100% - 48px); /* right + 10px */
> > > +    max-height: -webkit-calc(100% - 39px); /* bottom + 10px */
> > 
> > Awesome!
> 
> This is Eric's doing.
> 

  I would love to take credit for this cleverness, but Antoine dreamed this up.

> 
> > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:565
> > > +    -webkit-transform: rotateZ(270deg);
> > 
> > Is there a reason you used rotateZ rather than rotate? Did you want this to be composited?
> 
> I do.  Slider thumbs are small enough that compositing them is relatively inexpensive, it keeps the entire slider from repainting for every change event. Repainting is particularly expensive with blurs and shadows, 
> 

  It is probably worth documenting this in a comment.

> 
> > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:146
> > > +                case 'loadstart':
> > > +                    this.onloadstart(event);
> > > +                    break;
> > > +                case 'error':
> > > +                    this.onerror(event);
> > > +                    break;
> > > +                case 'abort':
> > > +                    this.onabort(event);
> > > +                    break;
> > > +                case 'suspend':
> > > +                    this.onsuspend(event);
> > > +                    break;
> > > +                case 'progress':
> > > +                    this.onprogress(event);
> > > +                    break;
> > > +                case 'stalled':
> > > +                    this.onstalled(event);
> > > +                    break;
> > > +                case 'waiting':
> > > +                    this.onwaiting(event);
> > > +                    break;
> > > +                case 'emptied':
> > > +                    this.onreadystatechange(event);
> > > +                    break;
> > 
> > OK, so remember that Array you made above with all the event names? Why not be even trickier?
> > Controller.HANDLED_EVENTS = [
> >  { name: 'loadstart' },
> > ...
> >  { name: 'emptied', handler: 'readystatechange' }
> > ...
> > ];
> > 
> > So basically if the object has a handler property, you call this["on" + eventDescriptor.handler](event), otherwise the default is this["on" + eventDescriptor.name](event);
> >
> > That way you can get rid of this entire switch statement.
> > 
> > Another nit: I suggest the functions get camel casing, and use "handle" rather than "on" e.g. handleWaiting and handleReadyStateChange.
> 
> Wouldn't this be better as a plain object in that case?  I.e.:
> 
> Controller.HANDLED_VIDEO_EVENTS = {
>   loadstart: 'handleLoadStart',
>   ...
>   emptied: 'handleReadyStateChange',
> }
> 
> Then you could do:
> 
> if (event.target === this.video) {
>   var handlerName = this.HANDLED_VIDEO_EVENTS[event.name];
>   var handler = this[handlerName];
>   handler.call(this, event);
> }
> 

  Nice!

> 
> > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:263
> > > +        } catch(e) {
> > > +            console.log(e);
> > > +        }
> > 
> > This is a little scary. What could go wrong? Remember there isn't necessarily a console at this point.
> 
> If an exception is thrown and nobody explicitly cleans the exception up from within WebCore (by calling ExecState::clearException()), then this can cause an ASSERT crash the next time that VM (the one owned by the HTMLMediaElement for use by controls) is accessed.
> 

How about:

    } catch(e) {
        if (window.console)
            console.log(e);
    }

[Eric Carlson]

(In reply to comment #10)
> (In reply to comment #8)
> > (From update of attachment 210811 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=210811&action=review

> 
> > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:446
> > > +        this.controls.statusDisplay.innerText = 'Loading';
> > 
> > All of these status strings should be localized, but it is fine to do them in a follow up bug.
> 
> Sure; this was on my TODO list, but I wasn't sure how to localize string from within JavaScript.
> 

  I am not sure either, I only meant it is worth a FIXME and a bug number.

[Jer Noble]

(In reply to comment #12)
> Couple more things:
> 
> 1. I *think* we support calc() unprefixed

Just tested, and we do!

> 2. If you use forEach() (do it, it's awesome), the second arg is "this" so you don't have to use a .bind() call

Will do.

[Jer Noble]

(In reply to comment #13)
> > > > +    -webkit-transform: rotateZ(270deg);
> > > 
> > > Is there a reason you used rotateZ rather than rotate? Did you want this to be composited?
> > 
> > I do.  Slider thumbs are small enough that compositing them is relatively inexpensive, it keeps the entire slider from repainting for every change event. Repainting is particularly expensive with blurs and shadows, 
> > 
> 
>   It is probably worth documenting this in a comment.

Will do. (There are three instances of this in the .css file)

> > > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:263
> > > > +        } catch(e) {
> > > > +            console.log(e);
> > > > +        }
> > > 
> > > This is a little scary. What could go wrong? Remember there isn't necessarily a console at this point.
> > 
> > If an exception is thrown and nobody explicitly cleans the exception up from within WebCore (by calling ExecState::clearException()), then this can cause an ASSERT crash the next time that VM (the one owned by the HTMLMediaElement for use by controls) is accessed.
> > 
> 
> How about:
> 
>     } catch(e) {
>         if (window.console)
>             console.log(e);
>     }

Sure thing.

[Jer Noble]

Created attachment 210933 [details]
Patch

[Jer Noble]

Created attachment 210935 [details]
Patch

Fix release mac and mac-wk2.

[Antoine Quint]

I'll review this tomorrow morning when I'm in front of my computer…

[Build Bot]

Comment on attachment 210935 [details]
Patch

Attachment 210935 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1733117

New failing tests:
http/tests/css/vertical-align-baseline-after-image-load-2.html
media/audio-controls-rendering.html
fast/dom/boolean-attribute-reflection.html
fast/hidpi/video-controls-in-hidpi.html
media/track/track-css-all-cues.html
http/tests/inspector/inspect-element.html
compositing/geometry/video-opacity-overlay.html
media/track/track-css-cue-lifetime.html
http/tests/css/vertical-align-baseline-after-image-load-3.html
media/controls-styling-strict.html
compositing/layers-inside-overflow-scroll.html
media/controls-without-preload.html
compositing/self-painting-layers.html
compositing/overflow/scroll-ancestor-update.html
media/audio-delete-while-slider-thumb-clicked.html
media/controls-after-reload.html
media/media-controls-clone-crash.html
compositing/reflections/load-video-in-reflection.html
media/click-volume-bar-not-pausing.html
fast/css/object-fit/object-fit-video-poster.html
compositing/geometry/video-fixed-scrolling.html
fast/layers/video-layer.html
media/media-captions-no-controls.html
compositing/video/video-object-fit.html
media/controls-strict.html
fullscreen/video-controls-drag.html
fullscreen/video-controls-override.html
http/tests/css/vertical-align-baseline-after-image-load.html
accessibility/media-element.html
compositing/overflow/overflow-compositing-descendant.html

[Build Bot]

Created attachment 211000 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Jer Noble]

(In reply to comment #11)
> (In reply to comment #9)
> > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:375
> > > +        if (type == this.controlsType)
> > 
> > Another case for ===
> 
> I don't think it matters for literals.

Aha! This checks that the literals are the same type as well as equal!  I'll change this.

-Jer

[Dean Jackson]

(In reply to comment #22)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:375
> > > > +        if (type == this.controlsType)
> > > 
> > > Another case for ===
> > 
> > I don't think it matters for literals.
> 
> Aha! This checks that the literals are the same type as well as equal!  I'll change this.

Yeah, I don't think it is official style, but we should use === everywhere unless you're doing something that's completely safe, such as blah == "hello"

[Antoine Quint]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #11)
> > > (In reply to comment #9)
> > > > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:375
> > > > > +        if (type == this.controlsType)
> > > > 
> > > > Another case for ===
> > > 
> > > I don't think it matters for literals.
> > 
> > Aha! This checks that the literals are the same type as well as equal!  I'll change this.
> 
> Yeah, I don't think it is official style, but we should use === everywhere unless you're doing something that's completely safe, such as blah == "hello"

In Web Inspector code, we use === everywhere unless there is a good reason.

[Antoine Quint]

Comment on attachment 210935 [details]
Patch

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

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:107
> +    background-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 15 17"><linearGradient id="a" x2="0" y2="100%" gradientUnits="userSpaceOnUse"><stop offset="0" stop-color="#D8D8D8"/><stop offset="0.44444" stop-color="#D8D8D8"/><stop offset="0.44444" stop-color="#D0D0D0"/><stop offset="0.55555" stop-color="#D0D0D0"/><stop offset="0.55555" stop-color="#C8C8C8"/><stop offset="1" stop-color="#D0D0D0"/></linearGradient><path d="m 7.9131,2 0,-1.548 -2.586,2.155 0,-2.171 -2.582,2.208 2.582,2.175 0,-2.139 2.586,2.155 0,-1.276 c 3.138,0.129 5.491,2.681 5.543,5.838 l -1.031,0 0.016,0.215 1.015,0 c -0.06,3.19 -2.629,5.765 -5.819,5.833 l 0,-1.018 -0.214,0 0,1.021 c -3.21,-0.047 -5.801,-2.631 -5.862,-5.836 l 1.045,0 -0.016,-0.215 -1.045,0 c -0.052,-0.288 -0.318,-0.654 -0.766,-0.654 -0.538,0 -0.755,0.484 -0.755,0.75 0,4.146 3.331,7.506 7.476,7.506 4.146,0 7.506,-3.36 7.506,-7.506 0,-4.059 -3.066,-7.357 -7.093,-7.493" style="fill:url(#a)"/><path d="m 5.1729,11.0518 c -0.38,0 -0.668,-0.129 -0.945,-0.366 -0.083,-0.071 -0.186,-0.134 -0.338,-0.134 -0.277,0 -0.511,0.238 -0.511,0.521 0,0.154 0.083,0.301 0.179,0.383 0.394,0.346 0.911,0.563 1.601,0.563 1.077,0 1.739,-0.681 1.739,-1.608 l 0,-0.013 c 0,-0.911 -0.641,-1.265 -1.296,-1.376 l 0.945,-0.919 c 0.193,-0.19 0.317,-0.337 0.317,-0.604 0,-0.294 -0.228,-0.477 -0.538,-0.477 l -2.354,0 c -0.248,0 -0.455,0.21 -0.455,0.464 0,0.253 0.207,0.463 0.455,0.463 l 1.485,0 -0.939,0.961 c -0.166,0.169 -0.228,0.295 -0.228,0.444 0,0.25 0.207,0.463 0.455,0.463 l 0.166,0 c 0.594,0 0.945,0.222 0.945,0.624 l 0,0.012 c 0,0.367 -0.282,0.599 -0.683,0.599" style="fill:url(#a)"/><path d="m 10.354,9.5342 c 0,0.876 -0.379,1.525 -0.979,1.525 -0.599,0 -0.992,-0.655 -0.992,-1.539 l 0,-0.012 c 0,-0.884 0.388,-1.527 0.979,-1.527 0.592,0 0.992,0.663 0.992,1.539 l 0,0.014 z m -0.979,-2.512 c -1.197,0 -2.008,1.097 -2.008,2.498 l 0,0.014 c 0,1.401 0.792,2.484 1.995,2.484 1.205,0 2.01,-1.097 2.01,-2.498 l 0,-0.012 c 0,-1.402 -0.805,-2.486 -1.997,-2.486" style="fill:url(#a)"/></svg>');

Nit: you could probably use a better gradient name than "a" here. Second nit, you use rgba() colors before and here you use hexadecimals, why not rgb() to match the previous style? This applies to other SVG gradients defined throughout this file.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:399
> +    /* Slider thumbs are small, so forcing a compositing layer is inexpensive,
> +       and it keeps the slider from having to repaint while sliding. */
> +    -webkit-transform: translateY(1px) rotateZ(-45deg);

I think the comment could point that it's forced into a compositing layer due to the use of rotateZ(). This may not be obvious to everyone.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:502
> +    -webkit-align-items: flex-start !important;
> +    -webkit-justify-content: flex-end !important;

This is kind of a nit, and I don't think we have project-wide guidelines about this, but I think it would be nice for maintenance concerns to point out why we're using an !important here, ie. point to the rule that's being overridden here, if known. This goes for any other !important in this patch.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:566
> +    /* Slider thumbs are small, so forcing a compositing layer is inexpensive,
> +       and it keeps the slider from having to repaint while sliding. */
> +    -webkit-transform: rotateZ(270deg);

See previous comment about why the compositing layer is forced.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:597
> +    position: absolute;
> +
> +

Why two empty lines?

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:63
> +    HIDE_CONTROLS_DELAY: 4 * 1000,
> +    REWIND_AMOUNT: 30 * 1000,
> +    MAXIMUM_SEEK_RATE: 8,
> +    SEEK_DELAY: 1500,

Just a quick style note, in Web Inspector code we don't use THIS_TYPE_OF_CASING for constants, but rather use ThisTypeOfCasing.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:65
> +    addListeners: function addListeners()

How come all methods are named this way? It seems unnecessary to me, why not drop the method name after the `function` keyword?

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:68
> +            this.video.addEventListener(name, this, false);

In Web Inspector code we would not have the last parameter for addEventListener/removeEventListener if it's `false`.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:79
> +            this.video.removeEventListener(name, this, false);

See previous comment about the last `false` attribute.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:89
> +        try {

Not a big fan of this massive try/catch. What's the purpose here? Do you know what kind of situations may yield an exception? Shouldn't those exceptions be handled explicitly? You may also use console.assert() too.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:172
> +            if (window.console)

Under what conditions would there not be a `window.console` object?

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:179
> +        this.base = document.createElement('div');

Since you do that in createControls, why not assign to a `var base` here too and save some property lookups?

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:379
> +    handleProgress: function handleProgress(event)
> +    {
> +    },

This should either have a comment and/or FIXME to explain why it's there or be removed.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:411
> +    handlePlaying: function handlePlaying(event)
> +    {
> +    },

This should either have a comment and/or FIXME to explain why it's there or be removed.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:425
> +    handleRateChange: function handleRateChange(event)
> +    {
> +    },

This should either have a comment and/or FIXME to explain why it's there or be removed.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:453
> +        var track = event.track;
> +        track.removeEventListener('cuechange', this, false);

Looks like you could get rid of the `track` var here since it's only used once.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:548
> +        var percent = (event.offsetX - this.controls.timeline.offsetLeft) / this.controls.timeline.offsetWidth;

I'm not totally sure what offsetX is since I don't think it's part of any DOM standard, but this doesn't look like this would work well with transforms. If you want to obtain the coordinate of the mouse event local to the element, you should first transform the mouse coordinate to the element's local coordinate system and then work out where you are based on the untransformed width of the element.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:740
> +    updateReadyState: function updateReadyState()
> +    {
> +        if (this.video.readyState > HTMLMediaElement.HAVE_NOTHING)
> +            this.setStatusHidden(true);
> +        else
> +            this.setStatusHidden(false);
> +    },

What about a one-liner?

this.setStatusHidden(this.video.readyState > HTMLMediaElement.HAVE_NOTHING);

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:753
> +        if (hidden) {
> +            this.controls.statusDisplay.classList.add('hidden');
> +            this.controls.currentTime.classList.remove('hidden');
> +            this.controls.timeline.classList.remove('hidden');
> +            this.controls.remainingTime.classList.remove('hidden');
> +        } else {
> +            this.controls.statusDisplay.classList.remove('hidden');
> +            this.controls.currentTime.classList.add('hidden');
> +            this.controls.timeline.classList.add('hidden');
> +            this.controls.remainingTime.classList.add('hidden');

Sounds to me like the "hidden" CSS class name should be a constant. In inspector code we always have CSS class names be constants, that way you can easily tell what CSS classes are set by a given piece of script.

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:823
> +            menuItem.addEventListener('click', this.captionItemSelected.bind(this), false);

I wish we could just pass `this` as the event handler there and distinguish menu items from other clickable items with a CSS class name maybe?

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:854
> +        var item = event.target;
> +        this.host.setSelectedTextTrack(item.track);

You can lose the `item` var here.

[Jer Noble]

(In reply to comment #25)
> (From update of attachment 210935 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210935&action=review
> 
> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:107
> > +    background-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 15 17"><linearGradient id="a" x2="0" y2="100%" gradientUnits="userSpaceOnUse"><stop offset="0" stop-color="#D8D8D8"/><stop offset="0.44444" stop-color="#D8D8D8"/><stop offset="0.44444" stop-color="#D0D0D0"/><stop offset="0.55555" stop-color="#D0D0D0"/><stop offset="0.55555" stop-color="#C8C8C8"/><stop offset="1" stop-color="#D0D0D0"/></linearGradient><path d="m 7.9131,2 0,-1.548 -2.586,2.155 0,-2.171 -2.582,2.208 2.582,2.175 0,-2.139 2.586,2.155 0,-1.276 c 3.138,0.129 5.491,2.681 5.543,5.838 l -1.031,0 0.016,0.215 1.015,0 c -0.06,3.19 -2.629,5.765 -5.819,5.833 l 0,-1.018 -0.214,0 0,1.021 c -3.21,-0.047 -5.801,-2.631 -5.862,-5.836 l 1.045,0 -0.016,-0.215 -1.045,0 c -0.052,-0.288 -0.318,-0.654 -0.766,-0.654 -0.538,0 -0.755,0.484 -0.755,0.75 0,4.146 3.331,7.506 7.476,7.506 4.146,0 7.506,-3.36 7.506,-7.506 0,-4.059 -3.066,-7.357 -7.093,-7.493" style="fill:url(#a)"/><path d="m 5.1729,11.0518 c -0.38,0 -0.668,-0.129 -0.945,-0.366 -0.083,-0.071 -0.186,-0.134 -0.338,-0.134 -0.277,0 -0.511,0.238 -0.511,0.521 0,0.154 0.083,0.301 0.179,0.383 0.394,0.346 0.911,0.563 1.601,0.563 1.077,0 1.739,-0.681 1.739,-1.608 l 0,-0.013 c 0,-0.911 -0.641,-1.265 -1.296,-1.376 l 0.945,-0.919 c 0.193,-0.19 0.317,-0.337 0.317,-0.604 0,-0.294 -0.228,-0.477 -0.538,-0.477 l -2.354,0 c -0.248,0 -0.455,0.21 -0.455,0.464 0,0.253 0.207,0.463 0.455,0.463 l 1.485,0 -0.939,0.961 c -0.166,0.169 -0.228,0.295 -0.228,0.444 0,0.25 0.207,0.463 0.455,0.463 l 0.166,0 c 0.594,0 0.945,0.222 0.945,0.624 l 0,0.012 c 0,0.367 -0.282,0.599 -0.683,0.599" style="fill:url(#a)"/><path d="m 10.354,9.5342 c 0,0.876 -0.379,1.525 -0.979,1.525 -0.599,0 -0.992,-0.655 -0.992,-1.539 l 0,-0.012 c 0,-0.884 0.388,-1.527 0.979,-1.527 0.592,0 0.992,0.663 0.992,1.539 l 0,0.014 z m -0.979,-2.512 c -1.197,0 -2.008,1.097 -2.008,2.498 l 0,0.014 c 0,1.401 0.792,2.484 1.995,2.484 1.205,0 2.01,-1.097 2.01,-2.498 l 0,-0.012 c 0,-1.402 -0.805,-2.486 -1.997,-2.486" style="fill:url(#a)"/></svg>');
> 
> Nit: you could probably use a better gradient name than "a" here. 

Sure. I was trying to keep it short.  'gradient'?

> Second nit, you use rgba() colors before and here you use hexadecimals, why not rgb() to match the previous style? This applies to other SVG gradients defined throughout this file.

Sure thing.  (Jer runs off and writes a python script.)

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:399
> > +    /* Slider thumbs are small, so forcing a compositing layer is inexpensive,
> > +       and it keeps the slider from having to repaint while sliding. */
> > +    -webkit-transform: translateY(1px) rotateZ(-45deg);
> 
> I think the comment could point that it's forced into a compositing layer due to the use of rotateZ(). This may not be obvious to everyone.

Sure.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:502
> > +    -webkit-align-items: flex-start !important;
> > +    -webkit-justify-content: flex-end !important;
> 
> This is kind of a nit, and I don't think we have project-wide guidelines about this, but I think it would be nice for maintenance concerns to point out why we're using an !important here, ie. point to the rule that's being overridden here, if known. This goes for any other !important in this patch.

I'll add a note at the top of the "full screen" section; something like "Page sheets are not allowed to override the styles of full screen media elements. All the following rules will include a !important suffix to enforce this policy."

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:566
> > +    /* Slider thumbs are small, so forcing a compositing layer is inexpensive,
> > +       and it keeps the slider from having to repaint while sliding. */
> > +    -webkit-transform: rotateZ(270deg);
> 
> See previous comment about why the compositing layer is forced.
> 
> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.css:597
> > +    position: absolute;
> > +
> > +
> 
> Why two empty lines?

Because three was too many? :)

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:63
> > +    HIDE_CONTROLS_DELAY: 4 * 1000,
> > +    REWIND_AMOUNT: 30 * 1000,
> > +    MAXIMUM_SEEK_RATE: 8,
> > +    SEEK_DELAY: 1500,
> 
> Just a quick style note, in Web Inspector code we don't use THIS_TYPE_OF_CASING for constants, but rather use ThisTypeOfCasing.

I prefer CamelCase.  Dino?

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:65
> > +    addListeners: function addListeners()
> 
> How come all methods are named this way? It seems unnecessary to me, why not drop the method name after the `function` keyword?

I thought that if you dropped the name, the debugger will label those functions incorrectly in the stack trace.  Turns out, not so.  I'll de-duplicate the naming.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:68
> > +            this.video.addEventListener(name, this, false);
> 
> In Web Inspector code we would not have the last parameter for addEventListener/removeEventListener if it's `false`.

That makes sense.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:79
> > +            this.video.removeEventListener(name, this, false);
> 
> See previous comment about the last `false` attribute.
> 
> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:89
> > +        try {
> 
> Not a big fan of this massive try/catch. What's the purpose here? Do you know what kind of situations may yield an exception? Shouldn't those exceptions be handled explicitly? You may also use console.assert() too.

See comment #11.  It keeps us from throwing ASSERTs in unrelated code.

Why console.assert()?

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:172
> > +            if (window.console)
> 
> Under what conditions would there not be a `window.console` object?

No idea.  Ask Dino, who asked me to add this.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:179
> > +        this.base = document.createElement('div');
> 
> Since you do that in createControls, why not assign to a `var base` here too and save some property lookups?

Because it's quite a bit shorter. I'll add it though.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:379
> > +    handleProgress: function handleProgress(event)
> > +    {
> > +    },
> 
> This should either have a comment and/or FIXME to explain why it's there or be removed.

I'll just remove them.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:411
> > +    handlePlaying: function handlePlaying(event)
> > +    {
> > +    },
> 
> This should either have a comment and/or FIXME to explain why it's there or be removed.

Ditto.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:425
> > +    handleRateChange: function handleRateChange(event)
> > +    {
> > +    },
> 
> This should either have a comment and/or FIXME to explain why it's there or be removed.

Ditto.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:453
> > +        var track = event.track;
> > +        track.removeEventListener('cuechange', this, false);
> 
> Looks like you could get rid of the `track` var here since it's only used once.

Removed.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:548
> > +        var percent = (event.offsetX - this.controls.timeline.offsetLeft) / this.controls.timeline.offsetWidth;
> 
> I'm not totally sure what offsetX is since I don't think it's part of any DOM standard, but this doesn't look like this would work well with transforms. If you want to obtain the coordinate of the mouse event local to the element, you should first transform the mouse coordinate to the element's local coordinate system and then work out where you are based on the untransformed width of the element.

offsetX is based on the target element's border box. But you can easily calculate this with getBoundingClientRect() and clientX.  I'll change this.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:740
> > +    updateReadyState: function updateReadyState()
> > +    {
> > +        if (this.video.readyState > HTMLMediaElement.HAVE_NOTHING)
> > +            this.setStatusHidden(true);
> > +        else
> > +            this.setStatusHidden(false);
> > +    },
> 
> What about a one-liner?
> 
> this.setStatusHidden(this.video.readyState > HTMLMediaElement.HAVE_NOTHING);

Sure.

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:753
> > +        if (hidden) {
> > +            this.controls.statusDisplay.classList.add('hidden');
> > +            this.controls.currentTime.classList.remove('hidden');
> > +            this.controls.timeline.classList.remove('hidden');
> > +            this.controls.remainingTime.classList.remove('hidden');
> > +        } else {
> > +            this.controls.statusDisplay.classList.remove('hidden');
> > +            this.controls.currentTime.classList.add('hidden');
> > +            this.controls.timeline.classList.add('hidden');
> > +            this.controls.remainingTime.classList.add('hidden');
> 
> Sounds to me like the "hidden" CSS class name should be a constant. In inspector code we always have CSS class names be constants, that way you can easily tell what CSS classes are set by a given piece of script.

I don't see how this helps.  Is this just because it's at the top of the file?  

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:823
> > +            menuItem.addEventListener('click', this.captionItemSelected.bind(this), false);
> 
> I wish we could just pass `this` as the event handler there and distinguish menu items from other clickable items with a CSS class name maybe?

We could do a crazy registration structure:

this.listeners = {
    click: [
       { element:panel handler:handlePanelClick },
    ],
    ...
}

using:

function addListener(eventName, element, handler, useCapture) {
    this.listeners[eventName] = {element:element, handler:handler};
    element.addEventListener(eventName, this, useCapture);
}

function removeListener(eventName, element, handler, useCapture) {
    this.listeners[eventName] = this.listeners[eventName].filter(function(entry) { return entry.element !== element; });
    element.removeEventListener(eventName, this, useCapture);
}

and then:

listeners[event.name].forEach(function(entry){
    if (entry.element == event.currentTarget && typeof(entry.handler) == 'function')
        entry.handler.call(event, this);
}, this);

> > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:854
> > +        var item = event.target;
> > +        this.host.setSelectedTextTrack(item.track);
> 
> You can lose the `item` var here.

Ok.

[Antoine Quint]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 210935 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=210935&action=review
> > Nit: you could probably use a better gradient name than "a" here. 
> 
> Sure. I was trying to keep it short.  'gradient'?

Sure. It's a nit anyway.

> > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:65
> > > +    addListeners: function addListeners()
> > 
> > How come all methods are named this way? It seems unnecessary to me, why not drop the method name after the `function` keyword?
> 
> I thought that if you dropped the name, the debugger will label those functions incorrectly in the stack trace.  Turns out, not so.  I'll de-duplicate the naming.

Cool. Additionally, you have full control about the naming of the method in the inspector via the `displayName` property of the function object (not that it's necessary here).

> > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:79
> > > +            this.video.removeEventListener(name, this, false);
> > 
> > See previous comment about the last `false` attribute.
> > 
> > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:89
> > > +        try {
> > 
> > Not a big fan of this massive try/catch. What's the purpose here? Do you know what kind of situations may yield an exception? Shouldn't those exceptions be handled explicitly? You may also use console.assert() too.
> 
> See comment #11.  It keeps us from throwing ASSERTs in unrelated code.

OK. Then a comment would be nice here so that it's clear why it's needed.

> > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:172
> > > +            if (window.console)
> > 
> > Under what conditions would there not be a `window.console` object?
> 
> No idea.  Ask Dino, who asked me to add this.

Dino must know something I don't.

> > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:179
> > > +        this.base = document.createElement('div');
> > 
> > Since you do that in createControls, why not assign to a `var base` here too and save some property lookups?
> 
> Because it's quite a bit shorter. I'll add it though.

It will make your code more consistent and slightly more efficient.

> > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:548
> > > +        var percent = (event.offsetX - this.controls.timeline.offsetLeft) / this.controls.timeline.offsetWidth;
> > 
> > I'm not totally sure what offsetX is since I don't think it's part of any DOM standard, but this doesn't look like this would work well with transforms. If you want to obtain the coordinate of the mouse event local to the element, you should first transform the mouse coordinate to the element's local coordinate system and then work out where you are based on the untransformed width of the element.
> 
> offsetX is based on the target element's border box. But you can easily calculate this with getBoundingClientRect() and clientX.  I'll change this.

Make sure you test with a transformed <video> (rotated for example).

> > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:753
> > > +        if (hidden) {
> > > +            this.controls.statusDisplay.classList.add('hidden');
> > > +            this.controls.currentTime.classList.remove('hidden');
> > > +            this.controls.timeline.classList.remove('hidden');
> > > +            this.controls.remainingTime.classList.remove('hidden');
> > > +        } else {
> > > +            this.controls.statusDisplay.classList.remove('hidden');
> > > +            this.controls.currentTime.classList.add('hidden');
> > > +            this.controls.timeline.classList.add('hidden');
> > > +            this.controls.remainingTime.classList.add('hidden');
> > 
> > Sounds to me like the "hidden" CSS class name should be a constant. In inspector code we always have CSS class names be constants, that way you can easily tell what CSS classes are set by a given piece of script.
> 
> I don't see how this helps.  Is this just because it's at the top of the file?  

Well, in truth, you're creating a different string for each of these calls when it should be a constant. Anyway, that's how we do things in the Web Inspector code, it's cleaner. It's not a show-stopper obviously.

> > > Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:823
> > > +            menuItem.addEventListener('click', this.captionItemSelected.bind(this), false);
> > 
> > I wish we could just pass `this` as the event handler there and distinguish menu items from other clickable items with a CSS class name maybe?
> 
> We could do a crazy registration structure:
> 
> this.listeners = {
>     click: [
>        { element:panel handler:handlePanelClick },
>     ],
>     ...
> }
> 
> using:
> 
> function addListener(eventName, element, handler, useCapture) {
>     this.listeners[eventName] = {element:element, handler:handler};
>     element.addEventListener(eventName, this, useCapture);
> }
> 
> function removeListener(eventName, element, handler, useCapture) {
>     this.listeners[eventName] = this.listeners[eventName].filter(function(entry) { return entry.element !== element; });
>     element.removeEventListener(eventName, this, useCapture);
> }
> 
> and then:
> 
> listeners[event.name].forEach(function(entry){
>     if (entry.element == event.currentTarget && typeof(entry.handler) == 'function')
>         entry.handler.call(event, this);
> }, this);

Or as a one-off just for the click handler, you could just have a manual check. Your call.

[Jer Noble]

Created attachment 211474 [details]
Patch

Address Antoine's comments and add a new listener registration system which eliminates the giant switch statement.

[Antoine Quint]

(In reply to comment #28)
> Created an attachment (id=211474) [details]
> Patch
> 
> Address Antoine's comments and add a new listener registration system which eliminates the giant switch statement.

Looks good! I'd like someone else to go through the C++ parts before this gets r+'d.

[Build Bot]

Comment on attachment 211474 [details]
Patch

Attachment 211474 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1868105

New failing tests:
media/track/track-automatic-subtitles.html
webarchive/loading/video-in-webarchive.html
media/audio-controls-rendering.html
media/track/media-element-enqueue-event-crash.html
media/media-volume-slider-rendered-normal.html
js/array-with-double-assign.html
http/tests/inspector/appcache/appcache-manifest-with-non-existing-file.html
media/track/track-css-cue-lifetime.html
media/controls-styling-strict.html
media/track/track-css-matching-lang.html
media/controls-without-preload.html
media/video-controls-captions-trackmenu-hide-on-click-outside.html
media/track/track-active-cues.html
media/video-controls-captions-trackmenu-includes-enabled-track.html
media/track/add-and-remove-track.html
media/media-volume-slider-rendered-below.html
media/click-volume-bar-not-pausing.html
fast/layers/video-layer.html
accessibility/media-element.html
media/media-controls-clone.html
media/track/track-css-matching-default.html
media/media-captions-no-controls.html
media/video-controls-captions-trackmenu-hide-on-click.html
media/controls-strict.html
fullscreen/video-controls-override.html
media/audio-delete-while-slider-thumb-clicked.html
fast/hidpi/video-controls-in-hidpi.html
js/array-with-double-push.html
media/controls-after-reload.html
http/tests/media/video-redirect.html

[Build Bot]

Created attachment 211551 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Build Bot]

Comment on attachment 211474 [details]
Patch

Attachment 211474 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1903007

New failing tests:
media/track/track-automatic-subtitles.html
webarchive/loading/video-in-webarchive.html
media/audio-controls-rendering.html
fast/hidpi/video-controls-in-hidpi.html
media/media-volume-slider-rendered-normal.html
media/track/track-css-matching-timestamps.html
media/track/track-css-cue-lifetime.html
media/track/track-css-matching-default.html
media/controls-styling-strict.html
media/track/track-css-matching-lang.html
media/controls-without-preload.html
media/video-controls-captions-trackmenu-hide-on-click-outside.html
media/track/track-active-cues.html
media/video-controls-captions-trackmenu-includes-enabled-track.html
media/track/add-and-remove-track.html
media/media-volume-slider-rendered-below.html
media/click-volume-bar-not-pausing.html
accessibility/media-element.html
media/track/track-css-matching.html
media/media-controls-clone.html
fast/layers/video-layer.html
media/media-captions-no-controls.html
media/video-controls-captions-trackmenu-hide-on-click.html
media/controls-strict.html
media/track/media-element-enqueue-event-crash.html
fullscreen/video-controls-override.html
media/audio-delete-while-slider-thumb-clicked.html
media/track/track-css-property-whitelist.html
media/controls-after-reload.html
http/tests/media/video-redirect.html

[Build Bot]

Created attachment 211557 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Dean Jackson]

Comment on attachment 211474 [details]
Patch

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

Only looking over the C++ stuff here, after previous reviews.

> Source/WebCore/DerivedSources.make:858
> +
> +

Nit: extra blank line

> Source/WebCore/html/HTMLMediaElement.cpp:5219
> +    if (overlayValue.isUndefinedOrNull())
> +        return;

Why don't you do the same as above and test if (!overlayValue.isFunction())) ?

It would be nice if we could share the code to get the DOMWrapperWorld, ScriptController, JSDOMGlobalObject, ExecState and overlayValue.

> Source/WebCore/html/HTMLMediaElement.h:53
>  #include "VideoTrack.h"
>  #endif
>  
> +
> +
>  namespace WebCore {

??? Intentional?

[Jer Noble]

Committed r156546: <http://trac.webkit.org/changeset/156546>

(In reply to comment #35)
> Committed r156546: <http://trac.webkit.org/changeset/156546>

This caused bug 132531.