Bug 56810 – [EFL] Add seek forward / backword buttons to MediaControl UI

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Gyuyoung Kim]

Add seek forward | backword buttons to media control.

[Gyuyoung Kim]

Created attachment 86440 [details]
Proposed Patch

[Gyuyoung Kim]

Created attachment 86442 [details]
Screen Capture - Seek Button

[Gyuyoung Kim]

Created attachment 87024 [details]
Modified Patch

Demarchi, I make new patch. Please review.

[Lucas De Marchi]

Comment on attachment 87024 [details]
Modified Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87024&action=review

> Source/WebCore/ChangeLog:8
> +        [EFL] Add seek forward / backword buttons to MediaControl UI.
> +        https://bugs.webkit.org/show_bug.cgi?id=56810
> +
> +        Add seek forward / backword buttons to media control.

typo: backword

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:1139
> +    MediaControlPlayButtonElement* button = static_cast<MediaControlPlayButtonElement*>(node);

MediaControlSeekButtonElement, no?

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:1152
> +    MediaControlPlayButtonElement* button = static_cast<MediaControlPlayButtonElement*>(node);

Same here

> Source/WebKit/efl/ChangeLog:5
> +
> +        [EFL] Add seek forward / backword buttons to MediaControl UI.

typo, as above

> Source/WebKit/efl/DefaultTheme/widget/mediacontrol/seekbutton/seek_button.edc:58
> +         program {
> +             signal: "seekforward";
> +             action: STATE_SET "seekforward" 0.0;
> +             target: "seek_button";
> +         }
> +         program {
> +             signal: "seekbackward";
> +             action: STATE_SET "seekbackward" 0.0;
> +             target: "seek_button";
> +         }

Contrary to Mute/Unmute and Play/Pause, for me it seems weird to share the same button to seek forward/backward.

[Gyuyoung Kim]

Created attachment 87453 [details]
Modified Patch

Ok, I make new patch according to your comment. BTW, I change FormType names of media control in order to avoid to duplicate between FormType and MediaControlElementType.

[Lucas De Marchi]

Comment on attachment 87453 [details]
Modified Patch

LGTM

[Gyuyoung Kim]

Created attachment 88178 [details]
Current MediaControl button for EFL port

[Gyuyoung Kim]

(In reply to comment #6)
> (From update of attachment 87453 [details])
> LGTM

Hello Antonio,

Please review this patch. :-)

[Eric Seidel (no email)]

Comment on attachment 87453 [details]
Modified Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87453&action=review

Seems reasonable.

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:1140
> +    MediaControlSeekButtonElement* button = static_cast<MediaControlSeekButtonElement*>(node);

Shouldn't we check the type here?

> Source/WebCore/platform/efl/RenderThemeEfl.cpp:1153
> +    MediaControlSeekButtonElement* button = static_cast<MediaControlSeekButtonElement*>(node);

again, typecheck?

[Eric Seidel (no email)]

Comment on attachment 87453 [details]
Modified Patch

Please check the types, since clearly in an earlier version of the patch we got them wrong. :)

[Gyuyoung Kim]

(In reply to comment #10)
> (From update of attachment 87453 [details])
> Please check the types, since clearly in an earlier version of the patch we got them wrong. :)

Hello Eric,

Th

[Gyuyoung Kim]

(In reply to comment #10)
> (From update of attachment 87453 [details])
> Please check the types, since clearly in an earlier version of the patch we got them wrong. :)

Sorry, I don't understand exactly. Do you mean this patch should check if node is media element or not first ?

For example,

 Node* mediaNode = node ? node->shadowAncestorNode() : 0;
 if (!mediaNode || (!mediaNode->hasTagName(videoTag) && !mediaNode->hasTagName(audioTag)))
      return 0;

[Gyuyoung Kim]

Created attachment 88769 [details]
Modified Patch

I add type checking condition as below,

 86 +    Node* node = object->node();
 87 +    if (!node || !node->isMediaControlElement())
 88 +        return 0;

Eric, how do you think this ?

[WebKit Commit Bot]

Comment on attachment 88769 [details]
Modified Patch

Clearing flags on attachment: 88769

Committed r83410: <http://trac.webkit.org/changeset/83410>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.