Bug 49481 - Implement WebKit Full Screen support
Summary: Implement WebKit Full Screen support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Jer Noble
URL: https://wiki.mozilla.org/Gecko:FullSc...
Keywords:
Depends on: 43099
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-12 15:55 PST by Jer Noble
Modified: 2011-03-03 16:48 PST (History)
19 users (show)

See Also:


Attachments
WebCore-RenderFullScreen (12.20 KB, patch)
2010-11-12 16:55 PST, Jer Noble
abarth: review-
Details | Formatted Diff | Diff
WebCore-DOM (5.08 KB, patch)
2010-11-12 16:56 PST, Jer Noble
simon.fraser: review-
Details | Formatted Diff | Diff
WebCore-Rendering (6.92 KB, patch)
2010-11-12 16:56 PST, Jer Noble
simon.fraser: review-
Details | Formatted Diff | Diff
WebCore-EverythingElse (3.97 KB, patch)
2010-11-12 16:57 PST, Jer Noble
no flags Details | Formatted Diff | Diff
WebKit-WebFullScreenController (40.13 KB, patch)
2010-11-12 16:57 PST, Jer Noble
simon.fraser: review-
Details | Formatted Diff | Diff
WebKit-WebView (4.44 KB, patch)
2010-11-12 16:58 PST, Jer Noble
simon.fraser: review+
Details | Formatted Diff | Diff
WebCore-RenderFullScreen (14.16 KB, patch)
2010-11-15 10:44 PST, Jer Noble
no flags Details | Formatted Diff | Diff
WebCore-DOM (7.16 KB, patch)
2010-12-03 15:13 PST, Jer Noble
no flags Details | Formatted Diff | Diff
WebCore-DOM (2.77 KB, patch)
2010-12-03 15:30 PST, Jer Noble
no flags Details | Formatted Diff | Diff
WebCore-DOM (7.74 KB, patch)
2010-12-03 15:31 PST, Jer Noble
no flags Details | Formatted Diff | Diff
WebCore-DOM (7.73 KB, patch)
2010-12-03 15:48 PST, Jer Noble
no flags Details | Formatted Diff | Diff
WebKit-WebFullScreenController (39.76 KB, patch)
2010-12-03 16:13 PST, Jer Noble
no flags Details | Formatted Diff | Diff
NewFullScreen-Part-1-RenderFullScreen (13.12 KB, patch)
2010-12-21 11:13 PST, Jer Noble
no flags Details | Formatted Diff | Diff
NewFullScreen-Part-2-The-Document (16.52 KB, patch)
2010-12-21 11:14 PST, Jer Noble
simon.fraser: review+
Details | Formatted Diff | Diff
NewFullScreen-Part-3-Renderering (10.56 KB, patch)
2010-12-21 11:15 PST, Jer Noble
simon.fraser: review-
Details | Formatted Diff | Diff
NewFullScreen-Part-4-Element-screenRect.patch (3.63 KB, patch)
2010-12-21 11:16 PST, Jer Noble
simon.fraser: review+
Details | Formatted Diff | Diff
NewFullScreen-Part-5-WebFullscreenController (42.05 KB, patch)
2010-12-21 11:17 PST, Jer Noble
simon.fraser: review+
Details | Formatted Diff | Diff
NewFullScreen-Part-1-RenderFullScreen (13.11 KB, patch)
2010-12-21 11:26 PST, Jer Noble
simon.fraser: review+
Details | Formatted Diff | Diff
NewFullScreen-Part-3-Renderering (11.44 KB, patch)
2011-01-06 11:30 PST, Jer Noble
simon.fraser: 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 2010-11-12 15:55:38 PST
Implement the Gecko Full Screen API support inside of WebKit.
Comment 1 Jer Noble 2010-11-12 16:55:34 PST
Created attachment 73793 [details]
WebCore-RenderFullScreen
Comment 2 Jer Noble 2010-11-12 16:56:00 PST
Created attachment 73794 [details]
WebCore-DOM
Comment 3 Jer Noble 2010-11-12 16:56:42 PST
Created attachment 73795 [details]
WebCore-Rendering
Comment 4 Jer Noble 2010-11-12 16:57:09 PST
Created attachment 73796 [details]
WebCore-EverythingElse
Comment 5 Jer Noble 2010-11-12 16:57:44 PST
Created attachment 73797 [details]
WebKit-WebFullScreenController
Comment 6 Jer Noble 2010-11-12 16:58:11 PST
Created attachment 73798 [details]
WebKit-WebView
Comment 7 WebKit Review Bot 2010-11-12 16:58:39 PST
Attachment 73795 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/page/FrameView.cpp', u'WebCore/rendering/RenderLayerBacking.cpp', u'WebCore/rendering/RenderLayerCompositor.cpp', u'WebCore/rendering/RenderLayerCompositor.h']" exit_code: 1
WebCore/rendering/RenderLayerCompositor.cpp:806:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 WebKit Review Bot 2010-11-12 17:00:21 PST
Attachment 73797 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKit/ChangeLog', u'WebKit/WebKit.xcodeproj/project.pbxproj', u'WebKit/mac/ChangeLog', u'WebKit/mac/WebView/WebFullscreenController.h', u'WebKit/mac/WebView/WebFullscreenController.mm']" exit_code: 1
WebKit/mac/WebView/WebFullscreenController.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/mac/WebView/WebFullscreenController.h:40:  _element is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:41:  _renderer is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:42:  _replacementElement is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:46:  _rendererLayer is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:47:  _backgroundLayer is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:48:  _mediaEventListener is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:50:  _isAnimating is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:51:  _isFullscreen is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:52:  _isWindowLoaded is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:53:  _forceDisableAnimation is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:54:  _isPlaying is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:55:  _idleDisplaySleepAssertion is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:56:  _idleSystemSleepAssertion is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:58:  _savedUIMode is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:59:  _savedUIOptions is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:60:  _initialFrame is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 17 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Jer Noble 2010-11-12 17:03:38 PST
(In reply to comment #7)
> WebCore/rendering/RenderLayerCompositor.cpp:806:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]

I've removed the "else" statement.
Comment 10 Eric Seidel (no email) 2010-11-12 17:13:13 PST
Attachment 73795 [details] did not build on mac:
Build output: http://queues.webkit.org/results/5827003
Comment 11 WebKit Review Bot 2010-11-12 17:22:01 PST
Attachment 73795 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5819003
Comment 12 Early Warning System Bot 2010-11-12 17:36:14 PST
Attachment 73795 [details] did not build on qt:
Build output: http://queues.webkit.org/results/5826003
Comment 13 Daniel Bates 2010-11-12 23:03:09 PST
Comment on attachment 73793 [details]
WebCore-RenderFullScreen

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

> WebCore/rendering/RenderFullScreen.cpp:8
> +/*
> + *  RenderFullScreen.cpp
> + *  WebCore
> + *
> + *  Created by Jer Noble on 10/25/10.
> + *  Copyright 2010 Apple Computer. All rights reserved.
> + *
> + */

Briefly glanced at this patch and noticed that this comment is similar to the default Xcode copyright comment. Instead, this file should have the same copyright comment as in RenderFullScreen.h. Also, while the license in RenderFullScreen.h is sufficient it differs in wording from the Apple license used in Apple-contributed files, see <http://trac.webkit.org/browser/trunk/WebKit/LICENSE>.
Comment 14 Adam Barth 2010-11-12 23:16:49 PST
Comment on attachment 73793 [details]
WebCore-RenderFullScreen

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

>> WebCore/rendering/RenderFullScreen.cpp:8
>> +/*
>> + *  RenderFullScreen.cpp
>> + *  WebCore
>> + *
>> + *  Created by Jer Noble on 10/25/10.
>> + *  Copyright 2010 Apple Computer. All rights reserved.
>> + *
>> + */
> 
> Briefly glanced at this patch and noticed that this comment is similar to the default Xcode copyright comment. Instead, this file should have the same copyright comment as in RenderFullScreen.h. Also, while the license in RenderFullScreen.h is sufficient it differs in wording from the Apple license used in Apple-contributed files, see <http://trac.webkit.org/browser/trunk/WebKit/LICENSE>.

Good point.  We need a WebKit-compatible license block.
Comment 15 Jer Noble 2010-11-15 10:44:43 PST
Created attachment 73911 [details]
WebCore-RenderFullScreen

Updated license text in both RenderFullScreen.cpp and RenderFullScreen.h.
Comment 16 WebKit Review Bot 2010-11-15 10:57:06 PST
Attachment 73911 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/WebCore.xcodeproj/project.pbxproj', u'WebCore/rendering/RenderFullScreen.cpp', u'WebCore/rendering/RenderFullScreen.h', u'WebCore/rendering/RenderObject.h']" exit_code: 1
WebCore/rendering/RenderFullScreen.cpp:33:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Simon Fraser (smfr) 2010-11-15 11:24:34 PST
Is it possible to make a series of patches that are not split by directory, but rather logically build each on top of the last?
Comment 18 Jer Noble 2010-11-15 12:10:30 PST
(In reply to comment #17)
> Is it possible to make a series of patches that are not split by directory, but rather logically build each on top of the last?

They do.  In order:

WebCore-RenderFullScreen: contains the new RenderFullScreen class and the necessary project file changes.
WebCore-DOM: contains changes to the Document and Node classes needed to create and access the RenderFullScreenObject.
WebCore-Rendering: uses the DOM changes to correctly render the RenderFullScreen object.
WebCore-EverythingElse: export file changes and miscellaneous changes not directly related to the RenderFullScreen object.
WebKit-WebFullScreenController: uses the DOM changes to host the RenderFullScreen layer in a new fullscreen window.
WebKit-WebView: Ties the new Document notifications to the new window controller.
Comment 19 Andrew Scherkus 2010-11-15 17:24:59 PST
exciting!  will skim through the patches
Comment 20 Build Bot 2010-11-15 20:17:52 PST
Attachment 73795 [details] did not build on win:
Build output: http://queues.webkit.org/results/5939095
Comment 21 Eric Seidel (no email) 2010-11-17 17:01:18 PST
Attachment 73795 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6204020
Comment 22 David Dorwin 2010-11-19 16:10:57 PST
Hi. I'll be implementing full screen support in Chromium, so I was reading through these patches.

> WebCore/dom/Document.cpp:4764
> +    element->detach();
>      // m_fullScreenElement has already been cleared; recalc the style of 
>      // the passed in element instead.
>      element->setNeedsStyleRecalc(FullStyleChange);
>      if (element != documentElement())
>          documentElement()->setNeedsStyleRecalc(FullStyleChange);
> +    element->attach();
It's not clear to me why the element needs to be detached then attached. An explanation might be helpful.

Looking at Node::setNeedsStyleRecalc(), it appears that the detach() call might result in the element->setNeedsStyleRecalc(FullStyleChange) call having no effect. Maybe I'm missing something.
Comment 23 Jer Noble 2010-11-19 16:33:59 PST
(In reply to comment #22)
> It's not clear to me why the element needs to be detached then attached. An explanation might be helpful.
> 
> Looking at Node::setNeedsStyleRecalc(), it appears that the detach() call might result in the element->setNeedsStyleRecalc(FullStyleChange) call having no effect. Maybe I'm missing something.

Detaching and re-attaching forces the element's renderer to be re-created, thus disconnecting it from the RenderFullScreen renderer.  Without detaching the element, I don't think the style update machinery would know that element's renderer needs to be reparented.

It might be the case that the element->attach() is unnecessary, and the element will get attach() called on it as part of the normal style recalculation.  But I thought it best to be explicit about it.
Comment 24 Simon Fraser (smfr) 2010-12-02 11:36:39 PST
Comment on attachment 73794 [details]
WebCore-DOM

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

> WebCore/dom/Document.cpp:4741
> +    setNeedsStyleRecalc(SyntheticStyleChange);
> +    documentElement()->setNeedsStyleRecalc(SyntheticStyleChange);
> +    m_fullScreenElement->setNeedsStyleRecalc(SyntheticStyleChange);

You don't need these if you're doing a recalcStyle(Force).

> WebCore/dom/Document.cpp:4746
> +    frame()->view()->setShouldUpdateWhileOffscreen(TRUE);

true, not TRUE

> WebCore/dom/Document.cpp:4772
> +    element->detach();
>      // m_fullScreenElement has already been cleared; recalc the style of 
>      // the passed in element instead.
>      element->setNeedsStyleRecalc(FullStyleChange);
>      if (element != documentElement())
>          documentElement()->setNeedsStyleRecalc(FullStyleChange);
> +    element->attach();
> +
>      updateStyleIfNeeded();

Why not just do a recalcStyle(Force) here too?

> WebCore/dom/Document.cpp:4778
> +    // Save off the m_fullScreenRenderer to destroy only after the client has
> +    // been notified that the renderer has changed:
> +    RenderBox* renderer = m_fullScreenRenderer;
> +    webkitSetFullScreenRenderer(0);
> +    renderer->destroy();

Renderers are not refcounted, and are only normally created/destroyed in attach/detach or style changes. It worries me that you're destroying the renderer here.

> WebCore/dom/Document.cpp:4780
> +    frame()->view()->setShouldUpdateWhileOffscreen(FALSE);

false, not FALSE

> WebCore/dom/Document.cpp:4788
> +void Document::webkitSetFullScreenRenderer(RenderBox* renderer)
> +{
> +    m_fullScreenRenderer = renderer;
> +    page()->chrome()->client()->fullScreenRendererChanged(m_fullScreenRenderer);
> +}

Where is the corresponding Document.h change?

This method does not need the webkit prefix.

> WebCore/dom/Node.cpp:1338
> +    if (document()->webkitFullScreen() && document()->webkitCurrentFullScreenElement() == this) {

document()->webkitFullScreen() is a bad method name. It should be isFullscreen(). Only methods exposed via IDL need the webkit prefix.

> WebCore/dom/Node.cpp:1345
> +        RenderFullScreen* fullscreenRenderer = new (document()->renderArena()) RenderFullScreen(document());
> +        fullscreenRenderer->setStyle(RenderFullScreen::createFullScreenStyle());
> +        parentRenderer->addChild(fullscreenRenderer, 0);
> +        parentRenderer = fullscreenRenderer;
> +        nextRenderer = 0;
> +        document()->webkitSetFullScreenRenderer(fullscreenRenderer);
> +    }

What prevents this renderer from being destroyed if JS runs in the document, and the parent's child renderers are re-computed?
Comment 25 Simon Fraser (smfr) 2010-12-02 11:41:18 PST
Comment on attachment 73795 [details]
WebCore-Rendering

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

There's too much special magic here, and it's not clear to me how much is just for the fullscreen animation.

> WebCore/page/FrameView.cpp:495
> +#if ENABLE(FULLSCREEN_API)
> +    Document* document = m_frame->document();
> +    if (document->webkitFullScreen() && document->webkitFullScreenRenderer())
> +        view->compositor()->updateCompositingLayers(CompositingUpdateAfterLayoutOrStyleChange, document->webkitFullScreenRenderer()->layer());
> +#endif

I don't think the fullscreen layers should be disconnected at all, so they shoudn't need updating separately.

> WebCore/page/FrameView.cpp:563
> +    // The fullScreenRenderer's graphicsLayer  has been re-parented, and the above recursive syncCompositingState
> +    // call will not cause the subtree under it to repaint.  Explicitly call the syncCompositingState on 
> +    // the fullScreenRenderer's graphicsLayer here:
> +    Document* document = m_frame->document();
> +    if (document->webkitFullScreen() && document->webkitFullScreenRenderer()) {
> +        RenderObject* fullScreenRenderer = document->webkitFullScreenRenderer();
> +        if (GraphicsLayer* fullScreenLayer = toRenderBox(fullScreenRenderer)->layer()->backing()->graphicsLayer())
> +            fullScreenLayer->syncCompositingState();
> +    }

Ditto.

> WebCore/page/FrameView.cpp:2005
> +        // The fullScreenRenderer's graphicsLayer  has been re-parented, and the above recursive syncCompositingState
> +        // call will not cause the subtree under it to repaint.  Explicitly call the syncCompositingState on 
> +        // the fullScreenRenderer's graphicsLayer here:
> +        if (document->webkitFullScreen() && document->webkitFullScreenRenderer()) {
> +            RenderObject* fullScreenRenderer = document->webkitFullScreenRenderer();
> +            if (GraphicsLayer* fullScreenLayer = toRenderBox(fullScreenRenderer)->layer()->backing()->graphicsLayer())
> +                fullScreenLayer->syncCompositingState();
> +        }

Ditto.

> WebCore/rendering/RenderLayerBacking.cpp:185
> +    // We'd need RenderObject::convertContainerToLocalQuad(), which doesn't yet exist.  If this
> +    // is a fullscreen renderer, don't clip to the viewport, as the renderer will be asked to
> +    // display outside of the viewport bounds.
> +    if (compositor()->compositingConsultsOverlap() && !layerOrAncestorIsTransformed(m_owningLayer) 
> +#if ENABLE(FULLSCREEN_API)
> +        && !layerOrAncestorIsFullScreen(m_owningLayer)
> +#endif

Is this only true during animation to fullscreen, or when in fullscreen? Why isn't the viewport fullscreen when in fullscreen?
Comment 26 Simon Fraser (smfr) 2010-12-02 11:43:01 PST
Comment on attachment 73796 [details]
WebCore-EverythingElse

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

> WebCore/css/fullscreen.css:2
>  :-webkit-full-screen {
> -    position:fixed;
> -    top:0;
> -    left:0;
> -    right:0;
> -    bottom:0;
> +    background-color: white;

Why do we no longer use position:fixed etc?

> WebCore/dom/Element.h:164
> +    IntRect screenRect();

Should be const. Needs as comment saying what it really means, or a better name, like boundingBoxInScreenCoordinates().
Comment 27 Simon Fraser (smfr) 2010-12-02 11:47:13 PST
Comment on attachment 73911 [details]
WebCore-RenderFullScreen

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

> WebCore/ChangeLog:56
> +        * WebCore.exp.in: Export a number of symbols newly used by WebKit.
> +        * WebCore.xcodeproj/project.pbxproj: Add RenderFullScreen.cpp,h to the project.
> +        * css/fullscreen.css: Change the default style rules for fullscreen elements.
> +        * dom/Document.cpp:
> +        (WebCore::Document::Document): Initialize m_fullscreenRenderer.
> +        (WebCore::Document::detach): Uninitialize m_fullscreenRenderer.
> +        (WebCore::Document::webkitWillEnterFullScreenForElement): 
> +        (WebCore::Document::webkitDidExitFullScreenForElement):
> +        (WebCore::Document::webkitSetFullScreenRenderer): Set m_fullscreenRenderer, and notify chrome clients.
> +        * dom/Document.h:
> +        (WebCore::Document::webkitFullScreenRenderer): Accessor for m_fullscreenRenderer.
> +        * dom/Element.cpp:
> +        (WebCore::Element::screenRect): Moved into Element from HTMLMediaElement.
> +        * dom/Node.cpp:
> +        (WebCore::Node::createRendererIfNeeded): Creates a RenderFullScreen parent for the
> +            current fullscreen element.
> +        * html/HTMLMediaElement.cpp: Moved screenRect into Element.
> +        * html/HTMLMediaElement.h: Ditto.
> +        * page/ChromeClient.h:
> +        (WebCore::ChromeClient::fullScreenRendererChanged): New function used to notify clients of full 
> +            screen renderer changes.
> +        * page/FrameView.cpp:
> +        (WebCore::FrameView::updateCompositingLayers):  Special treatment for fullscreen renderer.
> +        (WebCore::FrameView::syncCompositingStateRecursive): Ditto.
> +        (WebCore::FrameView::paintContents): Ditto.
> +        * rendering/RenderFullScreen.cpp: Added.
> +        (RenderFullScreen::createFullScreenStyle): Default style for RenderFullScreen elements.
> +        * rendering/RenderFullScreen.h: Added.
> +        * rendering/RenderLayerBacking.cpp:
> +        (WebCore::layerOrAncestorIsFullScreen): New function.
> +        (WebCore::RenderLayerBacking::updateCompositedBounds): Do not clip if the layerOrAncestorIsFullScreen.
> +        * rendering/RenderLayerCompositor.cpp:
> +        (WebCore::RenderLayerCompositor::rebuildCompositingLayerTree): Special treatment for fullscreen renderer.
> +        (WebCore::RenderLayerCompositor::requiresCompositingLayer): Ditto.
> +        (WebCore::RenderLayerCompositor::requiresCompositingForFullScreen): Ditto.
> +        * rendering/RenderLayerCompositor.h:
> +        * rendering/RenderObject.h:
> +        (WebCore::RenderObject::isFullScreen): New function.

There are lots of changes here that are not in this patch.

> WebCore/WebCore.xcodeproj/project.pbxproj:-21445
> -			developmentRegion = English;

Please update your Xcode!

> WebCore/rendering/RenderFullScreen.cpp:33
> +    PassRefPtr<RenderStyle> fullscreenStyle = RenderStyle::createDefaultStyle();

This should be a RefPtr.

> WebCore/rendering/RenderFullScreen.cpp:36
> +    // Create a stacking context:
> +    fullscreenStyle->setZIndex(INT_MAX);

setZIndex(0) is sufficient to create a stacking context.

> WebCore/rendering/RenderFullScreen.h:36
> +    RenderFullScreen(Node* node) : RenderFlexibleBox(node) { setReplaced(false); setIsAnonymous(false); }

We normally put these on multiple lines.
Comment 28 Simon Fraser (smfr) 2010-12-02 11:49:01 PST
Comment on attachment 73798 [details]
WebKit-WebView

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

> WebKit/mac/WebView/WebView.mm:5977
> +        // The backend may just warn us that the underlaying plaftormMovie()
> +        // has changed. Just force an update.

Typo "plaftorm"
Comment 29 Simon Fraser (smfr) 2010-12-02 11:52:44 PST
In general I think this feature should still work if accelerated compositing is disabled.
Comment 30 Simon Fraser (smfr) 2010-12-02 12:06:15 PST
Comment on attachment 73797 [details]
WebKit-WebFullScreenController

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

> WebKit/mac/WebView/WebFullscreenController.h:2
> + * Copyright (C) 2009 Apple Inc. All rights reserved.

2010

> WebKit/mac/WebView/WebFullscreenController.h:42
> +    RefPtr<WebCore::Element> _element; // (retain)
> +    WebCore::RenderBox* _renderer; // (set)
> +    RefPtr<WebCore::Element> _replacementElement; // (retain)

No need for the //retain comments on C++ things; it's obvious.

> WebKit/mac/WebView/WebFullscreenController.mm:83
> +using WebCore::AnimationList;
> +using WebCore::Animation;
> +using WebCore::DOMWindow;
> +using WebCore::Document;
> +using WebCore::Element;
> +using WebCore::Event;
> +using WebCore::EventListener;
> +using WebCore::EventNames;
> +using WebCore::FloatRect;
> +using WebCore::FloatSize;
> +using WebCore::GraphicsLayer;
> +using WebCore::HTMLMediaElement;
> +using WebCore::HTMLNames::videoTag;
> +using WebCore::Length;
> +using WebCore::LengthType;
> +using WebCore::Node;
> +using WebCore::NodeList;
> +using WebCore::RenderBlock;
> +using WebCore::RenderBox;
> +using WebCore::RenderFullScreen;
> +using WebCore::RenderLayer;
> +using WebCore::RenderLayerBacking;
> +using WebCore::RenderObject;
> +using WebCore::RenderStyle;
> +using WebCore::ScriptExecutionContext;
> +

Why not just "using namespace WebCore"?

> WebKit/mac/WebView/WebFullscreenController.mm:144
> +    [_tickleTimer invalidate];
> +    [_tickleTimer release];
> +    _tickleTimer = nil;

There's a retain cycle between the timer and self. You'll never hit dealloc with a non-nil timer.

> WebKit/mac/WebView/WebFullscreenController.mm:156
> +#ifdef BUILDING_ON_TIGER
> +    // WebFullscreenController is not supported on Tiger:
> +    ASSERT_NOT_REACHED();

This is odd. Why not assert earlier, or #ifdef all the code out on Tiger?

> WebKit/mac/WebView/WebFullscreenController.mm:210
> +    if (_element) {
> +        DOMWindow* window = _element->document()->domWindow();
> +        if (window) {
> +            window->removeEventListener(eventNames.playEvent,  _mediaEventListener.get(), true);
> +            window->removeEventListener(eventNames.pauseEvent, _mediaEventListener.get(), true);
> +            window->removeEventListener(eventNames.endedEvent, _mediaEventListener.get(), true);
> +        }
> +    }
> +        
> +    _element = element;
> +    
> +    if (_element) {
> +        DOMWindow* window = _element->document()->domWindow();
> +        if (window) {
> +            window->addEventListener(eventNames.playEvent,  _mediaEventListener, true);
> +            window->addEventListener(eventNames.pauseEvent, _mediaEventListener, true);
> +            window->addEventListener(eventNames.endedEvent, _mediaEventListener, true);
> +        }
> +    }

Can you explain what you're doing here? Why are media events special? What if the content has registered event listeners on the old window?

> WebKit/mac/WebView/WebFullscreenController.mm:274
> +        [[[self webView] window] orderOut:self];

Is this hiding the browser window? Is that the right thing to do in all cases (including non-browser clients)?

> WebKit/mac/WebView/WebFullscreenController.mm:312
> +    [[self window] setFrame:[[[self window] screen] frame] display:YES];

Maybe cache [self window]

> WebKit/mac/WebView/WebFullscreenController.mm:378
> +    // Set up the final style of the FullScreen render block.  Set an absolute
> +    // width and height equal to the size of the screen, and anchor the layer
> +    // at the top, left at (0,0).  The RenderFullScreen style is already set 
> +    // to position:fixed.
> +    PassRefPtr<RenderStyle> newStyle = RenderStyle::clone(_renderer->style());
> +    newStyle->setWidth(Length((int)screenFrame.size.width, WebCore::Fixed));
> +    newStyle->setHeight(Length((int)screenFrame.size.height, WebCore::Fixed));
> +    newStyle->setTop(Length(0, WebCore::Fixed));
> +    newStyle->setLeft(Length(0, WebCore::Fixed));
> +    _renderer->setStyle(newStyle);

It's weird to be running this code in WebKit. You should move it to WebCore somehow.

> WebKit/mac/WebView/WebFullscreenController.mm:389
> +    // Cause the document to layout, thus calculating a new fullscreen element size:
> +    [self _document]->updateLayout();
> +    
> +    RenderBox* childRenderer = _renderer->firstChildBox();
> +    CGRect destinationFrame = CGRectMake(childRenderer->x(), childRenderer->y(), childRenderer->width(), childRenderer->height());
> +    
> +    // Some properties haven't propogated from the GraphicsLayer to the CALayer yet.  So
> +    // tell the renderer's layer to sync it's compositing state:
> +    GraphicsLayer* rendererGraphics = _renderer->layer()->backing()->graphicsLayer();
> +    rendererGraphics->syncCompositingState();    

Maybe move this code too. Also don't assume that you have any GraphicsLayers; accelerated composting might be disabled.

> WebKit/mac/WebView/WebFullscreenController.mm:586
> +    _tickleTimer = [[NSTimer scheduledTimerWithTimeInterval:tickleTimerInterval target:self selector:@selector(_tickleTimerFired) userInfo:nil repeats:YES] retain];

This creates a retain cycle between the timer and self.

> WebKit/mac/WebView/WebFullscreenController.mm:651
> +- (BOOL)_isAnyMoviePlaying
> +{
> +    if (!_element)
> +        return NO;
> +    
> +    Node* nextNode = _element.get();
> +    while (nextNode)
> +    {
> +        if (nextNode->hasTagName(videoTag)) {
> +            HTMLMediaElement* element = static_cast<HTMLMediaElement*>(nextNode);
> +            if (!element->paused() && !element->ended())
> +                return YES;
> +        }
> +        
> +        nextNode = nextNode->traverseNextNode(_element.get());
> +    }
> +    
> +    return NO;
> +}
> +

What about movies in subframes?
Comment 31 Jer Noble 2010-12-02 12:14:40 PST
(In reply to comment #29)
> In general I think this feature should still work if accelerated compositing is disabled.

Fullscreen for video elements currently is disabled if accelerated compositing is not available.  Why is this different?
Comment 32 Jer Noble 2010-12-02 12:35:04 PST
(In reply to comment #24)
> (From update of attachment 73794 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73794&action=review
> 
> > WebCore/dom/Document.cpp:4741
> > +    setNeedsStyleRecalc(SyntheticStyleChange);
> > +    documentElement()->setNeedsStyleRecalc(SyntheticStyleChange);
> > +    m_fullScreenElement->setNeedsStyleRecalc(SyntheticStyleChange);
> 
> You don't need these if you're doing a recalcStyle(Force).

Gone.

> > WebCore/dom/Document.cpp:4746
> > +    frame()->view()->setShouldUpdateWhileOffscreen(TRUE);
> 
> true, not TRUE

Changed.

> > WebCore/dom/Document.cpp:4772
> > +    element->detach();
> >      // m_fullScreenElement has already been cleared; recalc the style of 
> >      // the passed in element instead.
> >      element->setNeedsStyleRecalc(FullStyleChange);
> >      if (element != documentElement())
> >          documentElement()->setNeedsStyleRecalc(FullStyleChange);
> > +    element->attach();
> > +
> >      updateStyleIfNeeded();
> 
> Why not just do a recalcStyle(Force) here too?

Sure.  I was just trying to do the least amount of updating necessary.  Changed.

> > WebCore/dom/Document.cpp:4778
> > +    // Save off the m_fullScreenRenderer to destroy only after the client has
> > +    // been notified that the renderer has changed:
> > +    RenderBox* renderer = m_fullScreenRenderer;
> > +    webkitSetFullScreenRenderer(0);
> > +    renderer->destroy();
> 
> Renderers are not refcounted, and are only normally created/destroyed in attach/detach or style changes. It worries me that you're destroying the renderer here.

Just trying to be stingy with memory.  I can not destroy the renderer.  Changed.

> > WebCore/dom/Document.cpp:4780
> > +    frame()->view()->setShouldUpdateWhileOffscreen(FALSE);
> 
> false, not FALSE
> 
> > WebCore/dom/Document.cpp:4788
> > +void Document::webkitSetFullScreenRenderer(RenderBox* renderer)
> > +{
> > +    m_fullScreenRenderer = renderer;
> > +    page()->chrome()->client()->fullScreenRendererChanged(m_fullScreenRenderer);
> > +}
> 
> Where is the corresponding Document.h change?

Whoops, somehow the Document.h file escaped the diff.  I'll regenerate.

> This method does not need the webkit prefix.

Changed.

> > WebCore/dom/Node.cpp:1338
> > +    if (document()->webkitFullScreen() && document()->webkitCurrentFullScreenElement() == this) {
> 
> document()->webkitFullScreen() is a bad method name. It should be isFullscreen(). Only methods exposed via IDL need the webkit prefix.

That function is exposed via the IDL.  I agree, it's a bad name, and I'd much rather it was isFullScreen() too.  Should we ignore the spec?

> > WebCore/dom/Node.cpp:1345
> > +        RenderFullScreen* fullscreenRenderer = new (document()->renderArena()) RenderFullScreen(document());
> > +        fullscreenRenderer->setStyle(RenderFullScreen::createFullScreenStyle());
> > +        parentRenderer->addChild(fullscreenRenderer, 0);
> > +        parentRenderer = fullscreenRenderer;
> > +        nextRenderer = 0;
> > +        document()->webkitSetFullScreenRenderer(fullscreenRenderer);
> > +    }
> 
> What prevents this renderer from being destroyed if JS runs in the document, and the parent's child renderers are re-computed?

Nothing.  But then the re-computation will create a new RenderFullScreen object, and in the above function will call document()->setFullScreenRenderer().  That will in turn notify all the clients.
Comment 33 Dave Hyatt 2010-12-02 12:47:28 PST
I have many objections to these patches.

(1) I don't understand why RenderFullScreen is needed.  What purpose is it serving?

(2) I dislike passing RenderObjects as arguments through the ChromeClient.  While this can be made to compile, it feels pretty ugly to me.  Renderers are really supposed to be internal to WebCore.  Why not just use the element instead of the renderer?

(3) I don't understand caching a renderer in Document for full-screen.  You already know the full-screen element. Why do you have to cache a renderer too?

(4) Why do we have a pseudo-class for full-screen?  I don't understand what purpose that serves.  Why do we have to have a fullscreen.css file?  Why not just teach img and video what to do in their RenderObjects?

This feels way more complicated than it needs to be to me, unless there's something I'm missing.
Comment 34 Simon Fraser (smfr) 2010-12-02 12:56:30 PST
(In reply to comment #32)
> > document()->webkitFullScreen() is a bad method name. It should be isFullscreen(). Only methods exposed via IDL need the webkit prefix.

We should work to get the spec changed.
Comment 35 Simon Fraser (smfr) 2010-12-02 12:58:05 PST
(In reply to comment #31)
> (In reply to comment #29)
> > In general I think this feature should still work if accelerated compositing is disabled.
> 
> Fullscreen for video elements currently is disabled if accelerated compositing is not available.  Why is this different?

Because this is generic fullscreen.
Comment 36 Jer Noble 2010-12-02 13:03:44 PST
(In reply to comment #30)
> (From update of attachment 73797 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73797&action=review
> 
> > WebKit/mac/WebView/WebFullscreenController.h:2
> > + * Copyright (C) 2009 Apple Inc. All rights reserved.
> 
> 2010

Changed.

> > WebKit/mac/WebView/WebFullscreenController.h:42
> > +    RefPtr<WebCore::Element> _element; // (retain)
> > +    WebCore::RenderBox* _renderer; // (set)
> > +    RefPtr<WebCore::Element> _replacementElement; // (retain)
> 
> No need for the //retain comments on C++ things; it's obvious.

Removed.

> > WebKit/mac/WebView/WebFullscreenController.mm:83
> > +using WebCore::AnimationList;
> > +using WebCore::Animation;
> > +using WebCore::DOMWindow;
> > +using WebCore::Document;
> > +using WebCore::Element;
> > +using WebCore::Event;
> > +using WebCore::EventListener;
> > +using WebCore::EventNames;
> > +using WebCore::FloatRect;
> > +using WebCore::FloatSize;
> > +using WebCore::GraphicsLayer;
> > +using WebCore::HTMLMediaElement;
> > +using WebCore::HTMLNames::videoTag;
> > +using WebCore::Length;
> > +using WebCore::LengthType;
> > +using WebCore::Node;
> > +using WebCore::NodeList;
> > +using WebCore::RenderBlock;
> > +using WebCore::RenderBox;
> > +using WebCore::RenderFullScreen;
> > +using WebCore::RenderLayer;
> > +using WebCore::RenderLayerBacking;
> > +using WebCore::RenderObject;
> > +using WebCore::RenderStyle;
> > +using WebCore::ScriptExecutionContext;
> > +
> 
> Why not just "using namespace WebCore"?

Just trying not to pull in more than I needed.  Changed.

> > WebKit/mac/WebView/WebFullscreenController.mm:144
> > +    [_tickleTimer invalidate];
> > +    [_tickleTimer release];
> > +    _tickleTimer = nil;
> 
> There's a retain cycle between the timer and self. You'll never hit dealloc with a non-nil timer.

It will always be a retain loop anyway, since as a repeating timer, it's retained by the run loop until someone calls invalidate on it.  And I was specifically instructed in the review of WebVideoFullscreenController to retain the _tickleTimer.  But you're right, this should probably be an ASSERT(!_tickleTimer).

> > WebKit/mac/WebView/WebFullscreenController.mm:156
> > +#ifdef BUILDING_ON_TIGER
> > +    // WebFullscreenController is not supported on Tiger:
> > +    ASSERT_NOT_REACHED();
> 
> This is odd. Why not assert earlier, or #ifdef all the code out on Tiger?

We do assert earlier, in setElement:, and in windowDidLoad:.  Are you saying we shouldn't assert here, because we probably already asserted earlier?

> > WebKit/mac/WebView/WebFullscreenController.mm:210
> > +    if (_element) {
> > +        DOMWindow* window = _element->document()->domWindow();
> > +        if (window) {
> > +            window->removeEventListener(eventNames.playEvent,  _mediaEventListener.get(), true);
> > +            window->removeEventListener(eventNames.pauseEvent, _mediaEventListener.get(), true);
> > +            window->removeEventListener(eventNames.endedEvent, _mediaEventListener.get(), true);
> > +        }
> > +    }
> > +        
> > +    _element = element;
> > +    
> > +    if (_element) {
> > +        DOMWindow* window = _element->document()->domWindow();
> > +        if (window) {
> > +            window->addEventListener(eventNames.playEvent,  _mediaEventListener, true);
> > +            window->addEventListener(eventNames.pauseEvent, _mediaEventListener, true);
> > +            window->addEventListener(eventNames.endedEvent, _mediaEventListener, true);
> > +        }
> > +    }
> 
> Can you explain what you're doing here? Why are media events special? What if the content has registered event listeners on the old window?

We're listening for play/pause events so that we can disable the screensaver.  This is to keep feature parity with the old video fullscreen mode.

> > WebKit/mac/WebView/WebFullscreenController.mm:274
> > +        [[[self webView] window] orderOut:self];
> 
> Is this hiding the browser window? Is that the right thing to do in all cases (including non-browser clients)?

We probably don't really want the user to be able to interact with the non-fullscreen part of the window (in the multi-monitor straddling corner case).  I guess we could cancel fullscreen mode if the original window became active instead.  But I'll remove this for now.

> > WebKit/mac/WebView/WebFullscreenController.mm:312
> > +    [[self window] setFrame:[[[self window] screen] frame] display:YES];
> 
> Maybe cache [self window]

Sure.

> > WebKit/mac/WebView/WebFullscreenController.mm:378
> > +    // Set up the final style of the FullScreen render block.  Set an absolute
> > +    // width and height equal to the size of the screen, and anchor the layer
> > +    // at the top, left at (0,0).  The RenderFullScreen style is already set 
> > +    // to position:fixed.
> > +    PassRefPtr<RenderStyle> newStyle = RenderStyle::clone(_renderer->style());
> > +    newStyle->setWidth(Length((int)screenFrame.size.width, WebCore::Fixed));
> > +    newStyle->setHeight(Length((int)screenFrame.size.height, WebCore::Fixed));
> > +    newStyle->setTop(Length(0, WebCore::Fixed));
> > +    newStyle->setLeft(Length(0, WebCore::Fixed));
> > +    _renderer->setStyle(newStyle);
> 
> It's weird to be running this code in WebKit. You should move it to WebCore somehow.

I'm going to have to pass in a width and a height somehow.  I don't see a way to do that which would be less weird.

> > WebKit/mac/WebView/WebFullscreenController.mm:389
> > +    // Cause the document to layout, thus calculating a new fullscreen element size:
> > +    [self _document]->updateLayout();
> > +    
> > +    RenderBox* childRenderer = _renderer->firstChildBox();
> > +    CGRect destinationFrame = CGRectMake(childRenderer->x(), childRenderer->y(), childRenderer->width(), childRenderer->height());
> > +    
> > +    // Some properties haven't propogated from the GraphicsLayer to the CALayer yet.  So
> > +    // tell the renderer's layer to sync it's compositing state:
> > +    GraphicsLayer* rendererGraphics = _renderer->layer()->backing()->graphicsLayer();
> > +    rendererGraphics->syncCompositingState();    
> 
> Maybe move this code too. Also don't assume that you have any GraphicsLayers; accelerated composting might be disabled.

Fullscreen should be disabled if accelerated compositing is not available.

> > WebKit/mac/WebView/WebFullscreenController.mm:586
> > +    _tickleTimer = [[NSTimer scheduledTimerWithTimeInterval:tickleTimerInterval target:self selector:@selector(_tickleTimerFired) userInfo:nil repeats:YES] retain];
> 
> This creates a retain cycle between the timer and self.

See above.  Because self is the only thing capable of canceling the timer, it's a retain cycle regardless.

> > WebKit/mac/WebView/WebFullscreenController.mm:651
> > +- (BOOL)_isAnyMoviePlaying
> > +{
> > +    if (!_element)
> > +        return NO;
> > +    
> > +    Node* nextNode = _element.get();
> > +    while (nextNode)
> > +    {
> > +        if (nextNode->hasTagName(videoTag)) {
> > +            HTMLMediaElement* element = static_cast<HTMLMediaElement*>(nextNode);
> > +            if (!element->paused() && !element->ended())
> > +                return YES;
> > +        }
> > +        
> > +        nextNode = nextNode->traverseNextNode(_element.get());
> > +    }
> > +    
> > +    return NO;
> > +}
> > +
> 
> What about movies in subframes?

They won't count for the purposes of disabling the screensaver.
Comment 37 Jer Noble 2010-12-02 13:12:44 PST
(In reply to comment #33)
> I have many objections to these patches.
> 
> (1) I don't understand why RenderFullScreen is needed.  What purpose is it serving?

It's there to guarantee a CALayer is created (for the purpose of the enter- and exit-fullscreen animations.)

> (2) I dislike passing RenderObjects as arguments through the ChromeClient.  While this can be made to compile, it feels pretty ugly to me.  Renderers are really supposed to be internal to WebCore.  Why not just use the element instead of the renderer?

Because, for the animation to work, I need access to the CALayer backing the RenderLayer.  

> (3) I don't understand caching a renderer in Document for full-screen.  You already know the full-screen element. Why do you have to cache a renderer too?

I guess that could be replaced by asking the fullscreen element's renderer for it's parent renderer.

> (4) Why do we have a pseudo-class for full-screen?  I don't understand what purpose that serves.  Why do we have to have a fullscreen.css file?  Why not just teach img and video what to do in their RenderObjects?

Because (1) it's part of the spec, and (2) authors may want different styling for their objects once they enter full screen mode.

> This feels way more complicated than it needs to be to me, unless there's something I'm missing.

The objective I was given was to create an arbitrary element fullscreen experience which was indistinguishable in performance and look-and-feel from the existing video element fullscreen support.  The complexity in the patch reflects how difficult that is to achieve.  This proposed technique came out of discussions between myself, Maciej, Daren, and Dan.  I'm open to alternative proposals for different techniques.  In that case, this will be the third fullscreen implementation that I've written then thrown away.
Comment 38 Jer Noble 2010-12-02 13:20:27 PST
(In reply to comment #35)
> (In reply to comment #31)
> > (In reply to comment #29)
> > > In general I think this feature should still work if accelerated compositing is disabled.
> > 
> > Fullscreen for video elements currently is disabled if accelerated compositing is not available.  Why is this different?
> 
> Because this is generic fullscreen.

The primary use case for this work is to take video elements fullscreen (with custom controls).  I don't really see the difference.  If accelerated compositing isn't available, the performance of drawing 30 fps video across a fullscreen window in software-only mode is going to be atrocious.
Comment 39 Jer Noble 2010-12-02 13:43:28 PST
(In reply to comment #26)
> (From update of attachment 73796 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73796&action=review
> 
> > WebCore/css/fullscreen.css:2
> >  :-webkit-full-screen {
> > -    position:fixed;
> > -    top:0;
> > -    left:0;
> > -    right:0;
> > -    bottom:0;
> > +    background-color: white;
> 
> Why do we no longer use position:fixed etc?

Because the parent renderer of the fullscreen element is now a flex box.  

> > WebCore/dom/Element.h:164
> > +    IntRect screenRect();
> 
> Should be const. Needs as comment saying what it really means, or a better name, like boundingBoxInScreenCoordinates().

This method was simply moved up the hierarchy chain from HTMLMediaElement.  Changing it's name would require unrelated changes elsewhere in the project.  I suggest a separate bug for that.  Making it const should be no problem though.
Comment 40 Jer Noble 2010-12-02 14:43:47 PST
(In reply to comment #36)
> > 
> > It's weird to be running this code in WebKit. You should move it to WebCore somehow.
> 
> I'm going to have to pass in a width and a height somehow.  I don't see a way to do that which would be less weird.

Actually, I might be able to add a "setFullscreenRendererSize(int, int)" function which does the equivalent, but pushes the implementation back down into webkit.  Let me look into that.
Comment 41 Jer Noble 2010-12-03 15:09:58 PST
(In reply to comment #22)
> Hi. I'll be implementing full screen support in Chromium, so I was reading through these patches.
> 
> > WebCore/dom/Document.cpp:4764
> > +    element->detach();
> >      // m_fullScreenElement has already been cleared; recalc the style of 
> >      // the passed in element instead.
> >      element->setNeedsStyleRecalc(FullStyleChange);
> >      if (element != documentElement())
> >          documentElement()->setNeedsStyleRecalc(FullStyleChange);
> > +    element->attach();
> It's not clear to me why the element needs to be detached then attached. An explanation might be helpful.
> 
> Looking at Node::setNeedsStyleRecalc(), it appears that the detach() call might result in the element->setNeedsStyleRecalc(FullStyleChange) call having no effect. Maybe I'm missing something.

I looked into this, and it appears you're right.   However, the renderer gets created anyway during updateStyleIfNeeded(), despite that setNeedsStyleRecalc() has had no effect.  Regardless, I've swapped the order of the detach() and the setNeedsStyleRecalc().
Comment 42 Jer Noble 2010-12-03 15:13:21 PST
Created attachment 75552 [details]
WebCore-DOM

Addressed Simon's and David's reviews.
Comment 43 WebKit Review Bot 2010-12-03 15:18:12 PST
Attachment 75552 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/dom/Document.cpp', u'WebCore/dom/Document.h', u'WebCore/dom/Node.cpp', u'WebCore/page/ChromeClient.h']" exit_code: 1
WebCore/dom/Document.cpp:4884:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 44 Simon Fraser (smfr) 2010-12-03 15:22:34 PST
Comment on attachment 75552 [details]
WebCore-DOM

Isn't this patch incomplete? Where are the idl changes for webkitIsFullScreen?
Comment 45 Jer Noble 2010-12-03 15:29:38 PST
(In reply to comment #44)
> (From update of attachment 75552 [details])
> Isn't this patch incomplete? Where are the idl changes for webkitIsFullScreen?

You're right, I'll recreate it and repost.
Comment 46 Jer Noble 2010-12-03 15:30:13 PST
Created attachment 75558 [details]
WebCore-DOM
Comment 47 Jer Noble 2010-12-03 15:31:25 PST
Created attachment 75559 [details]
WebCore-DOM

Whoops, that last patch was missing Document.cpp.
Comment 48 WebKit Review Bot 2010-12-03 15:39:28 PST
Attachment 75559 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/dom/Document.cpp', u'WebCore/dom/Document.h', u'WebCore/dom/Document.idl', u'WebCore/dom/Node.cpp', u'WebCore/page/ChromeClient.h']" exit_code: 1
WebCore/dom/Document.cpp:4884:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 49 WebKit Review Bot 2010-12-03 15:46:56 PST
Attachment 75552 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6775046
Comment 50 Jer Noble 2010-12-03 15:48:43 PST
Created attachment 75564 [details]
WebCore-DOM

Addressed the StyleBot and Simon's PassRefPtr review.
Comment 51 Jer Noble 2010-12-03 16:13:07 PST
Created attachment 75574 [details]
WebKit-WebFullScreenController

Addressed Simon's review.
Comment 52 WebKit Review Bot 2010-12-03 16:17:06 PST
Attachment 75574 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKit/ChangeLog', u'WebKit/WebKit.xcodeproj/project.pbxproj', u'WebKit/mac/ChangeLog', u'WebKit/mac/WebView/WebFullscreenController.h', u'WebKit/mac/WebView/WebFullscreenController.mm']" exit_code: 1
WebKit/mac/WebView/WebFullscreenController.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/mac/WebView/WebFullscreenController.h:40:  _element is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:41:  _renderer is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:42:  _replacementElement is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:46:  _rendererLayer is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:47:  _backgroundLayer is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:48:  _mediaEventListener is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:50:  _isAnimating is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:51:  _isFullscreen is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:52:  _isWindowLoaded is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:53:  _forceDisableAnimation is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:54:  _isPlaying is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:55:  _idleDisplaySleepAssertion is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:56:  _idleSystemSleepAssertion is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:58:  _savedUIMode is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:59:  _savedUIOptions is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:60:  _initialFrame is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 17 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 53 WebKit Review Bot 2010-12-03 16:25:53 PST
Attachment 75564 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6748042
Comment 54 Early Warning System Bot 2010-12-03 16:29:30 PST
Attachment 75564 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6778033
Comment 55 Build Bot 2010-12-03 18:25:24 PST
Attachment 75564 [details] did not build on win:
Build output: http://queues.webkit.org/results/6763033
Comment 56 Eric Seidel (no email) 2010-12-03 22:10:29 PST
Attachment 75564 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6753044
Comment 57 Eric Seidel (no email) 2010-12-04 00:55:22 PST
Attachment 75574 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6780032
Comment 58 Eric Seidel (no email) 2010-12-04 23:26:31 PST
Attachment 75564 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6744044
Comment 59 Jer Noble 2010-12-06 11:29:51 PST
(In reply to comment #25)
> (From update of attachment 73795 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73795&action=review
> 
> There's too much special magic here, and it's not clear to me how much is just for the fullscreen animation.

It's all necessary for the fullscreen animation.  Without the changes in the patch, the fullscreen layer will not update during the animation.

> I don't think the fullscreen layers should be disconnected at all, so they shoudn't need updating separately.

Given the requirements of the feature, what do you suggest be done instead?

> Is this only true during animation to fullscreen, or when in fullscreen? Why isn't the viewport fullscreen when in fullscreen?

For now, this is true during the animation AND the entire time the layer is in fullscreen.  This patch does not address reparenting the WebView into the fullscreen window.  That will be addressed in a future patch.
Comment 60 Simon Fraser (smfr) 2010-12-06 11:43:07 PST
I understand that the animation is important, but I'd much rather get the basics of the feature working first, and then focus on the animation second.
Comment 61 Jer Noble 2010-12-06 11:53:10 PST
(In reply to comment #60)
> I understand that the animation is important, but I'd much rather get the basics of the feature working first, and then focus on the animation second.

Once this patch goes in, I can start working on the rest of the feature.  Do you have any suggestions for the patch at hand?
Comment 62 Philippe Normand 2010-12-09 10:35:56 PST
CCiny myself as I'd be interested to work on support for this on GTK too. Some initial support already landed in r73624
Comment 63 Jer Noble 2010-12-21 11:13:31 PST
Created attachment 77133 [details]
NewFullScreen-Part-1-RenderFullScreen
Comment 64 Jer Noble 2010-12-21 11:14:26 PST
Created attachment 77134 [details]
NewFullScreen-Part-2-The-Document
Comment 65 Jer Noble 2010-12-21 11:15:02 PST
Created attachment 77135 [details]
NewFullScreen-Part-3-Renderering
Comment 66 WebKit Review Bot 2010-12-21 11:16:06 PST
Attachment 77133 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/WebCore.xcodeproj/project.pbxproj', u'WebCore/css/fullscreen.css', u'WebCore/rendering/RenderFullScreen.cpp', u'WebCore/rendering/RenderFullScreen.h', u'WebCore/rendering/RenderObject.h']" exit_code: 1
WebCore/rendering/RenderFullScreen.cpp:27:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
WebCore/rendering/RenderFullScreen.cpp:41:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 67 Jer Noble 2010-12-21 11:16:28 PST
Created attachment 77136 [details]
NewFullScreen-Part-4-Element-screenRect.patch
Comment 68 Jer Noble 2010-12-21 11:17:06 PST
Created attachment 77137 [details]
NewFullScreen-Part-5-WebFullscreenController
Comment 69 WebKit Review Bot 2010-12-21 11:20:06 PST
Attachment 77137 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/WebCore.exp.in', u'WebKit/mac/ChangeLog', u'WebKit/mac/WebView/WebFullscreenController.h', u'WebKit/mac/WebView/WebFullscreenController.mm']" exit_code: 1
WebKit/mac/WebView/WebFullscreenController.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/mac/WebView/WebFullscreenController.h:40:  _element is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:41:  _renderer is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:42:  _replacementElement is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:46:  _placeholderView is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:47:  _rendererLayer is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:48:  _backgroundLayer is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:49:  _mediaEventListener is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:51:  _isAnimating is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:52:  _isFullscreen is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:53:  _isWindowLoaded is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:54:  _forceDisableAnimation is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:55:  _isPlaying is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:56:  _idleDisplaySleepAssertion is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:57:  _idleSystemSleepAssertion is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:59:  _savedUIMode is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:60:  _savedUIOptions is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/mac/WebView/WebFullscreenController.h:61:  _initialFrame is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 18 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 70 Eric Seidel (no email) 2010-12-21 11:24:03 PST
Clearly check-webkit-style doesn't understand that your file contains AppKit-style objective-C.  Maybe check-webkit-style doesn't understand objective-c?
Comment 71 Jer Noble 2010-12-21 11:26:09 PST
Created attachment 77139 [details]
NewFullScreen-Part-1-RenderFullScreen

Addressed the PassRefPtr -> RefPtr issue pointed out by the style-bot.
Comment 72 Jer Noble 2010-12-21 11:29:24 PST
I've re-organized and regenerated the patchset for this feature.  Now each individual subpatch includes a ChangeLog.  (Yay, git!)

Also, this patchset addresses Simon's desire to see mouse-events, keyboard-events, keyboard-security and view reparenting work.  This patchset allows fullscreen to occur in environments where accelerated compositing is not available; in that case, there will be no animation between windowed and full screen states.  This patchset also makes explicit that the special cases in rendering only occur while the RenderFullScreen renderer is animating.
Comment 73 WebKit Review Bot 2010-12-21 11:44:38 PST
Attachment 77135 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7333035
Comment 74 Early Warning System Bot 2010-12-21 11:53:55 PST
Attachment 77135 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7217092
Comment 75 Build Bot 2010-12-21 11:58:44 PST
Attachment 77135 [details] did not build on win:
Build output: http://queues.webkit.org/results/7212093
Comment 76 WebKit Review Bot 2010-12-21 12:13:06 PST
Attachment 77135 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7214105
Comment 77 Eric Seidel (no email) 2010-12-21 12:33:11 PST
Attachment 77133 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7202099
Comment 78 WebKit Review Bot 2010-12-21 12:33:29 PST
Attachment 77135 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7248087
Comment 79 WebKit Review Bot 2010-12-21 12:50:24 PST
Attachment 77135 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7279102
Comment 80 WebKit Review Bot 2010-12-21 12:54:53 PST
Attachment 77137 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7306085
Comment 81 David Levin 2010-12-21 13:02:38 PST
(In reply to comment #70)
> Clearly check-webkit-style doesn't understand that your file contains AppKit-style objective-C.  Maybe check-webkit-style doesn't understand objective-c?

Noted and added to my work queue. (fwiw, this bug would likely benefit from dependent bugs but I'm removing myself due to all the broken build message :).)
Comment 82 Jer Noble 2010-12-21 13:03:53 PST
(In reply to comment #81)
> (In reply to comment #70)
> > Clearly check-webkit-style doesn't understand that your file contains AppKit-style objective-C.  Maybe check-webkit-style doesn't understand objective-c?
> 
> Noted and added to my work queue. (fwiw, this bug would likely benefit from dependent bugs but I'm removing myself due to all the broken build message :).)

That's a good idea.  I'll break future patch sets like this into separate, dependent bugs.
Comment 83 Eric Seidel (no email) 2010-12-21 13:32:52 PST
Attachment 77136 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7299091
Comment 84 WebKit Review Bot 2010-12-21 13:40:39 PST
Attachment 77139 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7250088
Comment 85 Simon Fraser (smfr) 2010-12-21 13:48:06 PST
Comment on attachment 77136 [details]
NewFullScreen-Part-4-Element-screenRect.patch

r=me if you fix the exports error.
Comment 86 Simon Fraser (smfr) 2010-12-21 13:58:36 PST
Comment on attachment 77139 [details]
NewFullScreen-Part-1-RenderFullScreen

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

> WebCore/rendering/RenderFullScreen.cpp:39
> +PassRefPtr<RenderStyle> RenderFullScreen::createFullScreenStyle()

Does the -webkit-full-screen pseudoclass get applied to the RenderFullScreen, or its child that is the element for which fullscreen was requested?

In general, we prefer to put styles into the UA stylesheet, rather than hardcoding them as is done here.

> WebCore/rendering/RenderFullScreen.h:37
> +    virtual bool isFullScreen() const { return true; }

I think this would be better as isRenderFullScreen(), otherwise it can be misinterpreted.
Comment 87 Jer Noble 2010-12-21 14:41:14 PST
(In reply to comment #85)
> (From update of attachment 77136 [details])
> r=me if you fix the exports error.

Now that I'm groking git, that should be an easy fix to this patch.  Thanks!
Comment 88 Jer Noble 2010-12-21 14:42:31 PST
(In reply to comment #86)
> (From update of attachment 77139 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77139&action=review
> 
> > WebCore/rendering/RenderFullScreen.cpp:39
> > +PassRefPtr<RenderStyle> RenderFullScreen::createFullScreenStyle()
> 
> Does the -webkit-full-screen pseudoclass get applied to the RenderFullScreen, or its child that is the element for which fullscreen was requested?

To the element.

> In general, we prefer to put styles into the UA stylesheet, rather than hardcoding them as is done here.

How would the styles get applied to a renderer with no matching element?

> > WebCore/rendering/RenderFullScreen.h:37
> > +    virtual bool isFullScreen() const { return true; }
> 
> I think this would be better as isRenderFullScreen(), otherwise it can be misinterpreted.

Sure.
Comment 89 David Levin 2010-12-22 00:26:18 PST
(In reply to comment #70)
> Clearly check-webkit-style doesn't understand that your file contains AppKit-style objective-C.  Maybe check-webkit-style doesn't understand objective-c?

fyi, https://bugs.webkit.org/show_bug.cgi?id=51452
Comment 90 Simon Fraser (smfr) 2011-01-03 16:41:28 PST
Comment on attachment 77135 [details]
NewFullScreen-Part-3-Renderering

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

I'd like to see one more rev of this patch.

> WebCore/ChangeLog:16
> +        The RenderFullScreen is intended to be used by clients of that API to allow a DOM subtree to
> +        be rendered outside its original Frame. Because of this, there are a few areas of the
> +        rendering code which need to be special cased: RenderFullScreen layers should not be clipped
> +        to the viewport, as they will almost always be rendering outside the viewport area;
> +        RenderFullScreen graphics layers should not be reparented by the RenderLayerCompositor, as
> +        the client will likely want to reparent the platformLayer into their own fullscreen platform
> +        window; the FrameView must update the RenderFullScreen graphics layer tree separately from
> +        the root layer, as the two trees are disconnected.

Rendering outside the viewport is only true now when animating, correct? If so, the changelog should be updated to say this.

> WebCore/page/FrameView.cpp:542
> +    if (document->webkitIsFullScreen() && document->webkitFullScreenRenderer() && document->webkitFullScreenRenderer()->isAnimating())

Maybe you could add a Document::isRunningFullscreenAnimation() method that encapsulates those tests.

> WebCore/page/FrameView.cpp:646
> +        if (GraphicsLayer* fullScreenLayer = toRenderBox(fullScreenRenderer)->layer()->backing()->graphicsLayer())

I think you should null-check layer()->backing().

> WebCore/page/FrameView.cpp:2153
> +            if (GraphicsLayer* fullScreenLayer = toRenderBox(fullScreenRenderer)->layer()->backing()->graphicsLayer())

Ditto.

> WebCore/rendering/RenderLayerBacking.cpp:63
> +#if ENABLE(FULLSCREEN_API)
> +#include "RenderFullScreen.h"
> +#endif
> +

Not sure why this include is necessary.

> WebCore/rendering/RenderLayerBacking.cpp:185
> +        && !layerOrAncestorIsFullScreen(m_owningLayer)

It's unfortunate that the common case of layerOrAncestorIsFullScreen() walks the whole tree. Maybe test whether the document has a fullscreen renderer first?

> WebCore/rendering/RenderLayerCompositor.cpp:819
> +#if ENABLE(FULLSCREEN_API)
> +        // For the sake of clients of the full screen renderer, don't reparent
> +        // the full screen layer out from under them if they're in the middle of
> +        // animating.
> +        if (layer->renderer()->isFullScreen() && toRenderFullScreen(layer->renderer())->isAnimating())
> +            return;
> +#endif

What happens to ensure that layers are correctly updated once the animation is complete? Do we just rely on the style change? Is there a way we can make this special-casing unnecessary?
Comment 91 Simon Fraser (smfr) 2011-01-03 17:07:45 PST
Comment on attachment 77137 [details]
NewFullScreen-Part-5-WebFullscreenController

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

> WebKit/mac/ChangeLog:44
> +        * WebView/WebFullscreenController.h: Added.
> +        * WebView/WebFullscreenController.mm: Added.
> +        (-[WebFullscreenController windowDidExitFullscreen:]):  Close the fullscreen window.
> +        (-[WebFullscreenController windowDidEnterFullscreen:]): Swap the webView back into the fullscreen window. 
> +        (-[WebFullscreenController animationDidStop:finished:]): Call windowDid{Exit|Enter}FullScreen as appropriate.
> +        (-[WebFullscreenController applicationDidResignActive:]):
> +        (-[WebFullscreenController applicationDidChangeScreenParameters:]): Resize the fullscreen window to match
> +            the new screen parameters.
> +        (-[WebFullscreenController enterFullscreen:]):  Set up the animation that will take the fullscreen element
> +            from its original screen rect into fullscreen.
> +        (-[WebFullscreenController exitFullscreen]): Swap the webView back into its original window.
> +            Set up the animation that will take the fullscreen element back into its original screen
> +            rect.
> +        (-[WebFullscreenController _updatePowerAssertions]): Now checks _isAnyMoviePlaying to determine 
> +            whether to disable screensaver and sleep.
> +        (-[WebFullscreenController _isAnyMoviePlaying]): Walks through the sub-tree starting at the fullscreen element
> +            looking for HTMLVideoElements; returns whether any are found to be playing.
> +        (-[WebFullscreenController _animationDuration]): Returns the current animation duration, affected by control
> +            and shift keys.
> +        (-[WebFullscreenWindow canBecomeKeyWindow]): Allow the window to become key.
> +        (-[WebFullscreenWindow keyDown:]): Handle the 'Esc' key.
> +        (-[WebFullscreenWindow cancelOperation:]): Request to exit fullscreen.
> +        (-[WebFullscreenWindow rendererLayer]): Convenience accessor.
> +        (-[WebFullscreenWindow setRendererLayer:]): Ditto.
> +        (-[WebFullscreenWindow backgroundLayer]): Ditto.
> +        (-[WebFullscreenWindow animationView]): Ditto.
> +        (MediaEventListener::MediaEventListener): Implements the EventListener protocol.
> +        (MediaEventListener::handleEvent): Tells its delegate to _updatePowerAssertions.

You should use FullScreen, not Fullscreen, consistently, for the classes, methods and filenames.

> WebKit/mac/WebView/WebFullscreenController.mm:33
> +#import <IOKit/pwr_mgt/IOPMLib.h>

Why not IOKit/IOKit.h?

> WebKit/mac/WebView/WebFullscreenController.mm:57
> +static const NSString* isFullscreenKey = @"isFullscreen";

Should be NSString * const IsFullScreenKey, or, given its usage, IsEnteringFullScreenKey

> WebKit/mac/WebView/WebFullscreenController.mm:88
> +@interface WebFullscreenController()

We usually name such a category Private

> WebKit/mac/WebView/WebFullscreenController.mm:105
> +#pragma mark -
> +#pragma mark Initialization

We don't use #pragma mark

> WebKit/mac/WebView/WebFullscreenController.mm:189
> +    if (_element) {
> +        DOMWindow* window = _element->document()->domWindow();
> +        if (window) {
> +            window->removeEventListener(eventNames.playEvent,  _mediaEventListener.get(), true);
> +            window->removeEventListener(eventNames.pauseEvent, _mediaEventListener.get(), true);
> +            window->removeEventListener(eventNames.endedEvent, _mediaEventListener.get(), true);
> +        }
> +    }
> +        
> +    _element = element;
> +    
> +    if (_element) {
> +        DOMWindow* window = _element->document()->domWindow();
> +        if (window) {
> +            window->addEventListener(eventNames.playEvent,  _mediaEventListener, true);
> +            window->addEventListener(eventNames.pauseEvent, _mediaEventListener, true);
> +            window->addEventListener(eventNames.endedEvent, _mediaEventListener, true);
> +        }
> +    }

Please add some comments about why you need to register this event handler.

> WebKit/mac/WebView/WebFullscreenController.mm:271
> +        WebFullscreenWindow* window = [self _fullscreenWindow];

The * go on the right for Obj-C objects, so WebFullscreenWindow *window =

> WebKit/mac/WebView/WebFullscreenController.mm:287
> +    BOOL isFullscreenAnimation = [[anim valueForKey:isFullscreenKey] boolValue];

Variable name is ambiguous. Should be isEnteringFullscreenAnimation

> WebKit/mac/WebView/WebFullscreenController.mm:407
> +    CGRect destinationFrame = CGRectMake(childRenderer->x(), childRenderer->y(), childRenderer->width(), childRenderer->height());    

childRenderer->x(), childRenderer->y() are not the correct way to find out where an element is. X and Y can be relative to some enclosing element (e.g. something with a transform). Maybe use getBoundingClientRect?

> WebKit/mac/WebView/WebFullscreenController.mm:498
> +    // webView was moved to the fullscreen window.  Check too see

One space after periods please. Typo at "too see".

> WebKit/mac/WebView/WebFullscreenController.mm:502
> +    if (_placeholderView && [_placeholderView window])
> +    {

Brace should be on previous line.

> WebKit/mac/WebView/WebFullscreenController.mm:663
> +#if !defined(BUILDING_ON_TIGER) // IOPMAssertionCreateWithName not defined on < 10.5
> +- (void)_disableIdleDisplaySleep
> +{
> +    if (_idleDisplaySleepAssertion == kIOPMNullAssertionID) 
> +#if defined(BUILDING_ON_LEOPARD) // IOPMAssertionCreateWithName is not defined in the 10.5 SDK
> +        IOPMAssertionCreate(kIOPMAssertionTypeNoDisplaySleep, kIOPMAssertionLevelOn, &_idleDisplaySleepAssertion);
> +#else // IOPMAssertionCreate is depreciated in > 10.5
> +        IOPMAssertionCreateWithName(kIOPMAssertionTypeNoDisplaySleep, kIOPMAssertionLevelOn, CFSTR("WebKit playing a video fullscreen."), &_idleDisplaySleepAssertion);
> +#endif
> +}
> +
> +- (void)_enableIdleDisplaySleep
> +{
> +    if (_idleDisplaySleepAssertion != kIOPMNullAssertionID) {
> +        IOPMAssertionRelease(_idleDisplaySleepAssertion);
> +        _idleDisplaySleepAssertion = kIOPMNullAssertionID;
> +    }
> +}
> +
> +- (void)_disableIdleSystemSleep
> +{
> +    if (_idleSystemSleepAssertion == kIOPMNullAssertionID) 
> +#if defined(BUILDING_ON_LEOPARD) // IOPMAssertionCreateWithName is not defined in the 10.5 SDK
> +        IOPMAssertionCreate(kIOPMAssertionTypeNoIdleSleep, kIOPMAssertionLevelOn, &_idleSystemSleepAssertion);
> +#else // IOPMAssertionCreate is depreciated in > 10.5
> +    IOPMAssertionCreateWithName(kIOPMAssertionTypeNoIdleSleep, kIOPMAssertionLevelOn, CFSTR("WebKit playing a video fullscreen."), &_idleSystemSleepAssertion);
> +#endif
> +}
> +
> +- (void)_enableIdleSystemSleep
> +{
> +    if (_idleSystemSleepAssertion != kIOPMNullAssertionID) {
> +        IOPMAssertionRelease(_idleSystemSleepAssertion);
> +        _idleSystemSleepAssertion = kIOPMNullAssertionID;
> +    }
> +}
> +

I think it would have been preferable to add this in a separate patch.

> WebKit/mac/WebView/WebFullscreenController.mm:739
> +    return (WebFullscreenWindow *)[super window];

[self window] should do, right?
Comment 92 Simon Fraser (smfr) 2011-01-03 17:19:46 PST
Comment on attachment 77134 [details]
NewFullScreen-Part-2-The-Document

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

> WebCore/dom/Document.cpp:3375
>          m_fullScreenElement->dispatchEvent(Event::create(eventNames().webkitfullscreenchangeEvent, true, false));

Is this a safe time to be calling script?

> WebCore/dom/Document.cpp:4838
> +    setNeedsStyleRecalc(SyntheticStyleChange);
> +    documentElement()->setNeedsStyleRecalc(SyntheticStyleChange);

Why are these synthetic style changes needed?

> WebCore/dom/Document.cpp:4839
> +    m_fullScreenElement->setNeedsStyleRecalc(SyntheticStyleChange);

I assume this one is required to get m_fullScreenElement a layer and compositing layer?

> WebCore/dom/Document.cpp:4851
> +    m_fullScreenElement->dispatchEvent(Event::create(eventNames().webkitfullscreenchangeEvent, true, false));

Is this a safe time to be calling script? Who calls webkitDidEnterFullScreenForElement()?

It might be safer to dispatch the event on a zero-delay timer.

> WebCore/dom/Document.cpp:4860
> +    setNeedsStyleRecalc(SyntheticStyleChange);
> +    documentElement()->setNeedsStyleRecalc(SyntheticStyleChange);

Ditto.

> WebCore/dom/Document.cpp:4896
> +void Document::setFullScreenRendererSize(int width, int height)

Use const IntSize&
Comment 93 Jer Noble 2011-01-04 10:23:41 PST
(In reply to comment #90)
> (From update of attachment 77135 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77135&action=review
> 
> I'd like to see one more rev of this patch.
> 
> > WebCore/ChangeLog:16
> > +        The RenderFullScreen is intended to be used by clients of that API to allow a DOM subtree to
> > +        be rendered outside its original Frame. Because of this, there are a few areas of the
> > +        rendering code which need to be special cased: RenderFullScreen layers should not be clipped
> > +        to the viewport, as they will almost always be rendering outside the viewport area;
> > +        RenderFullScreen graphics layers should not be reparented by the RenderLayerCompositor, as
> > +        the client will likely want to reparent the platformLayer into their own fullscreen platform
> > +        window; the FrameView must update the RenderFullScreen graphics layer tree separately from
> > +        the root layer, as the two trees are disconnected.
> 
> Rendering outside the viewport is only true now when animating, correct? If so, the changelog should be updated to say this.

Okay.

> > WebCore/page/FrameView.cpp:542
> > +    if (document->webkitIsFullScreen() && document->webkitFullScreenRenderer() && document->webkitFullScreenRenderer()->isAnimating())
> 
> Maybe you could add a Document::isRunningFullscreenAnimation() method that encapsulates those tests.

It seems like that, since this pattern is only used in FrameView.cpp, a static function called "bool isDocumentRunningFullScreenAnimation(Document*)" would suffice (instead of adding on yet another function to Document).

> > WebCore/page/FrameView.cpp:646
> > +        if (GraphicsLayer* fullScreenLayer = toRenderBox(fullScreenRenderer)->layer()->backing()->graphicsLayer())
> 
> I think you should null-check layer()->backing().
> 
> > WebCore/page/FrameView.cpp:2153
> > +            if (GraphicsLayer* fullScreenLayer = toRenderBox(fullScreenRenderer)->layer()->backing()->graphicsLayer())
> 
> Ditto.

Okay on both.

> > WebCore/rendering/RenderLayerBacking.cpp:63
> > +#if ENABLE(FULLSCREEN_API)
> > +#include "RenderFullScreen.h"
> > +#endif
> > +
> 
> Not sure why this include is necessary.

It's not.  Removed.

> > WebCore/rendering/RenderLayerBacking.cpp:185
> > +        && !layerOrAncestorIsFullScreen(m_owningLayer)
> 
> It's unfortunate that the common case of layerOrAncestorIsFullScreen() walks the whole tree. Maybe test whether the document has a fullscreen renderer first?

Sure.

> > WebCore/rendering/RenderLayerCompositor.cpp:819
> > +#if ENABLE(FULLSCREEN_API)
> > +        // For the sake of clients of the full screen renderer, don't reparent
> > +        // the full screen layer out from under them if they're in the middle of
> > +        // animating.
> > +        if (layer->renderer()->isFullScreen() && toRenderFullScreen(layer->renderer())->isAnimating())
> > +            return;
> > +#endif
> 
> What happens to ensure that layers are correctly updated once the animation is complete? Do we just rely on the style change? Is there a way we can make this special-casing unnecessary?

The final style update causes the layers to reparent. I haven't yet found a way to make this special-casing unnecessary.
Comment 94 Jer Noble 2011-01-04 11:59:10 PST
(In reply to comment #91)
> (From update of attachment 77137 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77137&action=review
> 
> You should use FullScreen, not Fullscreen, consistently, for the classes, methods and filenames.

Sure.  Renamed WebKitFullscreenController -> WebKitFullScreenController.

> > WebKit/mac/WebView/WebFullscreenController.mm:33
> > +#import <IOKit/pwr_mgt/IOPMLib.h>
> 
> Why not IOKit/IOKit.h?

There is no such header.  The closest thing is IOKit/IOKitLib.h, and that does not include the IOPMLib.h header.

> > WebKit/mac/WebView/WebFullscreenController.mm:57
> > +static const NSString* isFullscreenKey = @"isFullscreen";
> 
> Should be NSString * const IsFullScreenKey, or, given its usage, IsEnteringFullScreenKey

Changed on both counts.

> > WebKit/mac/WebView/WebFullscreenController.mm:88
> > +@interface WebFullscreenController()
> 
> We usually name such a category Private

For certain uses of categories, a continuation category (like above) is necessary, such as adding private @properties to Obj-C classes.  Also, using a continuation is safer, as the compiler checks that all the functions defined in the continuation have been implemented.  See http://www.friday.com/bbum/2009/09/11/class-extensions-explained/.

However, it's probably safe to make this a traditional category.  Changed.

> > WebKit/mac/WebView/WebFullscreenController.mm:105
> > +#pragma mark -
> > +#pragma mark Initialization
> 
> We don't use #pragma mark

There are 58 instances of #pragma mark in the WebKit codebase, excluding these.  #pragma mark is a problem in cross-platform code, but since this lives in the mac/ platform, it's perfectly safe, and it makes the code more readable and navigable in XCode.

> > WebKit/mac/WebView/WebFullscreenController.mm:189
> > +    if (_element) {
> > +        DOMWindow* window = _element->document()->domWindow();
> > +        if (window) {
> > +            window->removeEventListener(eventNames.playEvent,  _mediaEventListener.get(), true);
> > +            window->removeEventListener(eventNames.pauseEvent, _mediaEventListener.get(), true);
> > +            window->removeEventListener(eventNames.endedEvent, _mediaEventListener.get(), true);
> > +        }
> > +    }
> > +        
> > +    _element = element;
> > +    
> > +    if (_element) {
> > +        DOMWindow* window = _element->document()->domWindow();
> > +        if (window) {
> > +            window->addEventListener(eventNames.playEvent,  _mediaEventListener, true);
> > +            window->addEventListener(eventNames.pauseEvent, _mediaEventListener, true);
> > +            window->addEventListener(eventNames.endedEvent, _mediaEventListener, true);
> > +        }
> > +    }
> 
> Please add some comments about why you need to register this event handler.

Done.

> > WebKit/mac/WebView/WebFullscreenController.mm:271
> > +        WebFullscreenWindow* window = [self _fullscreenWindow];
> 
> The * go on the right for Obj-C objects, so WebFullscreenWindow *window =

I believe that rule applies only for ObjC headers.

> > WebKit/mac/WebView/WebFullscreenController.mm:287
> > +    BOOL isFullscreenAnimation = [[anim valueForKey:isFullscreenKey] boolValue];
> 
> Variable name is ambiguous. Should be isEnteringFullscreenAnimation

Changed.

> > WebKit/mac/WebView/WebFullscreenController.mm:407
> > +    CGRect destinationFrame = CGRectMake(childRenderer->x(), childRenderer->y(), childRenderer->width(), childRenderer->height());    
> 
> childRenderer->x(), childRenderer->y() are not the correct way to find out where an element is. X and Y can be relative to some enclosing element (e.g. something with a transform). Maybe use getBoundingClientRect?

getBoundingClientRect returns a PassRefPtr<ClientRect>, and ClientRect.h is not (currently) exported by WebCore.  I don't believe it will ever be possible for the childRenderer to be relative to anything other than its parent the RenderFullScreen object, since it's both position fixed, and creates a stacking context.  Is this not true?

> > WebKit/mac/WebView/WebFullscreenController.mm:498
> > +    // webView was moved to the fullscreen window.  Check too see
> 
> One space after periods please. Typo at "too see".

Whoops, changed.

> > WebKit/mac/WebView/WebFullscreenController.mm:502
> > +    if (_placeholderView && [_placeholderView window])
> > +    {
> 
> Brace should be on previous line.

Changed.

> > WebKit/mac/WebView/WebFullscreenController.mm:663
> > +#if !defined(BUILDING_ON_TIGER) // IOPMAssertionCreateWithName not defined on < 10.5
> > +- (void)_disableIdleDisplaySleep
> > +{
> > +    if (_idleDisplaySleepAssertion == kIOPMNullAssertionID) 
> > +#if defined(BUILDING_ON_LEOPARD) // IOPMAssertionCreateWithName is not defined in the 10.5 SDK
> > +        IOPMAssertionCreate(kIOPMAssertionTypeNoDisplaySleep, kIOPMAssertionLevelOn, &_idleDisplaySleepAssertion);
> > +#else // IOPMAssertionCreate is depreciated in > 10.5
> > +        IOPMAssertionCreateWithName(kIOPMAssertionTypeNoDisplaySleep, kIOPMAssertionLevelOn, CFSTR("WebKit playing a video fullscreen."), &_idleDisplaySleepAssertion);
> > +#endif
> > +}
> > +
> > +- (void)_enableIdleDisplaySleep
> > +{
> > +    if (_idleDisplaySleepAssertion != kIOPMNullAssertionID) {
> > +        IOPMAssertionRelease(_idleDisplaySleepAssertion);
> > +        _idleDisplaySleepAssertion = kIOPMNullAssertionID;
> > +    }
> > +}
> > +
> > +- (void)_disableIdleSystemSleep
> > +{
> > +    if (_idleSystemSleepAssertion == kIOPMNullAssertionID) 
> > +#if defined(BUILDING_ON_LEOPARD) // IOPMAssertionCreateWithName is not defined in the 10.5 SDK
> > +        IOPMAssertionCreate(kIOPMAssertionTypeNoIdleSleep, kIOPMAssertionLevelOn, &_idleSystemSleepAssertion);
> > +#else // IOPMAssertionCreate is depreciated in > 10.5
> > +    IOPMAssertionCreateWithName(kIOPMAssertionTypeNoIdleSleep, kIOPMAssertionLevelOn, CFSTR("WebKit playing a video fullscreen."), &_idleSystemSleepAssertion);
> > +#endif
> > +}
> > +
> > +- (void)_enableIdleSystemSleep
> > +{
> > +    if (_idleSystemSleepAssertion != kIOPMNullAssertionID) {
> > +        IOPMAssertionRelease(_idleSystemSleepAssertion);
> > +        _idleSystemSleepAssertion = kIOPMNullAssertionID;
> > +    }
> > +}
> > +
> 
> I think it would have been preferable to add this in a separate patch.

These already exist in WebVideoFullscreenController.mm, verbatim.

> > WebKit/mac/WebView/WebFullscreenController.mm:739
> > +    return (WebFullscreenWindow *)[super window];
> 
> [self window] should do, right?

Sure.
Comment 95 Jer Noble 2011-01-04 14:06:37 PST
(In reply to comment #92)
> (From update of attachment 77134 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77134&action=review
> 
> > WebCore/dom/Document.cpp:3375
> >          m_fullScreenElement->dispatchEvent(Event::create(eventNames().webkitfullscreenchangeEvent, true, false));
> 
> Is this a safe time to be calling script?

You're right, a zero delay timer call might be appropriate here.  There are already protections against reentrancy up in WebFullScreenController, but those wouldn't be necessary if that event was dispatched in the next run-loop.

> > WebCore/dom/Document.cpp:4838
> > +    setNeedsStyleRecalc(SyntheticStyleChange);
> > +    documentElement()->setNeedsStyleRecalc(SyntheticStyleChange);
> 
> Why are these synthetic style changes needed?

That was there to accommodate the :full-screen-root-with-target pseudo class, which was removed from the spec.  These lines should be able to be removed safely.

> > WebCore/dom/Document.cpp:4839
> > +    m_fullScreenElement->setNeedsStyleRecalc(SyntheticStyleChange);
> 
> I assume this one is required to get m_fullScreenElement a layer and compositing layer?

Yes.

> > WebCore/dom/Document.cpp:4851
> > +    m_fullScreenElement->dispatchEvent(Event::create(eventNames().webkitfullscreenchangeEvent, true, false));
> 
> Is this a safe time to be calling script? Who calls webkitDidEnterFullScreenForElement()?
> 
> It might be safer to dispatch the event on a zero-delay timer.

Indeed, as above.

> > WebCore/dom/Document.cpp:4860
> > +    setNeedsStyleRecalc(SyntheticStyleChange);
> > +    documentElement()->setNeedsStyleRecalc(SyntheticStyleChange);
> 
> Ditto.
> 
> > WebCore/dom/Document.cpp:4896
> > +void Document::setFullScreenRendererSize(int width, int height)
> 
> Use const IntSize&

Sure.
Comment 96 Jer Noble 2011-01-06 11:30:02 PST
Created attachment 78136 [details]
NewFullScreen-Part-3-Renderering
Comment 97 Simon Fraser (smfr) 2011-01-06 11:52:01 PST
Comment on attachment 78136 [details]
NewFullScreen-Part-3-Renderering

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

> WebCore/page/FrameView.cpp:529
> +static bool isDocumentRunningFullScreenAnimation(Document* document)

Since the param makes it obvious this applies to a Document, isRunningFullScreenAnimation(Document*) is better I think.

> WebCore/page/FrameView.cpp:646
> +    // The fullScreenRenderer's graphicsLayer  has been re-parented, and the above recursive syncCompositingState

Double space between graphicsLayer and has.

> WebCore/rendering/RenderFullScreen.cpp:36
> -        layer()->rendererContentChanged();
> +        layer()->contentChanged(RenderLayer::VideoChanged);

I can't see enough context to know if this is an appropriate change.
Comment 98 Jer Noble 2011-01-06 13:02:20 PST
(In reply to comment #97)
> (From update of attachment 78136 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78136&action=review
> 
> > WebCore/page/FrameView.cpp:529
> > +static bool isDocumentRunningFullScreenAnimation(Document* document)
> 
> Since the param makes it obvious this applies to a Document, isRunningFullScreenAnimation(Document*) is better I think.

Sure.

> > WebCore/page/FrameView.cpp:646
> > +    // The fullScreenRenderer's graphicsLayer  has been re-parented, and the above recursive syncCompositingState
> 
> Double space between graphicsLayer and has.

Removed.

> > WebCore/rendering/RenderFullScreen.cpp:36
> > -        layer()->rendererContentChanged();
> > +        layer()->contentChanged(RenderLayer::VideoChanged);
> 
> I can't see enough context to know if this is an appropriate change.

In https://bugs.webkit.org/show_bug.cgi?id=49746, you removed the RenderLayer::rendererContentChanged() function, and replaced it with RenderLayer::ContentChanged(ContentChangeType).  The VideoChanged enum will ensure setCompositingLayersNeedRebuild() gets called.

I can add a new enum for FullScreenChanged if you like, and change the associated code in RenderLayer to match.  Or we can do that in a later patch.
Comment 99 Simon Fraser (smfr) 2011-01-06 13:04:37 PST
(In reply to comment #98)

> > > WebCore/rendering/RenderFullScreen.cpp:36
> > > -        layer()->rendererContentChanged();
> > > +        layer()->contentChanged(RenderLayer::VideoChanged);
> > 
> > I can't see enough context to know if this is an appropriate change.
> 
> In https://bugs.webkit.org/show_bug.cgi?id=49746, you removed the RenderLayer::rendererContentChanged() function, and replaced it with RenderLayer::ContentChanged(ContentChangeType).  The VideoChanged enum will ensure setCompositingLayersNeedRebuild() gets called.
> 
> I can add a new enum for FullScreenChanged if you like, and change the associated code in RenderLayer to match.  Or we can do that in a later patch.

A new enum would be good, and doing so in this patch is fine.
Comment 100 Jer Noble 2011-01-07 14:47:14 PST
Committed r75277: https://trac.webkit.org/changeset/75277
Comment 101 Eric Seidel (no email) 2011-01-08 00:15:44 PST
This broke the bots. :(
Comment 102 Eric Seidel (no email) 2011-01-08 13:06:22 PST
Nearly a day later and all the bots are still broken.

I tried a rollout, but this is a multi-revision commit (which the sheriff-bot can handle, I just need to dig up all the revisions).

This was not well handled. :(
Comment 103 Eric Seidel (no email) 2011-01-08 13:20:24 PST
It's impossible to roll this out given the WebCore rename which went in last night, sadly.
Comment 104 Eric Seidel (no email) 2011-01-08 14:16:46 PST
I don't understand why these two tests are failing, but I'm going to check in failing results to allow the bots to go green again and the commit-queue to unclog (it's at 20 patches backlog, atm).
Comment 105 Eric Seidel (no email) 2011-01-08 14:18:14 PST
Committed r75326: <http://trac.webkit.org/changeset/75326>
Comment 106 Csaba Osztrogonác 2011-01-08 14:41:27 PST
(In reply to comment #103)
> It's impossible to roll this out given the WebCore rename which went in last night, sadly.

I have an idea how can we handle this kind of rollouts manually:
- Prepare rollout on an earlier revision with webkit-patch (before WebCore rename)
- Rename WebCore in the diff file with sed
- Apply the patch on the trunk
- Be happy :)
Comment 107 Jer Noble 2011-01-09 00:13:48 PST
The LayoutTest failures were being tracked by https://bugs.webkit.org/show_bug.cgi?id=52095, but I neglected to relate this bug to that one.
Comment 108 Eric Seidel (no email) 2011-03-02 16:53:43 PST
This may have contributed to peacekeeper performance troubles documented in bug 55626.

It's also not clear to me from the change, why the FULLSCREEN_API block in Node::createRendererIfNecessary ignores nextRenderer when appending the fullscreen node, or why it clears it.
Comment 109 Jer Noble 2011-03-02 16:58:17 PST
(In reply to comment #108)
> This may have contributed to peacekeeper performance troubles documented in bug 55626.
> 
> It's also not clear to me from the change, why the FULLSCREEN_API block in Node::createRendererIfNecessary ignores nextRenderer when appending the fullscreen node, or why it clears it.

Yes, it looks like there is an error there: addChild(fullscreenRenderer, 0) should be addChild(fullscreenRenderer, nextRenderer);  

But it has to be cleared before continuing, as the next child added will be the first child of the fullscreen renderer, and thus have no existing sibling.
Comment 110 Eric Seidel (no email) 2011-03-02 16:59:09 PST
(In reply to comment #109)
> (In reply to comment #108)
> > This may have contributed to peacekeeper performance troubles documented in bug 55626.
> > 
> > It's also not clear to me from the change, why the FULLSCREEN_API block in Node::createRendererIfNecessary ignores nextRenderer when appending the fullscreen node, or why it clears it.
> 
> Yes, it looks like there is an error there: addChild(fullscreenRenderer, 0) should be addChild(fullscreenRenderer, nextRenderer);  
> 
> But it has to be cleared before continuing, as the next child added will be the first child of the fullscreen renderer, and thus have no existing sibling.

How would we test this?  Is FULLSCREEN_API enabled?
Comment 111 Jer Noble 2011-03-02 17:01:50 PST
(In reply to comment #110)
> (In reply to comment #109)
> > (In reply to comment #108)
> > > This may have contributed to peacekeeper performance troubles documented in bug 55626.
> > > 
> > > It's also not clear to me from the change, why the FULLSCREEN_API block in Node::createRendererIfNecessary ignores nextRenderer when appending the fullscreen node, or why it clears it.
> > 
> > Yes, it looks like there is an error there: addChild(fullscreenRenderer, 0) should be addChild(fullscreenRenderer, nextRenderer);  
> > 
> > But it has to be cleared before continuing, as the next child added will be the first child of the fullscreen renderer, and thus have no existing sibling.
> 
> How would we test this?  Is FULLSCREEN_API enabled?

It is enabled on WebKit/OSX builds; but the feature must be enabled at runtime by setting a user default:

defaults write com.apple.Safari WebKitFullScreenEnabled -bool true
Comment 112 Eric Seidel (no email) 2011-03-03 16:48:14 PST
Bug 55720 fixes the renderer insertion position trouble, and makes us call nextRenderer only when needed.