[webkit-reviews] review denied: [Bug 133521] iOS8 media controls look different from iOS7 media controls : [Attachment 232489] Patch

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?