[webkit-reviews] review denied: [Bug 120895] [Mac] Implement the media controls in JavaScript. : [Attachment 210825] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Sep 6 19:37:20 PDT 2013
Dean Jackson <dino at apple.com> has denied Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 120895: [Mac] Implement the media controls in JavaScript.
https://bugs.webkit.org/show_bug.cgi?id=120895
Attachment 210825: Patch
https://bugs.webkit.org/attachment.cgi?id=210825&action=review
------- Additional Comments from Dean Jackson <dino at apple.com>
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::-web
kit-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.
More information about the webkit-reviews
mailing list