Bug 138493 - [iOS] Update optimized fullscreen media controls
Summary: [iOS] Update optimized fullscreen media controls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-06 21:05 PST by Eric Carlson
Modified: 2015-05-04 11:13 PDT (History)
1 user (show)

See Also:


Attachments
Proposed patch. (20.47 KB, patch)
2014-11-06 21:26 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Updated patch (20.42 KB, patch)
2014-11-07 08:17 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Updated patch (20.42 KB, patch)
2014-11-07 08:49 PST, Eric Carlson
bfulgham: review+
Details | Formatted Diff | Diff
Patch for landing. (20.56 KB, patch)
2014-11-07 09:26 PST, Eric Carlson
eric.carlson: commit-queue+
Details | Formatted Diff | Diff
Patch for landing. (20.58 KB, patch)
2014-11-07 09:28 PST, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2014-11-06 21:05:58 PST
Update the artwork and logic.
Comment 1 Eric Carlson 2014-11-06 21:26:13 PST
Created attachment 241161 [details]
Proposed patch.
Comment 2 WebKit Commit Bot 2014-11-06 21:28:40 PST
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.
Comment 3 Eric Carlson 2014-11-07 08:17:28 PST
Created attachment 241184 [details]
Updated patch
Comment 4 WebKit Commit Bot 2014-11-07 08:18:51 PST
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.
Comment 5 Eric Carlson 2014-11-07 08:49:36 PST
Created attachment 241185 [details]
Updated patch
Comment 6 WebKit Commit Bot 2014-11-07 08:52:19 PST
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.
Comment 7 Brent Fulgham 2014-11-07 09:01:50 PST
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?
Comment 8 Eric Carlson 2014-11-07 09:19:22 PST
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.
Comment 9 Eric Carlson 2014-11-07 09:26:06 PST
Created attachment 241186 [details]
Patch for landing.
Comment 10 Eric Carlson 2014-11-07 09:28:19 PST
Created attachment 241187 [details]
Patch for landing.
Comment 11 WebKit Commit Bot 2014-11-07 10:10:49 PST
Comment on attachment 241187 [details]
Patch for landing.

Clearing flags on attachment: 241187

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