[webkit-reviews] review denied: [Bug 133521] iOS8 media controls look different from iOS7 media controls : [Attachment 232489] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jun 4 14:43:01 PDT 2014
Eric Carlson <eric.carlson at apple.com> has denied Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 133521: iOS8 media controls look different from iOS7 media controls
https://bugs.webkit.org/show_bug.cgi?id=133521
Attachment 232489: Patch
https://bugs.webkit.org/attachment.cgi?id=232489&action=review
------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=232489&action=review
> Source/WebCore/ChangeLog:3
> + iOS8 media controls look different from iOS7 media controls
This describes the problem but not the solution, so please include a
description of what has changed.
> Source/WebCore/ChangeLog:20
> + (audio):
> + (::-webkit-media-controls):
> + (audio::-webkit-media-controls):
> + (audio::-webkit-media-controls-wireless-playback-picker-button):
> + (audio::-webkit-media-controls-panel):
> + (video::-webkit-media-controls-panel):
> + (audio::-webkit-media-controls-fullscreen-button):
> + (audio::-webkit-media-controls-time-remaining-display):
> + (video::-webkit-media-controls-current-time-display):
> + (video::-webkit-media-controls-time-remaining-display):
I think it is helpful for people reading the ChangeLog later to have a
description of what changed in each method/rule.
> Source/WebCore/Modules/mediacontrols/mediaControlsiOS.css:61
> + /* padding-top: 10px; */
Why is this commented out?
> LayoutTests/platform/ios-sim/media/audio-width.html:1
> +<p>Test that audio elements have a minimum width.<p>
Why is this before the <head> element?
> LayoutTests/platform/ios-sim/media/audio-width.html:10
> +<audio id="aud_1" type="audio/mp4" controls></audio>
> +<audio id="aud_2" type="audio/mp4" controls></audio>
Why is this in the <head> element? Please create a fully formed document.
I think that indentation aids readability and will help others have to have to
read and understand this test later, so please use it.
> LayoutTests/platform/ios-sim/media/audio-width.html:23
> +var width1 = width2 = NaN; // default value causes failure
WebKit style is to declare each variable on its own line, and to only include
comments when they document something that is not immediately obvious.
> LayoutTests/platform/ios-sim/media/audio-width.html:26
> + width1 = parseInt(window.getComputedStyle(player1).width); // Npx parsed
as int N
This comment doesn't add much value, you can leave it out.
> LayoutTests/platform/ios-sim/media/audio-width.html:30
> + testRunner.notifyDone();
You include video-test.js so you can call endTest() instead and automatically
get a logging that the test ended successfully.
> LayoutTests/platform/ios-sim/media/audio-width.html:38
> +var player1 = document.getElementById("aud_1"),
> + player2 = document.getElementById("aud_2");
> +if (player1 && player2) {
> + player1.src = player2.src = "../../../media/" + findMediaFile("audio",
"content/test");
> + runTest();
> +}
1 - Please put this in a function in <head> and call it in response to an event
(body.load or some-such).
2 - Please declare each variable.
3 - Is it likely that player1 or player2 will ever be null?
More information about the webkit-reviews
mailing list