Bug 56810

Created attachment 86440 [details] Proposed Patch

Created attachment 86442 [details] Screen Capture - Seek Button

Created attachment 87024 [details] Modified Patch Demarchi, I make new patch. Please review.

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.

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.

Comment on attachment 87453 [details] Modified Patch LGTM

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

(In reply to comment #6) > (From update of attachment 87453 [details]) > LGTM Hello Antonio, Please review this patch. :-)

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?

Comment on attachment 87453 [details] Modified Patch Please check the types, since clearly in an earlier version of the patch we got them wrong. :)

(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

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

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 ?

Comment on attachment 88769 [details] Modified Patch Clearing flags on attachment: 88769 Committed r83410: <http://trac.webkit.org/changeset/83410>

All reviewed patches have been landed. Closing bug.