Bug 104443 - [BlackBerry] Update Media Controls for BlackBerry Platform
Summary: [BlackBerry] Update Media Controls for BlackBerry Platform
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Griggs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-08 09:19 PST by John Griggs
Modified: 2012-12-12 14:19 PST (History)
15 users (show)

See Also:


Attachments
Patch (110.91 KB, patch)
2012-12-11 10:48 PST, John Griggs
no flags Details | Formatted Diff | Diff
Patch (109.62 KB, patch)
2012-12-11 12:56 PST, John Griggs
no flags Details | Formatted Diff | Diff
Patch (107.84 KB, patch)
2012-12-11 13:50 PST, John Griggs
no flags Details | Formatted Diff | Diff
Patch (107.84 KB, patch)
2012-12-12 07:55 PST, John Griggs
no flags Details | Formatted Diff | Diff
Patch (98.51 KB, patch)
2012-12-12 09:02 PST, John Griggs
no flags Details | Formatted Diff | Diff
Patch (96.83 KB, patch)
2012-12-12 12:58 PST, John Griggs
no flags Details | Formatted Diff | Diff
Patch (110.98 KB, patch)
2012-12-12 13:40 PST, John Griggs
no flags Details | Formatted Diff | Diff
Patch (105.62 KB, patch)
2012-12-12 13:54 PST, John Griggs
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Griggs 2012-12-08 09:19:53 PST
Update the media controls on the BlackBerry platform to allow separate styling of audio, embedded video and full screen video elements.
Comment 1 John Griggs 2012-12-11 10:48:32 PST
Created attachment 178835 [details]
Patch
Comment 2 John Griggs 2012-12-11 12:56:29 PST
Created attachment 178856 [details]
Patch
Comment 3 John Griggs 2012-12-11 12:57:16 PST
Comment on attachment 178856 [details]
Patch

Updated patch.
Comment 4 Rob Buis 2012-12-11 13:28:36 PST
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 5 John Griggs 2012-12-11 13:47:11 PST
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.
Comment 6 John Griggs 2012-12-11 13:50:23 PST
Created attachment 178866 [details]
Patch
Comment 7 John Griggs 2012-12-11 13:51:32 PST
Comment on attachment 178866 [details]
Patch

Fixes items from review except for those explained in my previous comment.
Comment 8 Rob Buis 2012-12-11 14:12:21 PST
Comment on attachment 178866 [details]
Patch

LGTM.
Comment 9 WebKit Review Bot 2012-12-11 14:16:49 PST
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
Comment 10 John Griggs 2012-12-12 07:55:03 PST
Created attachment 179044 [details]
Patch
Comment 11 John Griggs 2012-12-12 07:55:40 PST
Comment on attachment 179044 [details]
Patch

Updated to fix a one line error that was preventing the patch from applying cleanly.
Comment 12 Rob Buis 2012-12-12 07:56:59 PST
Comment on attachment 179044 [details]
Patch

Retrying.
Comment 13 WebKit Review Bot 2012-12-12 07:59:32 PST
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
Comment 14 John Griggs 2012-12-12 09:02:09 PST
Created attachment 179061 [details]
Patch
Comment 15 John Griggs 2012-12-12 09:04:21 PST
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 16 Rob Buis 2012-12-12 09:34:49 PST
Comment on attachment 179061 [details]
Patch

LGTM.
Comment 17 Eric Carlson 2012-12-12 09:55:45 PST
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 18 Antonio Gomes 2012-12-12 10:44:37 PST
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.
Comment 19 John Griggs 2012-12-12 12:58:33 PST
Created attachment 179108 [details]
Patch
Comment 20 John Griggs 2012-12-12 12:59:28 PST
Comment on attachment 179108 [details]
Patch

Latest patch removes #if PLATFORM(BLACKBERRY) from shared code.
Comment 21 John Griggs 2012-12-12 13:40:26 PST
Created attachment 179119 [details]
Patch
Comment 22 John Griggs 2012-12-12 13:42:18 PST
Comment on attachment 179119 [details]
Patch

Updated patch removes #if PLATFORM(BLACKBERRY) blocks from common code.
Comment 23 Rob Buis 2012-12-12 13:43:50 PST
Comment on attachment 179119 [details]
Patch

LGTM.
Comment 24 Antonio Gomes 2012-12-12 13:46:32 PST
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
Comment 25 John Griggs 2012-12-12 13:52:48 PST
(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.
Comment 26 John Griggs 2012-12-12 13:54:34 PST
Created attachment 179121 [details]
Patch
Comment 27 John Griggs 2012-12-12 13:55:02 PST
Comment on attachment 179121 [details]
Patch

Fixed ChangeLog issues
Comment 28 Rob Buis 2012-12-12 13:58:42 PST
Comment on attachment 179121 [details]
Patch

Retrying.
Comment 29 WebKit Review Bot 2012-12-12 14:19:32 PST
Comment on attachment 179121 [details]
Patch

Clearing flags on attachment: 179121

Committed r137514: <http://trac.webkit.org/changeset/137514>
Comment 30 WebKit Review Bot 2012-12-12 14:19:39 PST
All reviewed patches have been landed.  Closing bug.