Bug 62320 - Emit an error event when a request to enter full-screen is rejected.
Summary: Emit an error event when a request to enter full-screen is rejected.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-06-08 14:08 PDT by Jer Noble
Modified: 2012-01-12 11:15 PST (History)
8 users (show)

See Also:


Attachments
Patch (12.29 KB, patch)
2011-06-08 14:35 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (13.70 KB, patch)
2011-09-26 11:35 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (13.70 KB, patch)
2011-09-26 11:50 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (14.75 KB, patch)
2012-01-11 13:58 PST, Jer Noble
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2011-06-08 14:08:28 PDT
Emit an error event when a request to enter full-screen is rejected.
Comment 1 Jer Noble 2011-06-08 14:08:48 PDT
<rdar://problem/9574936>
Comment 2 Jer Noble 2011-06-08 14:35:38 PDT
Created attachment 96482 [details]
Patch
Comment 3 Eric Seidel (no email) 2011-06-13 15:22:58 PDT
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)?
Comment 4 Jer Noble 2011-06-13 15:29:51 PDT
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.
Comment 5 Jer Noble 2011-09-26 11:35:11 PDT
Created attachment 108696 [details]
Patch
Comment 6 WebKit Review Bot 2011-09-26 11:37:44 PDT
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.
Comment 7 Jer Noble 2011-09-26 11:50:57 PDT
Created attachment 108701 [details]
Patch

Fixed style error.
Comment 8 Eric Carlson 2011-10-10 14:14:28 PDT
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?
Comment 9 Jer Noble 2012-01-11 13:58:20 PST
Created attachment 122090 [details]
Patch

Addressed Eric's comments, and renamed the event 'fullscreenerror' to match the W3C spec.
Comment 10 Eric Carlson 2012-01-11 14:31:59 PST
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?
Comment 11 Jer Noble 2012-01-11 14:47:13 PST
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.
Comment 12 Jer Noble 2012-01-12 11:15:37 PST
Committed r104838: <http://trac.webkit.org/changeset/104838>