Summary: | [BlackBerry] Update Media Controls for BlackBerry Platform | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Griggs <jgriggs> | ||||||||||||||||||
Component: | WebKit BlackBerry | Assignee: | John Griggs <jgriggs> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | cmarcelo, dbates, eric.carlson, feature-media-reviews, gyuyoung.kim, macpherson, menard, mfeil, mifenton, ojan.autocc, rakuco, rwlbuis, tonikitoo, webkit.review.bot, yong.li.webkit | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
John Griggs
2012-12-08 09:19:53 PST
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. |