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

Thu Jun 5 15:24:35 PDT 2014

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

--- Comment #4 from Wenson Hsieh <wenson_hsieh at apple.com>  2014-06-05 15:24:57 PST ---
(From update of attachment 232489)
View in context: https://bugs.webkit.org/attachment.cgi?id=232489&action=review

Thanks, uploading a new patch soon. Sorry for the lack of descriptions/information and generally messy code -- this was a work in progress, and I'll have a more recent version out very soon.

>> 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.

Expanded to include brief description of fixes.

>> Source/WebCore/ChangeLog:20
>> +        (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.

More detailed descriptions of changes added.

>> Source/WebCore/Modules/mediacontrols/mediaControlsiOS.css:61
>> +    /* padding-top: 10px; */
> 
> Why is this commented out?

Sorry about that -- removed altogether

>> 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?

Fixed.

>> LayoutTests/platform/ios-sim/media/audio-width.html:10
>> +<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.

Fixed and added indentation to the file, thanks for the tip

>> 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.

Fixed.

>> 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.

Fixed.

>> 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.

Got it - fixed.

>> LayoutTests/platform/ios-sim/media/audio-width.html:38
>> +}
> 
> 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?

Removed null check and restructured javascript.

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