Simplify Document::webkitFullScreenEnabled element traversal.
Created attachment 152766 [details] Patch
Pointer lock will be using the same iframe allow logic. Reducing the code here first.
This patch subtly changes the behavior of Document::requestFullScreenForElement(). Before, a JavaScript call "iframe.webkitEnterFullScreen()" on an iframe without an "webkitallowfullscreen" attribute would allow the iframe to enter full screen mode. This makes sense, since it's the element owner that decides whether the full screen request will be accepted. With this patch, element->allowFullScreen() is checked against the requested element if that element is an iframe. This causes the above case (requesting full screen on an iframe directly) to fail. I believe the Document::fullScreenIsAllowedForElement() function could still be simplified, but there would have to be one iteration of finding the element's owner element before the do/while loop.
Created attachment 152817 [details] Patch
Thanks Jer, I've written a test to identify that particular case and adjusted the patch to handle it. Please take a look.
Comment on attachment 152817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152817&action=review > Source/WebCore/html/HTMLFrameElementBase.cpp:253 > + return (hasAttribute(webkitallowfullscreenAttr) || !isFrameElementBase()) Jer, I migrated this logic of !isFrameElementBase(), but we do not have a test that detects this logic being in place. Can you describe what scenario this logic is handling?
Comment on attachment 152817 [details] Patch Attachment 152817 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13282237
Created attachment 152830 [details] Patch
Created attachment 152872 [details] Patch
(In reply to comment #6) > (From update of attachment 152817 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152817&action=review > > > Source/WebCore/html/HTMLFrameElementBase.cpp:253 > > + return (hasAttribute(webkitallowfullscreenAttr) || !isFrameElementBase()) > > Jer, I migrated this logic of !isFrameElementBase(), but we do not have a test that detects this logic being in place. Can you describe what scenario this logic is handling? Doh, nevermind. As I moved the implementation to FrameElementBase I've removed that.
Comment on attachment 152872 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152872&action=review > Source/WebCore/dom/Document.cpp:5371 > + HTMLFrameOwnerElement* owner = element->document()->ownerElement(); > + if (!owner) > + return true; > + return owner->fullscreenIsAllowedByAllOwners(); This could be more easily written as: if (HTMLFrameOwnerElement* owner = element->document()->ownerElement()) return owner->fullscreenIsAllowedByAllOwners(); return true; > Source/WebCore/dom/Document.cpp:5583 > HTMLFrameOwnerElement* owner = ownerElement(); > if (!owner) > return true; > - > - do { > - if (!owner->isFrameElementBase()) > - continue; > - > - if (!static_cast<HTMLFrameElementBase*>(owner)->allowFullScreen()) > - return false; > - } while ((owner = owner->document()->ownerElement())); > - > - return true; > + return owner->fullscreenIsAllowedByAllOwners(); This too. > Source/WebCore/html/HTMLFrameElementBase.cpp:253 > + HTMLFrameOwnerElement* owner = document()->ownerElement(); > + return hasAttribute(webkitallowfullscreenAttr) && (!owner || owner->fullscreenIsAllowedByAllOwners()); Why recurse? It'd be trivial to walk up the owner chain instead. This function could even exist as a free function in Document.cpp. There's really no need for it to be a class function on HTMLFrameElementBase. > Source/WebCore/html/HTMLFrameOwnerElement.h:61 > +#if ENABLE(FULLSCREEN_API) > + virtual bool fullscreenIsAllowedByAllOwners() const { return false; } > +#endif This is probably a good idea (to provide a base implementation in HTMLFrameOwnerElement that is overridden in HTMLFrameElementBase), but there's no need to rename the function and provide a new implementation (see above about the free function possibility).
Created attachment 153041 [details] Patch
Created attachment 153044 [details] Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=153041&action=review Looking good! Just a couple of nits. > Source/WebCore/dom/Document.cpp:591 > + // 4. The fullscreenEnabled attribute must return true if the context object and all ancestor > + // browsing context's documents have their fullscreen enabled flag set, or false otherwise. Nit: please leave this comment in Document::webkitFullscreenEnabled(). > Source/WebCore/dom/Document.cpp:594 > + // Top-level browsing contexts are implied to have their allowFullScreen attribute set. > + if (owner) { Nit: please return (true) early in this case. Otherwise the comment doesn't make (as much) sense. (Also, other reviewers tend to prefer early returns to indented conditionals like this one.) Unofficial r=me.
(In reply to comment #14) > > Source/WebCore/dom/Document.cpp:591 > > + // 4. The fullscreenEnabled attribute must return true if the context object and all ancestor > > + // browsing context's documents have their fullscreen enabled flag set, or false otherwise. > > Nit: please leave this comment in Document::webkitFullscreenEnabled(). Looks like your latest patch does this. Thanks!
Comment on attachment 153044 [details] Patch Attachment 153044 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13271648
Created attachment 153049 [details] Patch
Thanks Jer, looks nice and clean now - easy to reuse for pointer lock. enne, take a look?
Created attachment 153050 [details] Patch
Comment on attachment 153050 [details] Patch R=me.
Comment on attachment 153050 [details] Patch Clearing flags on attachment: 153050 Committed r123005: <http://trac.webkit.org/changeset/123005>
All reviewed patches have been landed. Closing bug.