Bug 62320 – Emit an error event when a request to enter full-screen is rejected.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jer Noble]

Emit an error event when a request to enter full-screen is rejected.

[Jer Noble]

<rdar://problem/9574936>

[Jer Noble]

Created attachment 96482 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 96482 [details]
Patch

Is there a spec which covers this?  I wonder why this would be a separate event and not just a change event emitted with the same previous state (and maybe some error information)?

[Jer Noble]

There is no spec update which covers this.  This is a result of page author feedback after using the new full-screen API.

There is currently no event information passed along with the simple event fired when full-screen changes state.  The listener is expected to ask the document what the current full-screen state is when they catch the change event.  I don't think it's ideal to ask the page authors to assume that, if they get a full-screen change event, and the state hasn't changed, they should treat that as an error.

[Jer Noble]

Created attachment 108696 [details]
Patch

[WebKit Review Bot]

Attachment 108696 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/full..." exit_code: 1

Source/WebCore/dom/Document.cpp:4872:  Missing space before ( in while(  [whitespace/parens] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 108701 [details]
Patch

Fixed style error.

[Eric Carlson]

Comment on attachment 108701 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108701&action=review

> LayoutTests/ChangeLog:11
> +        * fullscreen/full-screen-iframe-not-allowed-expected.txt:
> +        * fullscreen/full-screen-iframe-not-allowed.html:
> +        * fullscreen/full-screen-request-rejected-expected.txt: Added.
> +        * fullscreen/full-screen-request-rejected.html: Added.

You should also verify that the "If the element was removed from our tree, also message the documentElement" code is run (for both events).

> Source/WebCore/dom/Document.cpp:444
>      , m_fullScreenChangeDelayTimer(this, &Document::fullScreenChangeDelayTimerFired)
> +    , m_fullScreenDeniedDelayTimer(this, &Document::fullScreenDeniedDelayTimerFired)

Do you really need another timer for this, couldn't you service both queues with a single timer?

[Jer Noble]

Created attachment 122090 [details]
Patch

Addressed Eric's comments, and renamed the event 'fullscreenerror' to match the W3C spec.

[Eric Carlson]

Comment on attachment 122090 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122090&action=review

> Source/WebCore/ChangeLog:12
> +        (webkitfullscreenrequesterror) in response.  But emit the event during the next

Don't you mean webkitfullscreenerror?

> Source/WebCore/ChangeLog:17
> +        (WebCore::Document::Document): Add initializers for new timer.

Didn't you remove the new timer?

> Source/WebCore/ChangeLog:21
> +        * dom/Document.idl: Add support for setting an onfullscreenerror attribute.

Isn't it onwebkitfullscreenerror?

> Source/WebCore/dom/Document.cpp:5209
> +    while (!m_fullScreenErrorEventTargetQueue.isEmpty()) {
> +        RefPtr<Element> element = m_fullScreenErrorEventTargetQueue.takeFirst();
> +        if (!element)
> +            element = documentElement();

Can takeFirst() return 0 when the queue is not empty?

[Jer Noble]

Comment on attachment 122090 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122090&action=review

>> Source/WebCore/ChangeLog:12
>> +        (webkitfullscreenrequesterror) in response.  But emit the event during the next
> 
> Don't you mean webkitfullscreenerror?

Whoops.  Will fix.

>> Source/WebCore/ChangeLog:17
>> +        (WebCore::Document::Document): Add initializers for new timer.
> 
> Didn't you remove the new timer?

I did.  I'll fix this comment.

>> Source/WebCore/ChangeLog:21
>> +        * dom/Document.idl: Add support for setting an onfullscreenerror attribute.
> 
> Isn't it onwebkitfullscreenerror?

Yep.

>> Source/WebCore/dom/Document.cpp:5209
>> +            element = documentElement();
> 
> Can takeFirst() return 0 when the queue is not empty?

If a null value was pushed into the queue, then yes.  documentElement() does not guarantee a non-null return value, so it's possible (though unlikely) that a null value could be stuffed into the queue.

[Jer Noble]

Committed r104838: <http://trac.webkit.org/changeset/104838>