[Webkit-unassigned] [Bug 133521] iOS8 media controls look different from iOS7 media controls

Wed Jun 4 14:43:02 PDT 2014

https://bugs.webkit.org/show_bug.cgi?id=133521

Eric Carlson <eric.carlson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #232489|review?                     |review-
               Flag|                            |

--- Comment #3 from Eric Carlson <eric.carlson at apple.com>  2014-06-04 14:43:25 PST ---
(From update of attachment 232489)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.