Bug 156630

Summary: Audio elements should be able to have a controls manager.
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: New BugsAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
bdakin: review+
Patch for landing
none
Patch for landing none

Jer Noble
Reported 2016-04-15 10:51:25 PDT
Audio elements should be able to have a controls manager.
Attachments
Patch (4.95 KB, patch)
2016-04-15 10:55 PDT, Jer Noble 		bdakin: review+
DetailsFormatted DiffDiff
Patch for landing (4.75 KB, patch)
2016-04-15 11:46 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Patch for landing (4.84 KB, patch)
2016-04-15 12:01 PDT, Jer Noble 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Jer Noble
Comment 1 2016-04-15 10:51:47 PDT
rdar://problem/25537071
Jer Noble
Comment 2 2016-04-15 10:55:28 PDT
Created attachment 276487 [details] Patch
Darin Adler
Comment 3 2016-04-15 11:01:58 PDT
Comment on attachment 276487 [details] Patch Why no regression test?
Beth Dakin
Comment 4 2016-04-15 11:03:36 PDT
Comment on attachment 276487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276487&action=review > Source/WebCore/html/MediaElementSession.cpp:-234 > - if (ScriptController::processingUserGestureForMedia()) Don't we want to keep this case? Any media that a user chooses to start playing should get a controls manager, no? With this change a small video will never get a controls manager. > Source/WebCore/html/MediaElementSession.cpp:230 > + return true; Why did this switch?
Beth Dakin
Comment 5 2016-04-15 11:06:28 PDT
Comment on attachment 276487 [details] Patch Adding back Darin's r+ that I overrode with my comments. Though I would like to see those comments addressed, as we discussed on irc. :)
Darin Adler
Comment 6 2016-04-15 11:31:04 PDT
For the record, it was totally OK with me to remove my review+ and replace it with either review- or review+ of your own. Part of the informal "what happens when two people review around the same time" system.
Jer Noble
Comment 7 2016-04-15 11:39:39 PDT
Comment on attachment 276487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276487&action=review >> Source/WebCore/html/MediaElementSession.cpp:-234 >> - if (ScriptController::processingUserGestureForMedia()) > > Don't we want to keep this case? Any media that a user chooses to start playing should get a controls manager, no? With this change a small video will never get a controls manager. Sure, i'll leave this in. >> Source/WebCore/html/MediaElementSession.cpp:230 >> + return true; > > Why did this switch? We only want the width and height check if the element hasVIdeo(). Audio elements should just fall through to the "true" case. But I can move this back up to its original location.
Jer Noble
Comment 8 2016-04-15 11:46:00 PDT
Created attachment 276488 [details] Patch for landing
Jer Noble
Comment 9 2016-04-15 12:01:44 PDT
Created attachment 276489 [details] Patch for landing
Jer Noble
Comment 10 2016-04-15 12:06:25 PDT
Committed r199603: <http://trac.webkit.org/changeset/199603>
Note You need to log in before you can comment on or make changes to this bug.