Implement WebKit Full Screen support

• Component: Platform
• Status: RESOLVED FIXED

[Jer Noble]

Implement the Gecko Full Screen API support inside of WebKit.

[Jer Noble]

Created attachment 73793 [details]
WebCore-RenderFullScreen

[Jer Noble]

Created attachment 73794 [details]
WebCore-DOM

[Jer Noble]

Created attachment 73795 [details]
WebCore-Rendering

[Jer Noble]

Created attachment 73796 [details]
WebCore-EverythingElse

[Jer Noble]

Created attachment 73797 [details]
WebKit-WebFullScreenController

[Jer Noble]

Created attachment 73798 [details]
WebKit-WebView

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[Jer Noble]

(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.

[Eric Seidel (no email)]

Attachment 73795 [details] did not build on mac:
Build output: http://queues.webkit.org/results/5827003

[WebKit Review Bot]

Attachment 73795 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5819003

[Early Warning System Bot]

Attachment 73795 [details] did not build on qt:
Build output: http://queues.webkit.org/results/5826003

[Daniel Bates]

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>.

[Adam Barth]

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.

[Jer Noble]

Created attachment 73911 [details]
WebCore-RenderFullScreen

Updated license text in both RenderFullScreen.cpp and RenderFullScreen.h.

[WebKit Review Bot]

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.

[Simon Fraser (smfr)]

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?

[Jer Noble]

(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.

[Andrew Scherkus]

exciting!  will skim through the patches

[Build Bot]

Attachment 73795 [details] did not build on win:
Build output: http://queues.webkit.org/results/5939095

[Eric Seidel (no email)]

Attachment 73795 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6204020

[David Dorwin]

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.

[Jer Noble]

(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.

[Simon Fraser (smfr)]

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?

[Simon Fraser (smfr)]

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?

[Simon Fraser (smfr)]

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().

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

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"

[Simon Fraser (smfr)]

In general I think this feature should still work if accelerated compositing is disabled.

[Simon Fraser (smfr)]

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?

[Jer Noble]

(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?

[Jer Noble]

(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.

[Dave Hyatt]

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.

[Simon Fraser (smfr)]

(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.

[Simon Fraser (smfr)]

(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.

[Jer Noble]

(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.

[Jer Noble]

(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.

[Jer Noble]

(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.

[Jer Noble]

(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.

[Jer Noble]

(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.

[Jer Noble]

(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().

[Jer Noble]

Created attachment 75552 [details]
WebCore-DOM

Addressed Simon's and David's reviews.

[WebKit Review Bot]

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.

[Simon Fraser (smfr)]

Comment on attachment 75552 [details]
WebCore-DOM

Isn't this patch incomplete? Where are the idl changes for webkitIsFullScreen?

[Jer Noble]

(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.

[Jer Noble]

Created attachment 75558 [details]
WebCore-DOM

[Jer Noble]

Created attachment 75559 [details]
WebCore-DOM

Whoops, that last patch was missing Document.cpp.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 75552 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6775046

[Jer Noble]

Created attachment 75564 [details]
WebCore-DOM

Addressed the StyleBot and Simon's PassRefPtr review.

[Jer Noble]

Created attachment 75574 [details]
WebKit-WebFullScreenController

Addressed Simon's review.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 75564 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6748042

[Early Warning System Bot]

Attachment 75564 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6778033

[Build Bot]

Attachment 75564 [details] did not build on win:
Build output: http://queues.webkit.org/results/6763033

[Eric Seidel (no email)]

Attachment 75564 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6753044

[Eric Seidel (no email)]

Attachment 75574 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6780032

[Eric Seidel (no email)]

Attachment 75564 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6744044

[Jer Noble]

(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.

[Simon Fraser (smfr)]

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.

[Jer Noble]

(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?

[Philippe Normand]

CCiny myself as I'd be interested to work on support for this on GTK too. Some initial support already landed in r73624

[Jer Noble]

Created attachment 77133 [details]
NewFullScreen-Part-1-RenderFullScreen

[Jer Noble]

Created attachment 77134 [details]
NewFullScreen-Part-2-The-Document

[Jer Noble]

Created attachment 77135 [details]
NewFullScreen-Part-3-Renderering

[WebKit Review Bot]

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.

[Jer Noble]

Created attachment 77136 [details]
NewFullScreen-Part-4-Element-screenRect.patch

[Jer Noble]

Created attachment 77137 [details]
NewFullScreen-Part-5-WebFullscreenController

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

Clearly check-webkit-style doesn't understand that your file contains AppKit-style objective-C.  Maybe check-webkit-style doesn't understand objective-c?

[Jer Noble]

Created attachment 77139 [details]
NewFullScreen-Part-1-RenderFullScreen

Addressed the PassRefPtr -> RefPtr issue pointed out by the style-bot.

[Jer Noble]

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.

[WebKit Review Bot]

Attachment 77135 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7333035

[Early Warning System Bot]

Attachment 77135 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7217092

[Build Bot]

Attachment 77135 [details] did not build on win:
Build output: http://queues.webkit.org/results/7212093

[WebKit Review Bot]

Attachment 77135 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7214105

[Eric Seidel (no email)]

Attachment 77133 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7202099

[WebKit Review Bot]

Attachment 77135 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7248087

[WebKit Review Bot]

Attachment 77135 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7279102

[WebKit Review Bot]

Attachment 77137 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7306085

[David Levin]

(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 :).)

[Jer Noble]

(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.

[Eric Seidel (no email)]

Attachment 77136 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7299091

[WebKit Review Bot]

Attachment 77139 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7250088

[Simon Fraser (smfr)]

Comment on attachment 77136 [details]
NewFullScreen-Part-4-Element-screenRect.patch

r=me if you fix the exports error.

[Simon Fraser (smfr)]

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.

[Jer Noble]

(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!

[Jer Noble]

(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.

[David Levin]

(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

[Simon Fraser (smfr)]

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?

[Simon Fraser (smfr)]

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?

[Simon Fraser (smfr)]

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&

[Jer Noble]

(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.

[Jer Noble]

(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.

[Jer Noble]

(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.

[Jer Noble]

Created attachment 78136 [details]
NewFullScreen-Part-3-Renderering

[Simon Fraser (smfr)]

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.

[Jer Noble]

(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.

[Simon Fraser (smfr)]

(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.

[Jer Noble]

Committed r75277: https://trac.webkit.org/changeset/75277

[Eric Seidel (no email)]

This broke the bots. :(

[Eric Seidel (no email)]

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. :(

[Eric Seidel (no email)]

It's impossible to roll this out given the WebCore rename which went in last night, sadly.

[Eric Seidel (no email)]

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).

[Eric Seidel (no email)]

Committed r75326: <http://trac.webkit.org/changeset/75326>

[Csaba Osztrogonác]

(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 :)

[Jer Noble]

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.

[Eric Seidel (no email)]

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.

[Jer Noble]

(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.

[Eric Seidel (no email)]

(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?

[Jer Noble]

(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

[Eric Seidel (no email)]

Bug 55720 fixes the renderer insertion position trouble, and makes us call nextRenderer only when needed.