Bug 104443 – [BlackBerry] Update Media Controls for BlackBerry Platform

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[John Griggs]

Update the media controls on the BlackBerry platform to allow separate styling of audio, embedded video and full screen video elements.

[John Griggs]

Created attachment 178835 [details]
Patch

[John Griggs]

Created attachment 178856 [details]
Patch

[John Griggs]

Comment on attachment 178856 [details]
Patch

Updated patch.

[Rob Buis]

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.

[John Griggs]

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.

[John Griggs]

Created attachment 178866 [details]
Patch

[John Griggs]

Comment on attachment 178866 [details]
Patch

Fixes items from review except for those explained in my previous comment.

[Rob Buis]

Comment on attachment 178866 [details]
Patch

LGTM.

[WebKit Review Bot]

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

[John Griggs]

Created attachment 179044 [details]
Patch

[John Griggs]

Comment on attachment 179044 [details]
Patch

Updated to fix a one line error that was preventing the patch from applying cleanly.

[Rob Buis]

Comment on attachment 179044 [details]
Patch

Retrying.

[WebKit Review Bot]

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

[John Griggs]

Created attachment 179061 [details]
Patch

[John Griggs]

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.

[Rob Buis]

Comment on attachment 179061 [details]
Patch

LGTM.

[Eric Carlson]

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)?

[Antonio Gomes]

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.

[John Griggs]

Created attachment 179108 [details]
Patch

[John Griggs]

Comment on attachment 179108 [details]
Patch

Latest patch removes #if PLATFORM(BLACKBERRY) from shared code.

[John Griggs]

Created attachment 179119 [details]
Patch

[John Griggs]

Comment on attachment 179119 [details]
Patch

Updated patch removes #if PLATFORM(BLACKBERRY) blocks from common code.

[Rob Buis]

Comment on attachment 179119 [details]
Patch

LGTM.

[Antonio Gomes]

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

[John Griggs]

(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.

[John Griggs]

Created attachment 179121 [details]
Patch

[John Griggs]

Comment on attachment 179121 [details]
Patch

Fixed ChangeLog issues

[Rob Buis]

Comment on attachment 179121 [details]
Patch

Retrying.

[WebKit Review Bot]

Comment on attachment 179121 [details]
Patch

Clearing flags on attachment: 179121

Committed r137514: <http://trac.webkit.org/changeset/137514>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.