Bug 160092 – Media controls should not be displayed for a video until it starts playing

• Component: Media
• Status: RESOLVED FIXED

[Wenson Hsieh]

Media controls should not be displayed on pages with multiple videos until a video starts playing

[Wenson Hsieh]

<rdar://problem/26986673>

[Wenson Hsieh]

Created attachment 284361 [details]
Patch

[Wenson Hsieh]

Created attachment 284364 [details]
Patch

[Darin Adler]

Comment on attachment 284364 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:7149
> +    size_t numberOfSessionsLargeEnoughForMainContent = PlatformMediaSessionManager::sharedManager().sessionsMatching([] (const PlatformMediaSession& session) {

I suggest auto here instead of size_t.

> Source/WebCore/html/HTMLMediaElement.cpp:7153
> +    PlatformMediaSession* session = PlatformMediaSessionManager::sharedManager().currentSessionMatching([numberOfSessionsLargeEnoughForMainContent] (const PlatformMediaSession& session) {

I suggest auto* here instead of a specific type.

> Source/WebCore/html/HTMLMediaElement.cpp:7154
> +        MediaElementSession::ShowControlsManagerReason reasonForControlsManager = session.reasonForControllingControlsManager();

I think auto would make this line of code shorter without loss of readability.

> Source/WebCore/platform/audio/PlatformMediaSessionManager.cpp:292
> +    Vector<PlatformMediaSession *> matchingSessions;

Extra space here before the "*".

[Wenson Hsieh]

Comment on attachment 284364 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.cpp:7149
>> +    size_t numberOfSessionsLargeEnoughForMainContent = PlatformMediaSessionManager::sharedManager().sessionsMatching([] (const PlatformMediaSession& session) {
> 
> I suggest auto here instead of size_t.

Got it! Changed to use auto.

>> Source/WebCore/html/HTMLMediaElement.cpp:7153
>> +    PlatformMediaSession* session = PlatformMediaSessionManager::sharedManager().currentSessionMatching([numberOfSessionsLargeEnoughForMainContent] (const PlatformMediaSession& session) {
> 
> I suggest auto* here instead of a specific type.

Will do.

>> Source/WebCore/html/HTMLMediaElement.cpp:7154
>> +        MediaElementSession::ShowControlsManagerReason reasonForControlsManager = session.reasonForControllingControlsManager();
> 
> I think auto would make this line of code shorter without loss of readability.

Will use auto for this too.

>> Source/WebCore/platform/audio/PlatformMediaSessionManager.cpp:292
>> +    Vector<PlatformMediaSession *> matchingSessions;
> 
> Extra space here before the "*".

Good catch!

[Wenson Hsieh]

Created attachment 284517 [details]
Patch

[Wenson Hsieh]

After talking with Beth, I've learned that we should not be showing media controls for large videos before they've ever begun playing. This (much simpler) patch implements that behavior.

[Beth Dakin]

Comment on attachment 284517 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/VideoControlsManager.mm:181
> +    __block bool isDoneLoading = NO;

Should be BOOL since it's NO?

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/VideoControlsManager.mm:205
> +    __block bool isDoneLoading = NO;

BOOL?

[Wenson Hsieh]

Comment on attachment 284517 [details]
Patch

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

>> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/VideoControlsManager.mm:181
>> +    __block bool isDoneLoading = NO;
> 
> Should be BOOL since it's NO?

Oops, good catch! It should be false instead of NO, since TestWebKitAPI::Util::run() takes a bool*.

>> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/VideoControlsManager.mm:205
>> +    __block bool isDoneLoading = NO;
> 
> BOOL?

Fixed!

[Wenson Hsieh]

Created attachment 284519 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 284519 [details]
Patch for landing

Clearing flags on attachment: 284519

Committed r203698: <http://trac.webkit.org/changeset/203698>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.