[webkit-reviews] review granted: [Bug 80660] Support W3C Full Screen API proposal : [Attachment 131563] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Mar 16 10:19:00 PDT 2012
Alexey Proskuryakov <ap at webkit.org> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 80660: Support W3C Full Screen API proposal
https://bugs.webkit.org/show_bug.cgi?id=80660
Attachment 131563: Patch
https://bugs.webkit.org/attachment.cgi?id=131563&action=review
------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=131563&action=review
> Source/WebCore/dom/Document.cpp:5105
> + // 2. Let doc be element's node document. (i.e. "this")
> + Document* doc = this;
We prefer to not abbreviate words in variable names.
> Source/WebCore/dom/Document.cpp:5148
> + } while (++current != docs.end());
So, Deque iterator is not invalidated for any operations you do above?
> Source/WebCore/dom/Document.cpp:5485
> +void Document::addDocumentToEventQueue(Document* doc)
Please have "fullscreen" somewhere in this name. It's super confusing
otherwise.
As elsewhere, "doc" is an unwelcome abbreviation.
> Source/WebKit/mac/WebView/WebView.mm:6287
> - (BOOL)_supportsFullScreenForElement:(const WebCore::Element*)element
withKeyboard:(BOOL)withKeyboard
I'm not sure if I fully understand why this function no longer looks at its
withKeyboard argument. But if it doesn't need to look at it, can it be just
removed?
> Source/WebKit2/UIProcess/WebFullScreenManagerProxy.cpp:-94
> void WebFullScreenManagerProxy::supportsFullScreen(bool withKeyboard, bool&
supports)
> {
> - if (withKeyboard)
Ditto.
More information about the webkit-reviews
mailing list