Bug 64428

Summary:

[EFL] Add fullscreen button to media control UI

Product:

WebKit

Reporter:

Gyuyoung Kim <gyuyoung.kim>

Component:

WebKit EFL

Assignee:

Gyuyoung Kim <gyuyoung.kim>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Proposed Patch	none
Modified Patch	none
Patch	none

Gyuyoung Kim

Reported 2011-07-12 20:23:29 PDT

Implement paintMediaFullscreenButton and Emit fullscreen signal in addition to implement functions for full screen in ChromeClientEfl.

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Gyuyoung Kim

Comment 1 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.

Raphael Kubo da Costa (:rakuco)

Comment 2 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?

Gyuyoung Kim

Comment 3 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.

Raphael Kubo da Costa (:rakuco)

Comment 4 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).

Gyuyoung Kim

Comment 5 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.

Gyuyoung Kim

Comment 6 2011-07-17 22:46:33 PDT

Could you anyone review this patch?

Raphael Kubo da Costa (:rakuco)

Comment 7 2011-07-18 06:01:18 PDT

Informal r+ from my side.

WebKit Review Bot

Comment 8 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>

WebKit Review Bot

Comment 9 2011-08-02 08:09:05 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.