Emit an error event when a request to enter full-screen is rejected.
<rdar://problem/9574936>
Created attachment 96482 [details] Patch
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)?
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.
Created attachment 108696 [details] Patch
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.
Created attachment 108701 [details] Patch Fixed style error.
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?
Created attachment 122090 [details] Patch Addressed Eric's comments, and renamed the event 'fullscreenerror' to match the W3C spec.
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 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.
Committed r104838: <http://trac.webkit.org/changeset/104838>