Bug 138493 – [iOS] Update optimized fullscreen media controls

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

Update the artwork and logic.

[Eric Carlson]

Created attachment 241161 [details]
Proposed patch.

[WebKit Commit Bot]

Attachment 241161 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:302:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:323:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/WebCore.exp.in:0:  Source/WebCore/WebCore.exp.in should be sorted, use Tools/Scripts/sort-export-file script  [list/order] [5]
Total errors found: 3 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 241184 [details]
Updated patch

[WebKit Commit Bot]

Attachment 241184 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:318:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/WebCore.exp.in:0:  Source/WebCore/WebCore.exp.in should be sorted, use Tools/Scripts/sort-export-file script  [list/order] [5]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 241185 [details]
Updated patch

[WebKit Commit Bot]

Attachment 241185 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:318:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/WebCore.exp.in:0:  Source/WebCore/WebCore.exp.in should be sorted, use Tools/Scripts/sort-export-file script  [list/order] [5]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Comment on attachment 241185 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241185&action=review

Looks good. I had a question about the 'shouldHaveStartPlaybackButton' function, and I wanted to double-check you intended to drop opacity from 0.9 to 0.4.

> Source/WebCore/Modules/mediacontrols/MediaControlsHost.cpp:315
> +String MediaControlsHost::mediaUIImageData(String partID) const

Couldn't partID be a const reference?

> Source/WebCore/Modules/mediacontrols/mediaControlsiOS.css:250
> +    opacity: 0.4;

I'm sure this was intentional, but it wasn't mentioned in the ChangeLog. So we now have a more transparent fullscreen button (i.e., 0.4 versus 0.9)?

> Source/WebCore/Modules/mediacontrols/mediaControlsiOS.js:81
> +        if (!this.doingSetup && !this.host.userGestureRequired && allowsInline)

I'm not sure I understand this change. We only want to show the start playback button when we ARE doingSetup?

[Eric Carlson]

Comment on attachment 241185 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241185&action=review

>> Source/WebCore/Modules/mediacontrols/mediaControlsiOS.css:250
>> +    opacity: 0.4;
> 
> I'm sure this was intentional, but it wasn't mentioned in the ChangeLog. So we now have a more transparent fullscreen button (i.e., 0.4 versus 0.9)?

Yes, it is more transparent to make the new artwork look correct.

>> Source/WebCore/Modules/mediacontrols/mediaControlsiOS.js:81
>> +        if (!this.doingSetup && !this.host.userGestureRequired && allowsInline)
> 
> I'm not sure I understand this change. We only want to show the start playback button when we ARE doingSetup?

We only want to show the big overlay play button until the user initiates playback and "allowsInline" will be true when a <video> element is created in a script that is triggered by a user gesture, so we DON'T want to draw the inline controls during setup.

[Eric Carlson]

Created attachment 241186 [details]
Patch for landing.

[Eric Carlson]

Created attachment 241187 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 241187 [details]
Patch for landing.

Clearing flags on attachment: 241187

Committed r175750: <http://trac.webkit.org/changeset/175750>