Bug 85870 – [EFL] Enable Fullscreen API

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Chris Dumez]

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

[Kihong Kwon]

Created attachment 140840 [details]
Patch.

[Chris Dumez]

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

[Gyuyoung Kim]

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]

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

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

Created attachment 141130 [details]
Patch.

[Chris Dumez]

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]

Created attachment 141133 [details]
Patch.

[Kihong Kwon]

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

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

Created attachment 141353 [details]
Patch.

[Gyuyoung Kim]

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]

Created attachment 141368 [details]
Patch.

[Kihong Kwon]

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

Created attachment 141369 [details]
Patch.

[Raphael Kubo da Costa (:rakuco)]

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 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]

Created attachment 141893 [details]
Patch for landing.

[WebKit Review Bot]

Comment on attachment 141893 [details]
Patch for landing.

Clearing flags on attachment: 141893

Committed r117205: <http://trac.webkit.org/changeset/117205>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dominik Röttsches (drott)]

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 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 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]

(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)]

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

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