Bug 59845 – Implement FULLSCREEN_API on Windows, Part 3: WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Implement FULLSCREEN_API on Windows, Part 3: WebKit2

[Jer Noble]

Created attachment 91770 [details]
Patch

[Jer Noble]

Created attachment 91772 [details]
Patch

Build fixes.

[Jeff Miller]

Comment on attachment 91772 [details]
Patch

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

> Source/WebKit2/ChangeLog:12
> +        * UIProcess/FullScreen/win/WKFullScreenWindowController.cpp: Added.

Are you sure we don't reserve WK as a prefix for WebKit2 API classes?  If so, this should probably just be FullScreenWindowController.cpp (with similar name changes to the classes).

[Adam Roben (:aroben)]

Comment on attachment 91772 [details]
Patch

Is there any way we can share more of this code with WebKit1? Maybe more of the code can live in WebCore?

[Jer Noble]

(In reply to comment #4)
> (From update of attachment 91772 [details])
> Is there any way we can share more of this code with WebKit1? Maybe more of the code can live in WebCore?

I guess we could put a virtual class down in WebCore and let the WebKit and WK2 implementations fill in the missing pieces; alternatively, we could define a "client" protocol that WebKit and WK2 would have to provide to fill in those same pieces.  I'll look into it.

[Adam Roben (:aroben)]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 91772 [details] [details])
> > Is there any way we can share more of this code with WebKit1? Maybe more of the code can live in WebCore?
> 
> I guess we could put a virtual class down in WebCore and let the WebKit and WK2 implementations fill in the missing pieces; alternatively, we could define a "client" protocol that WebKit and WK2 would have to provide to fill in those same pieces.  I'll look into it.

A new client class sounds like it would fit best with our existing strategies for sharing code.

[Jer Noble]

Created attachment 91966 [details]
Patch

Moved WebFullScreenController into WebCore and made both WebKit and WebKit2 WebView classes use this new shared class.

[Adam Roben (:aroben)]

Comment on attachment 91966 [details]
Patch

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

I don't see WebFullScreenControllerClient defined anywhere, so I only reviewed part of the patch.

> Source/WebCore/ChangeLog:11
> +        behalf by WebKit and WebKit2.  MediaPlayerPrivateFullscreenWindow now only creates a 
> +        CALayerHost once a root layer is set.

Why is this last part needed? (You should probably explain in the ChangeLog.)

> Source/WebCore/WebCore.vcproj/WebCore.vcproj:27527
> +						RelativePath="..\platform\graphics\win\WebFullScreenController.cpp"

We don't normally use the "Web" prefix for WebCore classes. Can we rename this to FullScreenController in another patch?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.cpp:107
> +        m_layerTreeHost = 0;

Please use nullptr.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.cpp:112
> +    if (!m_layerTreeHost)
> +        m_layerTreeHost = CACFLayerTreeHost::create();

Looks like we never call CACFLayerTreeHost::setWindow in the lazy-init case? How can rendering work if we don't do that?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h:61
> +    void createWindow(HWND ownerWindow, bool startVisible = true);

Optional boolean arguments make me so sad! They're usually unreadable at the call site ("what does true stand for?"), and they're easy for callers to misuse (because so many types can be implicitly converted to bool). I'd suggest making it a non-optional enum. Or maybe we should never start visible and have it be the caller's responsibility to call ShowWindow as needed.

> Source/WebCore/platform/graphics/win/WebFullScreenController.cpp:57
> +    virtual ~Private() { }

The compiler should do this for you.

[Jer Noble]

Comment on attachment 91966 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        CALayerHost once a root layer is set.
> 
> Why is this last part needed? (You should probably explain in the ChangeLog.)

Sure thing.  Basically, it interferes with child window rendering.  It marks the window region as validated when the CALayerHost draws, which leads to the WebView window not being sent WM_PAINT.  And even if that weren't the case, it still is a bit overkill to instantiate a whole CALayerHost just for drawing black into a window.  But I'll add all this to the ChangeLog.

>> Source/WebCore/WebCore.vcproj/WebCore.vcproj:27527
>> +						RelativePath="..\platform\graphics\win\WebFullScreenController.cpp"
> 
> We don't normally use the "Web" prefix for WebCore classes. Can we rename this to FullScreenController in another patch?

I can do this in this one;  I hovered on the edge of renaming it myself, but laziness won out.

>> Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.cpp:112
>> +        m_layerTreeHost = CACFLayerTreeHost::create();
> 
> Looks like we never call CACFLayerTreeHost::setWindow in the lazy-init case? How can rendering work if we don't do that?

Whoops, good catch.  If setRootChildLayer() is called before createWindow(), CACFLayerTreeHost::setWindow() will never get called.  Aaaaand, it looks like that's the order called by FullScreenVideoController.  I'll add a setWindow() call to the lazy instantiation.

>> Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h:61
>> +    void createWindow(HWND ownerWindow, bool startVisible = true);
> 
> Optional boolean arguments make me so sad! They're usually unreadable at the call site ("what does true stand for?"), and they're easy for callers to misuse (because so many types can be implicitly converted to bool). I'd suggest making it a non-optional enum. Or maybe we should never start visible and have it be the caller's responsibility to call ShowWindow as needed.

You're totally right; i'll make this an enum.  I just didn't want to affect the default behavior and have to go and make changes everywhere this is used.  (Which is, I know, only one other place.)

>> Source/WebCore/platform/graphics/win/WebFullScreenController.cpp:57
>> +    virtual ~Private() { }
> 
> The compiler should do this for you.

Are you sure?  I didn't think the compiler will make a virtual destructor by default.

[Jer Noble]

(In reply to comment #8)
> (From update of attachment 91966 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91966&action=review
> 
> I don't see WebFullScreenControllerClient defined anywhere, so I only reviewed part of the patch.

Hm, git has apparently decided, since that file was copied from one place to the other, to not include WebFullScreenController.h in the patch. I'll fix that and re-upload.

[Jer Noble]

Created attachment 91984 [details]
Patch

[Jer Noble]

Comment on attachment 91984 [details]
Patch

Whoops, some of my changes didn't make it into this patch (including the visibility enum.)  Will upload once again.

[Jer Noble]

Comment on attachment 91984 [details]
Patch

Double whoops.  Was looking at the wrong patch.  Everything should be fine.

[Jeff Miller]

Comment on attachment 91984 [details]
Patch

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

> Source/WebCore/platform/graphics/win/FullScreenController.cpp:107
> +    ASSERT(client);

This should be ASSERT_ARG(client, client).

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h:63
> +        StartVisible,

No comma needed after StartVisible.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h:65
> +    typedef bool StartingVisibilityFlag;

Is there a reason you typedef'd this to bool instead of just doing:

enum StartingVisibilityFlag {
    StartInvisible,
    StartVisible
};

?

[Jer Noble]

(In reply to comment #14)
> (From update of attachment 91984 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91984&action=review
> 
> > Source/WebCore/platform/graphics/win/FullScreenController.cpp:107
> > +    ASSERT(client);
> 
> This should be ASSERT_ARG(client, client).

Changed.

> > Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h:63
> > +        StartVisible,
> 
> No comma needed after StartVisible.

I always use a comma at the end of an enum, so that when entries are added, the comma doesn't have to be retroactively applied.  It makes for much cleaner diffs.  I realize though, in this case, that this isn't likely an enum that will be added to.  I'll remove it.

> > Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h:65
> > +    typedef bool StartingVisibilityFlag;
> 
> Is there a reason you typedef'd this to bool instead of just doing:
> 
> enum StartingVisibilityFlag {
>     StartInvisible,
>     StartVisible
> };
> 
> ?

Yes, passing enums directly into functions have led to a number of compiler errors in the past: whereas defining an enum twice will generate a compiler error, it's trivial to redefine a typedef.  (This exact issue recently came up when adding a function to WKSI.)

[Jeff Miller]

Comment on attachment 91984 [details]
Patch

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

>>> Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h:65
>>> +    typedef bool StartingVisibilityFlag;
>> 
>> Is there a reason you typedef'd this to bool instead of just doing:
>> 
>> enum StartingVisibilityFlag {
>>     StartInvisible,
>>     StartVisible
>> };
>> 
>> ?
> 
> Yes, passing enums directly into functions have led to a number of compiler errors in the past: whereas defining an enum twice will generate a compiler error, it's trivial to redefine a typedef.  (This exact issue recently came up when adding a function to WKSI.)

Isn't that only a problem when passing enums across an API boundary like WebKit2 or WKSI?  It should be fine to pass enums internally in WebCore, and I see many existing instances of this.

[Adam Roben (:aroben)]

Comment on attachment 91984 [details]
Patch

This is close! Just a few more things to clean up from the move into WebCore.

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

> Source/WebCore/platform/graphics/win/FullScreenController.h:47
> +class FullScreenControllerClient {
> +public:
> +    virtual HWND webViewWindow() const = 0;
> +    virtual HWND webViewHostWindow() const = 0;
> +    virtual void setWebViewHostWindow(HWND) = 0;
> +    virtual void webViewWillEnterFullScreen() = 0;
> +    virtual void webViewDidEnterFullScreen() = 0;
> +    virtual void webViewWillExitFullScreen() = 0;
> +    virtual void webViewDidExitFullScreen() = 0;
> +protected:
> +    virtual ~FullScreenControllerClient() { }
> +};

I usually prefer to put clients in their own header file so that headers of derived classes don't end up pulling in the whole non-client class declaration.

"WebView" isn't something that WebCore should know about, so we'll need to change these names somehow. You can probably just remove "webView" from the will/did functions.

I'd suggest using "parent window" instead of "host window". The "host window" terminology in WebKit1 is misleading (it was based on WebKit/mac's "host window" concept, but isn't the same thing), and conflicts with WebCore::HostWindow.

Chrome::platformPageClient can be used in place of webViewWindow (though it has such an awful name!). Maybe we should move [set]WebViewHostWindow there, too (if we give them appropriate names)?

>>> Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h:65
>>> +    typedef bool StartingVisibilityFlag;
>> 
>> Is there a reason you typedef'd this to bool instead of just doing:
>> 
>> enum StartingVisibilityFlag {
>>     StartInvisible,
>>     StartVisible
>> };
>> 
>> ?
> 
> Yes, passing enums directly into functions have led to a number of compiler errors in the past: whereas defining an enum twice will generate a compiler error, it's trivial to redefine a typedef.  (This exact issue recently came up when adding a function to WKSI.)

Can you point to a specific example where this was a problem? We use enum parameters in lots of places in WebKit/WebCore. I think it's definitely preferable to typedefing to bool. (But I'd also support making this behavior non-configurable and leaving it up to the caller.)

> Source/WebKit/win/WebView.cpp:6808
> +    m_fullScreenElement = 0;

Please use nullptr.

> Source/WebKit2/WebProcess/FullScreen/win/WebFullScreenManagerWin.cpp:72
> +    m_fullScreenRootLayer = layer;

I don't see any code that ever renders this layer. Am I missing something?

[Jer Noble]

(In reply to comment #17)
> (From update of attachment 91984 [details])
> This is close! Just a few more things to clean up from the move into WebCore.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=91984&action=review
> 
> > Source/WebCore/platform/graphics/win/FullScreenController.h:47
> > +class FullScreenControllerClient {
> > +public:
> > +    virtual HWND webViewWindow() const = 0;
> > +    virtual HWND webViewHostWindow() const = 0;
> > +    virtual void setWebViewHostWindow(HWND) = 0;
> > +    virtual void webViewWillEnterFullScreen() = 0;
> > +    virtual void webViewDidEnterFullScreen() = 0;
> > +    virtual void webViewWillExitFullScreen() = 0;
> > +    virtual void webViewDidExitFullScreen() = 0;
> > +protected:
> > +    virtual ~FullScreenControllerClient() { }
> > +};
> 
> I usually prefer to put clients in their own header file so that headers of derived classes don't end up pulling in the whole non-client class declaration.

Sure, though in this case, the rest of the header is about as long as this class itself, and pulls in nothing aside  from OwnPtr.h, which is why I didn't move it into its own file in the first place.  But I'll do it now.

> "WebView" isn't something that WebCore should know about, so we'll need to change these names somehow. You can probably just remove "webView" from the will/did functions.

I was trying to avoid conflicting with any existing function names.  How about:

virtual void fullScreenControllerWillEnterFullScreen(); 

> I'd suggest using "parent window" instead of "host window". The "host window" terminology in WebKit1 is misleading (it was based on WebKit/mac's "host window" concept, but isn't the same thing), and conflicts with WebCore::HostWindow.

parentWindow and setParentWindow are fine with me.
 
> Chrome::platformPageClient can be used in place of webViewWindow (though it has such an awful name!). Maybe we should move [set]WebViewHostWindow there, too (if we give them appropriate names)?

There's no way to access ChromeClient from within FullScreenWindow.  We'd have to add a function to FullScreenControllerClient which fetches the ChromeClient.  We could just replace FullScreenControllerClient with ChromeClient, and add those enter/exit functions to ChromeClient, and then I wouldn't have to split it out into its own file. ;)

>> Source/WebKit/win/WebView.cpp:6808
>> +    m_fullScreenElement = 0;
> 
> Please use nullptr.

Sure.

>> Source/WebKit2/WebProcess/FullScreen/win/WebFullScreenManagerWin.cpp:72
>> +    m_fullScreenRootLayer = layer;
> 
> I don't see any code that ever renders this layer. Am I missing something?

No, you're not.  The passed in layer /must/ have a super layer, or an assert happens when synchronizing the layers.  (The sync mechanism assumes all parentless layers are the root layer, and freaks out.)   However, on windows, since we don't animate the full screen element's layer, this function is called twice within the same runloop, once to set the layer, and the other to clear it.  The view won't even get a chance to redraw.

 I intend to fix this with the "Full Screen Flashing" bug, so that this function will never be called unnecessarily like this; see the comment about not setting RenderLayer::setAnimating().  (And I see an extra "S" at the end of that comment.)

[Jer Noble]

(In reply to comment #17)
> Can you point to a specific example where this was a problem? We use enum parameters in lots of places in WebKit/WebCore. I think it's definitely preferable to typedefing to bool. (But I'd also support making this behavior non-configurable and leaving it up to the caller.)

Jon Lee was trying to add a new WKSI function, and it was actually impossible to do when the function took an enum directly.  That was perhaps a peculiarity of the way functions are defined in WKSI, because implementing the function involved redeclaring the enum.  (Even though the functions were identically defined, they were mangled differently, because each enum is effectively unique.)  And it's unlikely to occur within a C++ class.

But typedefing to an intrinsic type solves the problem of call-site visibility, header visibility, and doesn't create any strangely mangled symbols.

However, i'm starting to come around to the idea that the default behavior should be not to make the window visible, and let the client deal with showing it.  That would solve the problem much more cleanly.

[Jer Noble]

Created attachment 92052 [details]
Patch

Addressed (most of) Adam's comments.  Found out I was misinterpreting how ::AnimateWindow() works.  (Hint: it's synchronous.)  Also, it requires windows to respond to the WM_PAINTCLIENT command, so that's been added to MediaPlayerPrivateFullScreenWindow class.

[Jer Noble]

(In reply to comment #20)
> Created an attachment (id=92052) [details]
> Patch
> 
> Addressed (most of) Adam's comments.  Found out I was misinterpreting how ::AnimateWindow() works.  (Hint: it's synchronous.)  Also, it requires windows to respond to the WM_PAINTCLIENT command, so that's been added to MediaPlayerPrivateFullScreenWindow class.

Just realized I left the 2s animation duration in this patch.  I'll lower it to 0.5s before I check in.

[WebKit Review Bot]

Attachment 92052 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/UIProcess/win/WebView.h:48:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 92052 [details]
Patch

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

> Source/WebCore/platform/graphics/win/FullScreenController.cpp:118
> +    m_private->m_backgroundWindow = adoptPtr(new MediaPlayerPrivateFullscreenWindow(m_private.get()));
> +    m_private->m_backgroundWindow->createWindow(0);
> +    ::AnimateWindow(m_private->m_backgroundWindow->hwnd(), kFullScreenAnimationDuration, AW_BLEND | AW_ACTIVATE);

Is the job of the background window just to draw black? If so, would it be better to have a simpler window class that just did that?

> Source/WebCore/platform/graphics/win/FullScreenController.cpp:134
> +    ::InvalidateRect(m_private->m_client->fullScreenClientWindow(), 0, true);
> +    ::UpdateWindow(m_private->m_client->fullScreenClientWindow());

You might be able to combine these into a single ::RedrawWindow call.

> Source/WebCore/platform/graphics/win/FullScreenController.cpp:135
> +    m_private->m_originalHost = originalHost;

Why do we need the originalHost local? Can't we just use the member variable from the start?

> Source/WebCore/platform/graphics/win/FullScreenController.cpp:154
> +    ::DestroyWindow(m_private->m_fullScreenWindow->hwnd());
> +    m_private->m_fullScreenWindow = 0;

I think m_fullScreenWindow will call ::DestroyWindow for you when it is destroyed.

Please use nullptr. Using 0 won't work anymore when we turn on strict PassOwnPtr.

> Source/WebCore/platform/graphics/win/FullScreenController.cpp:160
> +    ASSERT(m_private->m_backgroundWindow);
> +    success = ::AnimateWindow(m_private->m_backgroundWindow->hwnd(), kFullScreenAnimationDuration, AW_HIDE | AW_BLEND);
> +}

Why don't we need to clear out m_backgroundWindow?

> Source/WebCore/platform/graphics/win/FullScreenControllerClient.h:37
> +    virtual void setFullScreenClientParentWindow(HWND) = 0;

Maybe "fullScreenClientSetParentWindow" would be better? Then all the functions will have a common "fullScreenClient" prefix.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.cpp:187
> +            ::FillRect(hdc, &ps.rcPaint, (HBRUSH)::GetStockObject(BLACK_BRUSH));

Please use static_cast,

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.cpp:197
> +            HDC context = (HDC)wParam;
> +            ::GetClientRect(m_hwnd, &clientRect);
> +            ::FillRect(context, &clientRect, (HBRUSH)::GetStockObject(BLACK_BRUSH));
> +        }

Please use C++ casts.

Why don't we need to implement WM_PRINTCLIENT in the case where we have a layer host? Won't that make the ::AnimateWindow call for the full screen window not work?

> Source/WebKit/win/WebView.cpp:6813
> +void WebView::fullScreenClientWillEnterFullScreen()
> +{
> +    ASSERT(m_fullScreenElement);
> +    m_fullScreenElement->document()->webkitWillEnterFullScreenForElement(m_fullScreenElement.get());
> +}
> +
> +void WebView::fullScreenClientDidEnterFullScreen()
> +{
> +    ASSERT(m_fullScreenElement);
> +    m_fullScreenElement->document()->webkitDidEnterFullScreenForElement(m_fullScreenElement.get());
> +}
> +
> +void WebView::fullScreenClientWillExitFullScreen()
> +{
> +    ASSERT(m_fullScreenElement);
> +    m_fullScreenElement->document()->webkitWillExitFullScreenForElement(m_fullScreenElement.get());
> +}
> +
> +void WebView::fullScreenClientDidExitFullScreen()
> +{
> +    ASSERT(m_fullScreenElement);
> +    m_fullScreenElement->document()->webkitDidExitFullScreenForElement(m_fullScreenElement.get());
> +    m_fullScreenElement = nullptr;
> +}

Seems like this code could move into WebCore at some point, too. Can't the controller just keep track of this itself? I guess the WebKit2 implementation is different.

>> Source/WebKit2/UIProcess/win/WebView.h:48
>> +    class FullScreenController;
> 
> Code inside a namespace should not be indented.  [whitespace/indent] [4]

Silly style bot!

> Source/WebKit2/WebProcess/FullScreen/win/WebFullScreenManagerWin.cpp:69
> +    // FIXME: Disable setting RenderLayer::setAnimating() to make this unnecessary.s

Typo: unnecessary.s

[Adam Roben (:aroben)]

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 91984 [details] [details])
> >> Source/WebKit2/WebProcess/FullScreen/win/WebFullScreenManagerWin.cpp:72
> >> +    m_fullScreenRootLayer = layer;
> > 
> > I don't see any code that ever renders this layer. Am I missing something?
> 
> No, you're not.  The passed in layer /must/ have a super layer, or an assert happens when synchronizing the layers.  (The sync mechanism assumes all parentless layers are the root layer, and freaks out.)   However, on windows, since we don't animate the full screen element's layer, this function is called twice within the same runloop, once to set the layer, and the other to clear it.  The view won't even get a chance to redraw.
> 
>  I intend to fix this with the "Full Screen Flashing" bug, so that this function will never be called unnecessarily like this; see the comment about not setting RenderLayer::setAnimating().  (And I see an extra "S" at the end of that comment.)

Do does this mean this function should ideally never be called on Windows, but currently it *is* called? Can we just do nothing in that function?

[Jer Noble]

(In reply to comment #23)
> (From update of attachment 92052 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92052&action=review
> 
> > Source/WebCore/platform/graphics/win/FullScreenController.cpp:118
> > +    m_private->m_backgroundWindow = adoptPtr(new MediaPlayerPrivateFullscreenWindow(m_private.get()));
> > +    m_private->m_backgroundWindow->createWindow(0);
> > +    ::AnimateWindow(m_private->m_backgroundWindow->hwnd(), kFullScreenAnimationDuration, AW_BLEND | AW_ACTIVATE);
> 
> Is the job of the background window just to draw black? If so, would it be better to have a simpler window class that just did that?

I considered that as well, but I thought it would unnecessarily duplicate code to have two window classes that did (essentially) the same thing.

> > Source/WebCore/platform/graphics/win/FullScreenController.cpp:134
> > +    ::InvalidateRect(m_private->m_client->fullScreenClientWindow(), 0, true);
> > +    ::UpdateWindow(m_private->m_client->fullScreenClientWindow());
> 
> You might be able to combine these into a single ::RedrawWindow call.

Ooh, good idea!  I'll do it!

> > Source/WebCore/platform/graphics/win/FullScreenController.cpp:135
> > +    m_private->m_originalHost = originalHost;
> 
> Why do we need the originalHost local? Can't we just use the member variable from the start?
> 
> > Source/WebCore/platform/graphics/win/FullScreenController.cpp:154
> > +    ::DestroyWindow(m_private->m_fullScreenWindow->hwnd());
> > +    m_private->m_fullScreenWindow = 0;
> 
> I think m_fullScreenWindow will call ::DestroyWindow for you when it is destroyed.

You are, of course, right, as always.

> Please use nullptr. Using 0 won't work anymore when we turn on strict PassOwnPtr.

I was going to ask what the emphasis on nullptr was all about.  I'll try to reorganize my internal style map. :)

> > Source/WebCore/platform/graphics/win/FullScreenController.cpp:160
> > +    ASSERT(m_private->m_backgroundWindow);
> > +    success = ::AnimateWindow(m_private->m_backgroundWindow->hwnd(), kFullScreenAnimationDuration, AW_HIDE | AW_BLEND);
> > +}
> 
> Why don't we need to clear out m_backgroundWindow?

We probably should, but it's a tiny class, and ends up hidden, so it's not strictly necessary.  Ideally, we should clear it out at the end of enterFullScreen() /and/ exitFullScreen() and create it anew at the beginning of each.

> > Source/WebCore/platform/graphics/win/FullScreenControllerClient.h:37
> > +    virtual void setFullScreenClientParentWindow(HWND) = 0;
> 
> Maybe "fullScreenClientSetParentWindow" would be better? Then all the functions will have a common "fullScreenClient" prefix.

Sure thing!

> > Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.cpp:187
> > +            ::FillRect(hdc, &ps.rcPaint, (HBRUSH)::GetStockObject(BLACK_BRUSH));
> 
> Please use static_cast,
> 
> > Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.cpp:197
> > +            HDC context = (HDC)wParam;
> > +            ::GetClientRect(m_hwnd, &clientRect);
> > +            ::FillRect(context, &clientRect, (HBRUSH)::GetStockObject(BLACK_BRUSH));
> > +        }
> 
> Please use C++ casts.

Roger.

> Why don't we need to implement WM_PRINTCLIENT in the case where we have a layer host? Won't that make the ::AnimateWindow call for the full screen window not work?

Because the layerHost->paint() function doesn't use the HDC provided by the WM_PRINTCLIENT call, and I didn't want to have to either make a new layerHost function taking an HDC, or pipe it through the existing one.

The full screen window (in this case) doesn't use the layerHost; the child windows do, and they already respond to WM_PRINTCLIENT.  The reason I noticed this behavior was because the backgroundWindow refused to animate without it, but the fullScreen window (whose child is the webView window) animated perfectly.

> > Source/WebKit/win/WebView.cpp:6813
> > +void WebView::fullScreenClientWillEnterFullScreen()
> > +{
> > +    ASSERT(m_fullScreenElement);
> > +    m_fullScreenElement->document()->webkitWillEnterFullScreenForElement(m_fullScreenElement.get());
> > +}
> > +
> > +void WebView::fullScreenClientDidEnterFullScreen()
> > +{
> > +    ASSERT(m_fullScreenElement);
> > +    m_fullScreenElement->document()->webkitDidEnterFullScreenForElement(m_fullScreenElement.get());
> > +}
> > +
> > +void WebView::fullScreenClientWillExitFullScreen()
> > +{
> > +    ASSERT(m_fullScreenElement);
> > +    m_fullScreenElement->document()->webkitWillExitFullScreenForElement(m_fullScreenElement.get());
> > +}
> > +
> > +void WebView::fullScreenClientDidExitFullScreen()
> > +{
> > +    ASSERT(m_fullScreenElement);
> > +    m_fullScreenElement->document()->webkitDidExitFullScreenForElement(m_fullScreenElement.get());
> > +    m_fullScreenElement = nullptr;
> > +}
> 
> Seems like this code could move into WebCore at some point, too. Can't the controller just keep track of this itself? I guess the WebKit2 implementation is different.

Yeah, the WebKit2 implementation lives in the UIProcess, and so doesn't have any notion of Elements or Documents.

> >> Source/WebKit2/UIProcess/win/WebView.h:48
> >> +    class FullScreenController;
> > 
> > Code inside a namespace should not be indented.  [whitespace/indent] [4]
> 
> Silly style bot!

I know, right!? :)

> > Source/WebKit2/WebProcess/FullScreen/win/WebFullScreenManagerWin.cpp:69
> > +    // FIXME: Disable setting RenderLayer::setAnimating() to make this unnecessary.s
> 
> Typo: unnecessary.s

Un-essed.

[Jer Noble]

(In reply to comment #25)
> (In reply to comment #23)
> > > Source/WebCore/platform/graphics/win/FullScreenController.cpp:135
> > > +    m_private->m_originalHost = originalHost;
> > 
> > Why do we need the originalHost local? Can't we just use the member variable from the start?

Originally, I had some early returns for failure cases.  Now those are gone, and so the local variable can go as well.

[Jer Noble]

(In reply to comment #24)
> Do does this mean this function should ideally never be called on Windows, but currently it *is* called? Can we just do nothing in that function?

The only thing we /have/ to do in that function is to give the passed in layer a parent.

[Jer Noble]

The last remaining issue is that, in WebKit2 mode, the webView repaint does not happen synchronously.  so you still see the non-fullscreen mode when entering fullscreen for an instant, and vice versa on exiting full screen.

I'm going to check it in anyway, and fix the WK2 in another bug.

[Jer Noble]

Committed r85699: <http://trac.webkit.org/changeset/85699>