Bug 64428 - [EFL] Add fullscreen button to media control UI
Summary: [EFL] Add fullscreen button to media control UI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-12 20:23 PDT by Gyuyoung Kim
Modified: 2011-08-02 08:09 PDT (History)
4 users (show)

See Also:


Attachments
Proposed Patch (10.20 KB, patch)
2011-07-12 20:26 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Modified Patch (10.20 KB, patch)
2011-07-13 18:13 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (10.14 KB, patch)
2011-07-17 21:23 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2011-07-12 20:23:29 PDT
Implement paintMediaFullscreenButton and Emit fullscreen signal in addition to implement functions for full screen in ChromeClientEfl.
Comment 1 Gyuyoung Kim 2011-07-12 20:26:17 PDT
Created attachment 100615 [details]
Proposed Patch

This patch doesn't add normal screen button yet on full screen mode. The normal screen button will be added after finishing implementation of full screen mode.
Comment 2 Raphael Kubo da Costa (:rakuco) 2011-07-13 06:23:46 PDT
Looks mostly OK to me, with a few nitpicks below:

> Source/WebCore/ChangeLog:6
> +        Implement paintMediaFullscreenButton and Emit fullscreen signal.

Emit -> emit

> Source/WebKit/efl/ChangeLog:6
> +        Implement functions for full screen in ChrmeClientEfl in order to display a full screen button on media control UI.

ChrmeClientEfl -> ChromeClientEfl

> Source/WebKit/efl/DefaultTheme/widget/mediacontrol/fullscreenbutton/fullscreen_button.edc:4
> +    Copyright (C) 2011 Samsung Electronics

Are all these copyrights really needed for such a small file?
Comment 3 Gyuyoung Kim 2011-07-13 18:13:13 PDT
Created attachment 100746 [details]
Modified Patch

I fixed wrong descriptions. 

>> Are all these copyrights really needed for such a small file?
Other .edc files have same copyrights. So, I think this file need to have same copyright as well.
Comment 4 Raphael Kubo da Costa (:rakuco) 2011-07-14 07:08:35 PDT
I Am Not A Lawyer (TM), but in theory the other files should have all those copyrights because they derive from the original work done by INdT, right? This new EDC looks like some new work whose copyright should belong only to Samsung.

As for the rest, r+ from my side (dunno if you need to consult your legal side or if it can be committed as-is).
Comment 5 Gyuyoung Kim 2011-07-17 21:23:30 PDT
Created attachment 101123 [details]
Patch

ok, I think previous copyright is unneeded. If this patch is based on other patch which has previous copyright, I think we should mention previous copyright. But, this .edc file refer to Lucas comment. So, I leave profusion copyright.
Comment 6 Gyuyoung Kim 2011-07-17 22:46:33 PDT
Could you anyone review this patch?
Comment 7 Raphael Kubo da Costa (:rakuco) 2011-07-18 06:01:18 PDT
Informal r+ from my side.
Comment 8 WebKit Review Bot 2011-08-02 08:09:00 PDT
Comment on attachment 101123 [details]
Patch

Clearing flags on attachment: 101123

Committed r92189: <http://trac.webkit.org/changeset/92189>
Comment 9 WebKit Review Bot 2011-08-02 08:09:05 PDT
All reviewed patches have been landed.  Closing bug.