Bug 85870

Summary:

[EFL] Enable Fullscreen API

Product:

WebKit

Reporter:

Chris Dumez <cdumez>

Component:

WebKit EFL

Assignee:

Chris Dumez <cdumez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

d-r, gyuyoung.kim, kihong.kwon, lucas.de.marchi, rakuco, vimff0, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

86185, 86187

Attachments:

Description	Flags
Patch.	none
Patch.	none
Patch.	none
Patch.	none
Patch.	none
Patch.	tonikitoo: review+
Patch for landing.	none

Chris Dumez

Reported 2012-05-08 00:28:05 PDT

The Full Screen API is disabled on EFL port but enabled on all others. It is causing the following test case to fail: media/video-play-require-user-gesture.html

Attachments
Patch. (4.97 KB, patch) 2012-05-08 18:28 PDT, Kihong Kwon	no flags	Details Formatted Diff Diff
Patch. (7.73 KB, patch) 2012-05-10 03:24 PDT, Kihong Kwon	no flags	Details Formatted Diff Diff
Patch. (8.19 KB, patch) 2012-05-10 03:52 PDT, Kihong Kwon	no flags	Details Formatted Diff Diff
Patch. (13.84 KB, patch) 2012-05-11 01:37 PDT, Kihong Kwon	no flags	Details Formatted Diff Diff
Patch. (13.83 KB, patch) 2012-05-11 03:13 PDT, Kihong Kwon	no flags	Details Formatted Diff Diff
Patch. (85.11 KB, patch) 2012-05-11 03:23 PDT, Kihong Kwon	tonikitoo: review+	Details Formatted Diff Diff
Patch for landing. (84.82 KB, patch) 2012-05-15 01:42 PDT, Kihong Kwon	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Kihong Kwon

Comment 1 2012-05-08 18:28:34 PDT

Created attachment 140840 [details] Patch.

Chris Dumez

Comment 2 2012-05-08 21:56:50 PDT

Do not forget to remove the associated test cases from test expectations, assuming that they now pass.

Gyuyoung Kim

Comment 3 2012-05-08 23:14:28 PDT

Comment on attachment 140840 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=140840&action=review > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:606 > + return element->document()->page()->settings()->fullScreenEnabled(); Don't we need to consider *withKeyboard* ?

Kihong Kwon

Comment 4 2012-05-08 23:32:25 PDT

(In reply to comment #2) > Do not forget to remove the associated test cases from test expectations, assuming that they now pass. Oh...OK...:-)

Kihong Kwon

Comment 5 2012-05-09 00:25:29 PDT

(In reply to comment #3) > (From update of attachment 140840 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140840&action=review > > > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:606 > > + return element->document()->page()->settings()->fullScreenEnabled(); > > Don't we need to consider *withKeyboard* ? There are no implemented about "withKeyboard" in the chromium. But gtk use this value for full screen enabling (if withKeyboard is true, return false). That means gtk doesn't support keyboard input in the full screen mode. I think we can support keyboard input in the full screen mode like a chromium.

Kihong Kwon

Comment 6 2012-05-10 03:24:33 PDT

Created attachment 141130 [details] Patch.

Chris Dumez

Comment 7 2012-05-10 03:39:31 PDT

Comment on attachment 141130 [details] Patch. How about this section in test_expectations.txt? // The EFL port has no support for the Fullscreen API BUGWK85870 SKIP : fullscreen = FAIL BUGWK85870 : media/video-play-require-user-gesture.html = TEXT

Kihong Kwon

Comment 8 2012-05-10 03:52:52 PDT

Created attachment 141133 [details] Patch.

Kihong Kwon

Comment 9 2012-05-10 03:54:12 PDT

(In reply to comment #7) > (From update of attachment 141130 [details]) > How about this section in test_expectations.txt? > > // The EFL port has no support for the Fullscreen API > BUGWK85870 SKIP : fullscreen = FAIL > BUGWK85870 : media/video-play-require-user-gesture.html = TEXT It's removed in the new patch.

Dominik Röttsches (drott)

Comment 10 2012-05-10 09:19:22 PDT

Comment on attachment 141133 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=141133&action=review Does this patch fix media/media-fullscreen-inline.html = TEXT media/media-fullscreen-not-in-document.html = TEXT as well? > LayoutTests/ChangeLog:8 > + * platform/efl/Skipped: 5 fullscreen test cases are added. I would recommend to file bugs for the remaining failures and move them to test_expectations.txt with the newly filed bug ID as a prefix.

Kihong Kwon

Comment 11 2012-05-11 01:36:47 PDT

(In reply to comment #10) > (From update of attachment 141133 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141133&action=review > > Does this patch fix > media/media-fullscreen-inline.html = TEXT > media/media-fullscreen-not-in-document.html = TEXT > as well? No, these are not passed. I will file a bugs for these test cases. > > > LayoutTests/ChangeLog:8 > > + * platform/efl/Skipped: 5 fullscreen test cases are added. > > I would recommend to file bugs for the remaining failures and move them to test_expectations.txt with the newly filed bug ID as a prefix. OK.

Kihong Kwon

Comment 12 2012-05-11 01:37:41 PDT

Created attachment 141353 [details] Patch.

Gyuyoung Kim

Comment 13 2012-05-11 01:46:35 PDT

Comment on attachment 141353 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=141353&action=review Need to rebase. > Tools/ChangeLog:10 > + * Scripts/webkitperl/FeatureList.pm: It's better to write description detail to here.

Kihong Kwon

Comment 14 2012-05-11 03:13:04 PDT

Created attachment 141368 [details] Patch.

Kihong Kwon

Comment 15 2012-05-11 03:20:08 PDT

(In reply to comment #13) > (From update of attachment 141353 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141353&action=review > > Need to rebase. > > > Tools/ChangeLog:10 > > + * Scripts/webkitperl/FeatureList.pm: > > It's better to write description detail to here. OK. I will do that before landing this patch.

Kihong Kwon

Comment 16 2012-05-11 03:23:54 PDT

Created attachment 141369 [details] Patch.

Raphael Kubo da Costa (:rakuco)

Comment 17 2012-05-13 15:00:16 PDT

Comment on attachment 141369 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=141369&action=review > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:602 > bool ChromeClientEfl::supportsFullScreenForElement(const WebCore::Element* element, bool withKeyboard) Nit: the second parameter does not need to be named anymore.

Antonio Gomes

Comment 18 2012-05-14 09:49:35 PDT

Comment on attachment 141369 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=141369&action=review looks good, not setting cq so you guys go with what you think it is better. >> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:602 >> bool ChromeClientEfl::supportsFullScreenForElement(const WebCore::Element* element, bool withKeyboard) > > Nit: the second parameter does not need to be named anymore. i would name, and UNUSED_PARAM(withkeyboard);

Kihong Kwon

Comment 19 2012-05-15 01:42:13 PDT

Created attachment 141893 [details] Patch for landing.

WebKit Review Bot

Comment 20 2012-05-15 19:33:16 PDT

Comment on attachment 141893 [details] Patch for landing. Clearing flags on attachment: 141893 Committed r117205: <http://trac.webkit.org/changeset/117205>

WebKit Review Bot

Comment 21 2012-05-15 19:33:22 PDT

All reviewed patches have been landed. Closing bug.

Dominik Röttsches (drott)

Comment 22 2012-05-16 00:23:27 PDT

Comment on attachment 141893 [details] Patch for landing. View in context: https://bugs.webkit.org/attachment.cgi?id=141893&action=review > LayoutTests/platform/efl/test_expectations.txt:371 > +BUGWK85870 : fullscreen/full-screen-keyboard-disabled.html = TEXT Is this the right bug ID? Or did you intend to file a new bug for these?

Kihong Kwon

Comment 23 2012-05-16 00:36:26 PDT

Comment on attachment 141893 [details] Patch for landing. View in context: https://bugs.webkit.org/attachment.cgi?id=141893&action=review >> LayoutTests/platform/efl/test_expectations.txt:371 >> +BUGWK85870 : fullscreen/full-screen-keyboard-disabled.html = TEXT > > Is this the right bug ID? Or did you intend to file a new bug for these? I intended to file a new bug for that. But I missed to file a bug and changed bug ID. I will do that.

Dominik Röttsches (drott)

Comment 24 2012-05-16 01:57:06 PDT

Comment on attachment 141893 [details] Patch for landing. View in context: https://bugs.webkit.org/attachment.cgi?id=141893&action=review >>> LayoutTests/platform/efl/test_expectations.txt:371 >>> +BUGWK85870 : fullscreen/full-screen-keyboard-disabled.html = TEXT >> >> Is this the right bug ID? Or did you intend to file a new bug for these? > > I intended to file a new bug for that. But I missed to file a bug and changed bug ID. > I will do that. full-screen-keyboard-disabled.html is currently an unexpected pass. It seems that this test is already working but I haven't looked into the details. It would be good if you can check and remove from test_expectations.txt - thanks.

Kihong Kwon

Comment 25 2012-05-16 02:12:49 PDT

(In reply to comment #24) > (From update of attachment 141893 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141893&action=review > > >>> LayoutTests/platform/efl/test_expectations.txt:371 > >>> +BUGWK85870 : fullscreen/full-screen-keyboard-disabled.html = TEXT > >> > >> Is this the right bug ID? Or did you intend to file a new bug for these? > > > > I intended to file a new bug for that. But I missed to file a bug and changed bug ID. > > I will do that. > > full-screen-keyboard-disabled.html is currently an unexpected pass. It seems that this test is already working but I haven't looked into the details. It would be good if you can check and remove from test_expectations.txt - thanks. I got your point just now. I will remove it from test_expectations.txt. Thank you for your kind reporting.

Raphael Kubo da Costa (:rakuco)

Comment 26 2012-05-17 12:11:59 PDT

(In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 141133 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=141133&action=review > > > > Does this patch fix > > media/media-fullscreen-inline.html = TEXT > > media/media-fullscreen-not-in-document.html = TEXT > > as well? > > No, these are not passed. > I will file a bugs for these test cases. It looks like you haven't. I have removed media-fullscreen-inline.html from the skipped list because it is now passing. The other test still needs to be investigated, so please *do* open a bug report for that. Watching the tests is part of the required work of landing a new feature.

Kihong Kwon

Comment 27 2012-05-17 18:51:17 PDT

(In reply to comment #26) > (In reply to comment #11) > > (In reply to comment #10) > > > (From update of attachment 141133 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=141133&action=review > > > > > > Does this patch fix > > > media/media-fullscreen-inline.html = TEXT > > > media/media-fullscreen-not-in-document.html = TEXT > > > as well? > > > > No, these are not passed. > > I will file a bugs for these test cases. > > It looks like you haven't. I have removed media-fullscreen-inline.html from the skipped list because it is now passing. The other test still needs to be investigated, so please *do* open a bug report for that. Watching the tests is part of the required work of landing a new feature. I already file a bug for that.(Bug 86187) But I think I need to change description of Bug 86187 if media-fullscreen-inline.html is passed.

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