Summary: | [EFL] Add seek forward / backword buttons to MediaControl UI | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||||
Component: | WebKit EFL | Assignee: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | commit-queue, eric, kenneth, leandro, lucas.de.marchi, tonikitoo | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | Linux | ||||||||||||||||
Bug Depends on: | 55463 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Gyuyoung Kim
2011-03-22 01:46:56 PDT
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. |