Update the media controls on the BlackBerry platform to allow separate styling of audio, embedded video and full screen video elements.
Created attachment 178835 [details] Patch
Created attachment 178856 [details] Patch
Comment on attachment 178856 [details] Patch Updated patch.
Comment on attachment 178856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178856&action=review Needs some more iterations. > Source/WebCore/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). Any PR to mention? > Source/WebCore/ChangeLog:203 > + (WebCore::RenderThemeBlackBerry::supportsDataListUI): Need to regenerate your ChangeLog :) > Source/WebCore/PlatformBlackBerry.cmake:8 > +) Why remove this? What is the backstory? > Source/WebCore/html/shadow/MediaControlElementTypes.cpp:77 > +#if !PLATFORM(BLACKBERRY) Is there a cleaner way? Do we want VIDEO_TRACK in the first place? If yes then why not this part? > Source/WebCore/platform/blackberry/RenderThemeBlackBerry.cpp:420 > + } This is not relevant to your change, please remove. > Source/WebCore/platform/blackberry/RenderThemeBlackBerry.cpp:456 > + return false; Ditto.
Comment on attachment 178856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178856&action=review >> Source/WebCore/PlatformBlackBerry.cmake:8 >> +) > > Why remove this? What is the backstory? These are the media controls for Apple platforms - they are replaced by our controls in MediaControlsBlackBerry.cpp. Both of these files extend the code in MediaControls.cpp >> Source/WebCore/html/shadow/MediaControlElementTypes.cpp:77 >> +#if !PLATFORM(BLACKBERRY) > > Is there a cleaner way? Do we want VIDEO_TRACK in the first place? If yes then why not this part? The VIDEO_TRACK feature is apparently required (according to Eli), but the new Media Control changes from WebKit seem to depend on some rendering code for VIDEO_TRACK that we do not have yet. I cannot turn off VIDEO_TRACK entirely, so this is the easiest way to avoid chasing the rendering changes and get these media control changes in.
Created attachment 178866 [details] Patch
Comment on attachment 178866 [details] Patch Fixes items from review except for those explained in my previous comment.
Comment on attachment 178866 [details] Patch LGTM.
Comment on attachment 178866 [details] Patch Rejecting attachment 178866 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: -1 lines). Hunk #15 succeeded at 1061 (offset -1 lines). Hunk #16 succeeded at 1100 (offset -1 lines). Hunk #17 succeeded at 1134 (offset -1 lines). 1 out of 17 hunks FAILED -- saving rejects to file Source/WebCore/platform/blackberry/RenderThemeBlackBerry.cpp.rej patching file Source/WebCore/platform/blackberry/RenderThemeBlackBerry.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Rob Buis']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/15277298
Created attachment 179044 [details] Patch
Comment on attachment 179044 [details] Patch Updated to fix a one line error that was preventing the patch from applying cleanly.
Comment on attachment 179044 [details] Patch Retrying.
Comment on attachment 179044 [details] Patch Rejecting attachment 179044 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: -1 lines). Hunk #15 succeeded at 1061 (offset -1 lines). Hunk #16 succeeded at 1100 (offset -1 lines). Hunk #17 succeeded at 1134 (offset -1 lines). 1 out of 17 hunks FAILED -- saving rejects to file Source/WebCore/platform/blackberry/RenderThemeBlackBerry.cpp.rej patching file Source/WebCore/platform/blackberry/RenderThemeBlackBerry.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Rob Buis']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/15281533
Created attachment 179061 [details] Patch
Comment on attachment 179061 [details] Patch Another patch to attempt to get past the errors applying the patch. It looks like there were two conflicts - one in RenderThemeBlackBerry.cpp and one in Source/WebCore/ChangeLog. both are addressed in this patch.
Comment on attachment 179061 [details] Patch LGTM.
Comment on attachment 179061 [details] Patch Silvia just refactored media control code to get rid of as many "#if PLATFORM()" in shared code as possible. I would *really* rather not have a bunch of "#if PLATFORM(BLACKBERRY)" in MediaControlElements.cpp, can you do this another way (eg. subclass the controls you want to change)?
Comment on attachment 179061 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179061&action=review Agree with Eric. > Source/WebCore/html/shadow/MediaControlElements.cpp:440 > +#if PLATFORM(BLACKBERRY) > + if (event->type() == eventNames().mousedownEvent) { > + // We do not mute when the media player volume/mute control is touched > + // on Playbook. Instead we show/hide the volume slider. > + m_controls->toggleVolumeSlider(); > + event->setDefaultHandled(); > + return; > + } > +#endif this can be a setting at least, and have its own bug.
Created attachment 179108 [details] Patch
Comment on attachment 179108 [details] Patch Latest patch removes #if PLATFORM(BLACKBERRY) from shared code.
Created attachment 179119 [details] Patch
Comment on attachment 179119 [details] Patch Updated patch removes #if PLATFORM(BLACKBERRY) blocks from common code.
Comment on attachment 179119 [details] Patch LGTM.
Comment on attachment 179119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179119&action=review Maybe this patch should be split into smaller ones? > Source/WebCore/ChangeLog:13 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). > + > + No new tests (OOPS!). these should not be here
(In reply to comment #24) > (From update of attachment 179119 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179119&action=review > > Maybe this patch should be split into smaller ones? > > > Source/WebCore/ChangeLog:13 > > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). > > + > > + No new tests (OOPS!). > > these should not be here Antonio, I am SOOO close and under some time pressure. I would really appreciate not having to break this patch up. I have fixed the ChangeLog issues (which I thought I had already squashed) in a new patch - uploading now.
Created attachment 179121 [details] Patch
Comment on attachment 179121 [details] Patch Fixed ChangeLog issues
Comment on attachment 179121 [details] Patch Retrying.
Comment on attachment 179121 [details] Patch Clearing flags on attachment: 179121 Committed r137514: <http://trac.webkit.org/changeset/137514>
All reviewed patches have been landed. Closing bug.