Bug 31318 – Windows: Implement full screen mode for <video>

• Component: Media
• Status: RESOLVED FIXED

[Chris Marrin]

Implement full screen mode for <video>

rdar://problem/5966569

[Chris Marrin]

Created attachment 42898 [details]
Unfinished patch

This patch contains most of the functionality for full screen. There is a full-screen button on the in-page video controller which takes you to full-screen mode. This brings up a full screen controller with placeholder icons. Right now the center icon plays/pauses and the rightmost one takes you out of full screen. You can also hit space bar for play/pause and ESC to go out of full-screen.

Issues:

1) Replace icons

2) Implement timeline and volume sliders

3) Hook up max and min volume buttons

4) Handle video aspect ratio properly

5) Make HUD appear and disappear

6) Make HUD draggable.

This last issue is blocked because I don't know how to erase pixels on the special video overlay plane. Investigation is in progress.

[Chris Marrin]

Created attachment 44717 [details]
Icons for full-screen video controller

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44717 [details] without any errors.

[Jon Honeycutt]

Comment on attachment 44717 [details]
Icons for full-screen video controller

r=me

[Chris Marrin]

Landed icons in http://trac.webkit.org/changeset/52030

[Chris Marrin]

Created attachment 45401 [details]
Final patch

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 3072 characters of output:
, and then alphabetically sorted.  [build/include_order] [4]
WebKit/win/WebVideoFSHUDController.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/win/WebVideoFSHUDController.cpp:33:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/win/WebVideoFSHUDController.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/win/WebVideoFSHUDController.cpp:79:  Missing space after ,  [whitespace/comma] [3]
WebKit/win/WebVideoFSHUDController.cpp:81:  Missing space after ,  [whitespace/comma] [3]
WebKit/win/WebVideoFSHUDController.cpp:99:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebKit/win/WebVideoFSHUDController.cpp:97:  Missing space before ( in switch(  [whitespace/parens] [5]
WebKit/win/WebVideoFSHUDController.cpp:225:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/win/WebVideoFSHUDController.cpp:336:  Should have a space between // and comment  [whitespace/comments] [4]
WebKit/win/WebVideoFSHUDController.cpp:342:  Should have a space between // and comment  [whitespace/comments] [4]
WebKit/win/WebVideoFSHUDController.cpp:380:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebKit/win/WebVideoFSHUDController.cpp:379:  Missing space before ( in switch(  [whitespace/parens] [5]
WebCore/platform/graphics/win/QTMovieWin.h:117:  _handleMessages is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit/win/WebVideoFSController.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/win/WebVideoFSController.h:38:  This { should be at the end of the previous line  [whitespace/braces] [4]
WebCore/platform/graphics/win/QTMovieWin.cpp:1070:  QTMovieWin::_handleMessages is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/graphics/win/QTMovieWin.cpp:1080:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/win/QTMovieWin.cpp:1080:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/win/QTMovieWin.cpp:1081:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/win/QTMovieWin.cpp:1082:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/win/QTMovieWin.cpp:1082:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/win/QTMovieWin.cpp:1085:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/win/QTMovieWin.cpp:1085:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/platform/graphics/win/QTMovieWin.cpp:1086:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/win/QTMovieWin.cpp:1123:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/loader/FrameLoader.cpp:1477:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 38

[Chris Marrin]

Two issues remain with full-screen video.

1) Add select highlight to full-screen video HUD on Windows
(https://bugs.webkit.org/show_bug.cgi?id=32880)

2) Make full-screen video HUD fade in and out on Windows
(https://bugs.webkit.org/show_bug.cgi?id=32881)

[Darin Adler]

Comment on attachment 45401 [details]
Final patch

Exciting to see this!

The use of "the" and "my" prefixes in the code is not typical of WebKit code. Should keep that to a minimum.

> -    ASSERT(m_frame->document()->parsing());
> +    //ASSERT(m_frame->document()->parsing());

If this needs to be removed, then we want to remove it, not comment it out. The change log does not mention this, so maybe you ought to just put it back in?

> +    virtual bool supportsFullscreen() const { return true; }
> +
> +    virtual PlatformMedia platformMedia() const
> +    {
> +        PlatformMedia p;
> +        MediaPlayerPrivate* m = const_cast<MediaPlayerPrivate*>(this);
> +        p.qtMovie = reinterpret_cast<QTMovie*>(m->m_qtMovie.get());
> +        return p;
> +    }

It's normally not good style to put virtual function definitions inside class definitions, because that asks to inline them and they can rarely be inlined. So I suggest putting the function bodies in the cpp file instead of the header.

I also think these overrides should be private, since it's normally a programming mistake to call them with a MediaPlayerPrivate* rather than a MediaPlayerPrivateInterface*.

> +    WindowPtr m_fsWindow;
> +    GWorldPtr m_fsOrigGWorld;
> +    Rect m_fsRect;
> +    QTMovieWinFSClient* m_fsClient;
> +    char* m_fsRestoreState;

I find the use of "fs" and "FS" confusing. Would it really make things too long-winded to say "fullScreen" instead?

> +    QTMovieWin* movie = reinterpret_cast<QTMovieWin*>(GetWindowLongPtr(theWnd, GWL_USERDATA));

Can we use static_cast here?

> +	OSErr myErr = BeginFullScreen(&m_private->m_fsRestoreState, NULL, 0, 0, &m_private->m_fsWindow, NULL, fullScreenAllowEvents); 
> +	QTMLSetWindowWndProc(m_private->m_fsWindow, _handleMessages);
> +	CreatePortAssociation(GetPortNativeWindow(m_private->m_fsWindow), NULL, 0L);

Tabs here, so we won't be able to check in as-is.

The uses of NULL and 0L here should be replaced by uses of 0.

> +    HWND wnd = (HWND) GetPortNativeWindow(m_private->m_fsWindow);
> +    SetWindowLongPtr(wnd, GWL_USERDATA, (LONG) this);

> +    HWND wnd = (HWND) GetPortNativeWindow(m_private->m_fsWindow);

> +    DestroyPortAssociation((CGrafPtr) m_private->m_fsWindow);

Could we use static_cast or reinterpret_cast for these casts?

> +    SetMovieGWorld(m_private->m_movie, m_private->m_fsOrigGWorld, NULL);
> +    OSErr myErr = EndFullScreen(m_private->m_fsRestoreState, 0L);

More NULL and 0L.

> +class QTMovieWinFSClient {
> +public:
> +    virtual LRESULT fsClientWndProc(HWND theWnd, UINT theMessage, UINT wParam, LONG lParam) = 0;

Normally we leave out the argument name theWnd in cases like this. I'd also name the message just "message" rather than "theMessage".

> +    HWND enterFullscreen(QTMovieWinFSClient* client);

Normally we leave out the argument name in cases like this.

> +WebVideoFSController::WebVideoFSController()
> + : m_window(0)
> + , m_hudController(0)

Non-standard indentation here.

> +void WebVideoFSController::setMediaElement(WebCore::HTMLMediaElement* mediaElement)
> +{
> +    m_mediaElement = mediaElement;
> +    if (!m_mediaElement) {
> +        // FIXME: Do some cleanup here
> +        return;
> +    }
> +}

Is this really the way we want to check it in? Does we need that empty if statement?

I don't think the WebCore prefix is needed here. Instead we should do using namespace WebCore at the top of the file.

> +LRESULT WebVideoFSController::fsClientWndProc(HWND theWnd, UINT theMessage, UINT wParam, LONG lParam)

I'd leave off the "the" here.

> +    switch(theMessage) {

Missing space here.

> +bool WebVideoFSController::hudClientCanPlay()
> +{
> +    return m_mediaElement ? m_mediaElement->canPlay() : false;
> +}

I often use && for such boolean expressions rather than ?: -- m_mediaElement && m_mediaElement->canPlay().

> +    return m_mediaElement ? (m_mediaElement->currentTime()) : 0;

Extra parentheses here are not needed.

> +    return m_mediaElement ? (m_mediaElement->duration()) : 0;

Not here either.

> +#include "QTMovieWin.h"
> +#include "WebVideoFSHUDController.h"
> +
> +#include <wtf/RefPtr.h>
> +#include <WebCore/HTMLMediaElement.h>

Normally we sort includes in a single paragraph.

> +class WebVideoFSController : public QTMovieWinFSClient, public WebVideoFSHUDClient
> +{

Brace goes on the same line as the class name.

Should inherit from Noncopyable, unless one of the base classes adds it.

I suggest both base classes be private bases rather than public.

> +    void setMediaElement(WebCore::HTMLMediaElement* mediaElement);

We normally leave out the argument name in cases like this.

> +    // QTMovieWinFSClient
> +    virtual LRESULT fsClientWndProc(HWND theWnd, UINT theMessage, UINT wParam, LONG lParam);
> +
> +    // WebVideoFSHUDClient
> +    virtual void hudClientExitFullscreen();
> +    virtual bool hudClientCanPlay();
> +    virtual void hudClientPlay();
> +    virtual void hudClientPause();
> +    virtual float hudClientVolume() const;
> +    virtual void hudClientSetVolume(float);
> +    virtual float hudClientCurrentTime() const;
> +    virtual void hudClientSetCurrentTime(float);
> +    virtual float hudClientDuration() const;
> +    virtual void hudClientBeginScrubbing();
> +    virtual void hudClientEndScrubbing();

Can these all be private?

> +#include <windowsx.h>
> +
> +#include "WebKitDLL.h"
> +#include "WebVideoFSHUDController.h"
> +
> +#include <ApplicationServices/ApplicationServices.h>
> +#include <WebKitSystemInterface/WebKitSystemInterface.h>
> +
> +#include <WebCore/BitmapInfo.h>
> +#include <WebCore/Font.h>
> +#include <WebCore/FontSelector.h>
> +#include <WebCore/GraphicsContext.h>
> +#include <WebCore/TextRun.h>
> +#include <wtf/StdLibExtras.h>

Normally we sort includes in a single paragraph.

> +static const float kTimerInterval = 0.033;

The use of "k" in all the constants is not normal WebKit style.

> +// Button/slider/text positions
> +static const IntPoint kPlayPauseButtonPoint((kWindowWidth - kButtonSize) / 2, kMarginTop);
> +static const IntPoint kVolumeUpButtonPoint(kMargin + kButtonMiniSize + kVolumeSliderWidth + kButtonMiniSize / 2, kMarginTop + (kButtonSize - kButtonMiniSize) / 2);
> +static const IntPoint kVolumeDownButtonPoint(kMargin, kMarginTop + (kButtonSize - kButtonMiniSize) / 2);
> +static const IntPoint kVolumeSliderPoint(kVolumeDownButtonPoint.x() + kButtonMiniSize, kVolumeDownButtonPoint.y() + kButtonMiniSize / 2 - kSliderHeight / 2);
> +static const IntPoint kTimeSliderPoint(kWindowWidth / 2 - kTimeSliderWidth / 2, kWindowHeight - kMargin - kSliderHeight);
> +static const IntPoint kExitFullscreenButtonPoint(kWindowWidth - 2 * kMargin - kButtonMiniSize, kMarginTop + (kButtonSize - kButtonMiniSize) / 2);
> +static const IntPoint kLeftTextPoint(kWindowWidth / 2 - kTimeSliderWidth / 2 - kMargin, kWindowHeight - kMargin - kSliderHeight / 2);
> +static const IntPoint kRightTextPoint(kWindowWidth / 2 + kTimeSliderWidth / 2 + kMargin, kWindowHeight - kMargin - kSliderHeight / 2);

All these constants of classes with constructors are going to create initialization time constructors, which are not good for DLL loading speed and thus application startup speed. Can we find a way to avoid that?

I didn't finish reviewing. If I have more time I'll pick up where I left off.

[Simon Fraser (smfr)]

Comment on attachment 45401 [details]
Final patch



> Index: WebCore/loader/FrameLoader.cpp
> ===================================================================

>      ASSERT(m_workingURL.isEmpty());
>      ASSERT(m_frame->document());
> -    ASSERT(m_frame->document()->parsing());
> +    //ASSERT(m_frame->document()->parsing());
>      write(bytes, length);

Not part of the patch, I assume?

> Index: WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeWin.h
> ===================================================================

> +    virtual PlatformMedia platformMedia() const
> +    {
> +        PlatformMedia p;
> +        MediaPlayerPrivate* m = const_cast<MediaPlayerPrivate*>(this);
> +        p.qtMovie = reinterpret_cast<QTMovie*>(m->m_qtMovie.get());

It would be OK to do the const_cast inline.

p.qtMovie = reinterpret_cast<QTMovie*>(const_cast<MediaPlayerPrivate*>(this)->m_qtMovie.get());

Why is the const_cast needed anyway? Maybe just add a private const accessor for the qtMovie?

> Index: WebCore/platform/graphics/win/QTMovieWin.cpp
> ===================================================================

> +    WindowPtr m_fsWindow;
> +    GWorldPtr m_fsOrigGWorld;
> +    Rect m_fsRect;
> +    QTMovieWinFSClient* m_fsClient;
> +    char* m_fsRestoreState;

Please expand 'fs' to fullscreen in these names.

> @@ -159,6 +164,10 @@ QTMovieWinPrivate::QTMovieWinPrivate()
>  #if !ASSERT_DISABLED
>      , m_scaleCached(false)
>  #endif
> +    , m_fsWindow(0)
> +    , m_fsOrigGWorld(0)
> +    , m_fsClient(0)
> +    , m_fsRestoreState(0)

Initialize m_fsRect?

> @@ -1040,7 +1049,7 @@ bool QTMovieWin::initializeQuickTime() 
>      if (!initialized) {
>          initialized = true;
>          // Initialize and check QuickTime version
> -        OSErr result = InitializeQTML(0);
> +        OSErr result = InitializeQTML(kInitializeQTMLEnableDoubleBufferedSurface);

Does this have any impact on non-fullscreen video playback performance?

> +HWND QTMovieWin::enterFullscreen(QTMovieWinFSClient* client)
> +{
> +    m_private->m_fsClient = client;
> +    
> +	OSErr myErr = BeginFullScreen(&m_private->m_fsRestoreState, NULL, 0, 0, &m_private->m_fsWindow, NULL, fullScreenAllowEvents); 

myErr is unused. Should check for errors.

> +	QTMLSetWindowWndProc(m_private->m_fsWindow, _handleMessages);
> +	CreatePortAssociation(GetPortNativeWindow(m_private->m_fsWindow), NULL, 0L);
> +
> +    GetMovieBox(m_private->m_movie, &m_private->m_fsRect);
> +	GetMovieGWorld(m_private->m_movie, &m_private->m_fsOrigGWorld, NULL);
> +	SetMovieGWorld(m_private->m_movie, (CGrafPtr)m_private->m_fsWindow, GetGWorldDevice((CGrafPtr)m_private->m_fsWindow));

Tabs here. Use 0 rather than NULL.

> +
> +    // Set the size of the box to preserve aspect ratio
> +    Rect rect = m_private->m_fsWindow->portRect;
> +
> +    float movieRatio = static_cast<float>(m_private->m_width) / m_private->m_height;
> +    int windowWidth =  rect.right - rect.left;
> +    int windowHeight = rect.bottom - rect.top;
> +    float windowRatio = static_cast<float>(windowWidth) / windowHeight;
> +    int actualWidth = (windowRatio > movieRatio) ? (windowHeight * movieRatio) : windowWidth;
> +    int actualHeight = (windowRatio < movieRatio) ? (windowWidth / movieRatio) : windowHeight;
> +    int offsetX = (windowWidth - actualWidth) / 2;
> +    int offsetY = (windowHeight - actualHeight) / 2;

Ah, the old "resize maintaining aspect ratio" logic. I think we have that 3-4 times in WebKit. Maybe make a method on IntRect or FloatRect and use it?

> +    rect.left = offsetX;
> +    rect.right = offsetX + actualWidth;
> +    rect.top = offsetY;
> +    rect.bottom = offsetY + actualHeight;
> +
> +    SetMovieBox(m_private->m_movie, &rect);
> +    ShowHideTaskBar(true);
> +
> +    // set the this pointer on the HWND

Sentence case for comments pls.

> +    HWND wnd = (HWND) GetPortNativeWindow(m_private->m_fsWindow);

No space after the )

> +    SetWindowLongPtr(wnd, GWL_USERDATA, (LONG) this);

Ditto.

> +void QTMovieWin::exitFullscreen()
> +{
> +    if (!m_private->m_fsWindow)
> +        return;
> +
> +    HWND wnd = (HWND) GetPortNativeWindow(m_private->m_fsWindow);
> +    SetWindowLongPtr(wnd, GWL_USERDATA, 0);
> +    DestroyPortAssociation((CGrafPtr) m_private->m_fsWindow);
> +    SetMovieGWorld(m_private->m_movie, m_private->m_fsOrigGWorld, NULL);
> +    OSErr myErr = EndFullScreen(m_private->m_fsRestoreState, 0L);

Check for errors?

> +    SetMovieBox(m_private->m_movie, &m_private->m_fsRect);
> +    m_private->m_fsWindow = 0;
> +}

Please add an assertion in QTMovieWin's dtor to ensure that we've left fullscreen when it gets deleted, otherwise I think we'll leak a bunch of state.

> Index: WebCore/platform/graphics/win/QTMovieWin.h
> ===================================================================

> +class QTMovieWinFSClient {
> +public:
> +    virtual LRESULT fsClientWndProc(HWND theWnd, UINT theMessage, UINT wParam, LONG lParam) = 0;
> +};

Please expand 'FS' and 'fs'. It's not obvious to the casual reader what 'fs' is an abbreviation of.

> +    HWND enterFullscreen(QTMovieWinFSClient* client);

Add a comment saying what the return value is, and what it might be used for.

>  private:
>      void load(CFURLRef, bool preservesPitch);
> +    static LRESULT _handleMessages(HWND theWnd, UINT theMessage, UINT wParam, LONG lParam);
> +    LRESULT handleMessages(HWND theWnd, UINT theMessage, UINT wParam, LONG lParam);

Not a big fan of the same names differing by an underscore.

> Index: WebKit/win/WebVideoFSController.cpp
> ===================================================================

I'd prefer FS to be expanded in the file name and class names.

> +WebVideoFSController::WebVideoFSController()
> + : m_window(0)
> + , m_hudController(0)

Evil tabs.

> +void WebVideoFSController::setMediaElement(WebCore::HTMLMediaElement* mediaElement)
> +{
> +    m_mediaElement = mediaElement;
> +    if (!m_mediaElement) {
> +        // FIXME: Do some cleanup here
> +        return;
> +    }
> +}

This is the kind of FIXME that will never get fixed unless you do it now.

> +float WebVideoFSController::hudClientVolume() const
> +{
> +    return m_mediaElement ? m_mediaElement->volume() : 0;
> +}
> +void WebVideoFSController::hudClientSetVolume(float volume)

Missing blank line.

> +void WebVideoFSController::exitFullscreen()
> +{
> +    if (movie())
> +        movie()->exitFullscreen();
> +}

Group next to enterFullscreen().

> Index: WebKit/win/WebVideoFSController.h
> ===================================================================

> +class WebVideoFSController : public QTMovieWinFSClient, public WebVideoFSHUDClient

I'd prefer FS to be expanded in the file name and class names.

> +public:
> +    WebVideoFSController();
> +    ~WebVideoFSController();

Make the dtor virtual?

> +    WebCore::HTMLMediaElement* mediaElement() { return m_mediaElement.get(); }

Method should be const.

> +
> +    void enterFullscreen();
> +    void exitFullscreen();
> +
> +    // QTMovieWinFSClient
> +    virtual LRESULT fsClientWndProc(HWND theWnd, UINT theMessage, UINT wParam, LONG lParam);
> +
> +    // WebVideoFSHUDClient
> +    virtual bool hudClientCanPlay();

Should be const.

> +    QTMovieWin* movie();

const.

> Index: WebKit/win/WebVideoFSHUDController.cpp
> ===================================================================

> +void HUDSlider::draw(WebCore::GraphicsContext& context)
> +{
> +    // Draw gutter
> +    IntSize radius(m_height / 2, m_height / 2);
> +    context.fillRoundedRect(IntRect(m_position, IntSize(m_width, m_height)), radius, radius, radius, radius, kSliderGutterColor, DeviceColorSpace);
> +
> +    // Draw button
> +    context.setStrokeColor(kSliderButtonColor, DeviceColorSpace);
> +    context.setFillColor(kSliderButtonColor, DeviceColorSpace);

You should save and restore the context around code that changes state.

> +WebVideoFSHUDController::WebVideoFSHUDController(WebVideoFSHUDClient* client, HWND window)

> +    void* pixels;
> +    BitmapInfo bitmapInfo = BitmapInfo::createBottomUp(IntSize(kWindowWidth, kWindowHeight));
> +    m_bitmap.set(::CreateDIBSection(NULL, &bitmapInfo, DIB_RGB_COLORS, &pixels, NULL, 0));

Does 'pixels' need to be cleaned up?

> +bool WebVideoFSHUDController::registerWindowClass()
> +{
> +    static bool haveRegisteredWindowClass = false;
> +    if (haveRegisteredWindowClass)
> +        return true;
> +
> +    haveRegisteredWindowClass = true;
> +
> +    WNDCLASSEX wcex;
> +
> +    wcex.cbSize = sizeof(WNDCLASSEX);
> +
> +    wcex.style          = CS_HREDRAW | CS_VREDRAW;
> +    wcex.lpfnWndProc    = wndProc;
> +    wcex.cbClsExtra     = 0;
> +    wcex.cbWndExtra     = 4;
> +    wcex.hInstance      = gInstance;
> +    wcex.hIcon          = 0;
> +    wcex.hCursor        = ::LoadCursor(0, IDC_ARROW);
> +    wcex.hbrBackground  = 0;
> +    wcex.lpszMenuName   = 0;
> +    wcex.lpszClassName  = kWebVideoFSHUDWindowClassName;
> +    wcex.hIconSm        = 0;

Lining up like this isn't normal webkit style.

> +void WebVideoFSHUDController::createWindow()
> +{
> +    registerWindowClass();

Check the return value?

> +static String timeToString(float time)
> +{
> +    if (!isfinite(time))
> +        time = 0;
> +    int seconds = fabsf(time); 
> +    int hours = seconds / (60 * 60);
> +    int minutes = (seconds / 60) % 60;
> +    seconds %= 60;
> +
> +    if (hours) {
> +        if (hours > 9)
> +            return String::format("%s%02d:%02d:%02d", (time < 0 ? "-" : ""), hours, minutes, seconds);
> +        return String::format("%s%01d:%02d:%02d", (time < 0 ? "-" : ""), hours, minutes, seconds);
> +    }
> +
> +    return String::format("%s%02d:%02d", (time < 0 ? "-" : ""), minutes, seconds);
> +}

Would be nice to share this code with Mac.

> +void WebVideoFSHUDController::draw()
> +{
> +    GraphicsContext gc(bitmapDC, true);

Context is often called 'ctx'.

> +    SystemParametersInfo(SPI_GETNONCLIENTMETRICS, metrics.cbSize, &metrics, 0);
> +    //desc.setWeight(FontWeightBold);

Don't check in commented code.

> +    FontFamily family;
> +    family.setFamily(metrics.lfSmCaptionFont.lfFaceName);
> +    desc.setFamily(family);
> +
> +    desc.setComputedSize(kTextSize);
> +    //desc.setGenericFamily(FontDescription::MonospaceFamily);

Ditto.

> +LRESULT WebVideoFSHUDController::wndProc(HWND wnd, UINT message, UINT wParam, LONG lParam)

> +    switch(message) {

Missing space after switch.

> +void WebVideoFSHUDController::onChar(int c)
> +{
> +    if (c == 0x1b)
> +        m_client->hudClientExitFullscreen();
> +    else if (c == 0x20)
> +        togglePlay();
> +}

Are there constants for those chars you can use? At least comment them.

> +void WebVideoFSHUDController::timerFired(WebCore::Timer<WebVideoFSHUDController>*)
> +{
> +    // When the movie is playing, the event queue seems to be getting starved,
> +    // so we fire a timer every 33ms and simulate a mouse move. This only
> +    // happens when the left button is down

Is there a Radar to track the cause of this hack?

> +void WebVideoFSHUDController::onMouseMove(int x, int y)
> +{
> +    // Messages are actually coming into the full-screen window, so we have to subtract the window position
> +    x -= m_posX;
> +    y -= m_posY;

You could make this self-documenting with a method call, and cleaner using IntPoint everywhere:
    IntPoint  localCoords = convertToLocalCoords(point);

> +        draw();
> +    } else if (m_movingWindow) {
> +        m_posX += x - m_moveOffsetX;
> +        m_posY += y - m_moveOffsetY;
> +        draw();

Should you really be drawing synchronously in all these places, or would it suffice just to invalidate and
let Windows call you back to do drawing?

> Index: WebKit/win/WebVideoFSHUDController.h
> ===================================================================
> +class HUDWidget {
> +public:
> +    HUDWidget(const WebCore::IntPoint& position, int width, int height)

Use IntSize for width and height? Or, IntRect for frame?

> +    bool isPicked(int x, int y) const { return x >= m_position.x() && x < m_position.x() + m_width && y >= m_position.y() && y < m_position.y() + m_height; }

I'd prefer this be called 'hitTest'. "Picking" is more of a 3D term.

> +protected:
> +    WebCore::IntPoint m_position;
> +    int m_width, m_height;

Crys out for IntRect.

> +class WebVideoFSHUDClient {
> +public:
> +    virtual void hudClientExitFullscreen() = 0;
> +    virtual bool hudClientCanPlay() = 0;

Should be const.

> +class WebVideoFSHUDController {

> +    WebVideoFSHUDClient* m_client;
> +    HWND m_window, m_parentWindow;
> +    OwnPtr<HBITMAP> m_bitmap;
> +    int m_fsWidth, m_fsHeight;
> +    int m_posX, m_posY;

IntRect?

> +    int m_moveOffsetX, m_moveOffsetY;

IntPoint or IntSize?

> Index: WebKit/win/WebView.cpp
> ===================================================================

> +void WebView::enterFullscreenForNode(Node* node)
> +{
> +    ASSERT(node->hasTagName(WebCore::HTMLNames::videoTag));

I think this check should not just be in the ASSERT; check the type, and return if it's not video.

> +void WebView::exitFullscreen()
> +{
> +    free(malloc(1));
> +    m_fsController = 0;
> +    free(malloc(1));

Looks like you left some testing code in.

Mostly good, but I'd like to see one more rev.

[Chris Marrin]

I've gone through and changed all instances of Fullscreen to FS and fs in my code because it WAS making everything get very longwinded, especially WebVideoFullScreenHUDController! It's not so bad for other names (like fsScreen or something) but for consistency I changed them all.

[Chris Marrin]

> +    QTMovieWin* movie = reinterpret_cast<QTMovieWin*>(GetWindowLongPtr(theWnd, GWL_USERDATA));

> Can we use static_cast here?

No, even though the call is GetWindowLongPtr() it returns a LONG (thanks to Microsoft's wonderful naming scheme), so you have to use reinterpret_cast to get it to be a pointer.

[Chris Marrin]

> > @@ -159,6 +164,10 @@ QTMovieWinPrivate::QTMovieWinPrivate()
> >  #if !ASSERT_DISABLED
> >      , m_scaleCached(false)
> >  #endif
> > +    , m_fsWindow(0)
> > +    , m_fsOrigGWorld(0)
> > +    , m_fsClient(0)
> > +    , m_fsRestoreState(0)
> 
> Initialize m_fsRect?

I'm not sure how to initialize a struct.

> 
> > @@ -1040,7 +1049,7 @@ bool QTMovieWin::initializeQuickTime() 
> >      if (!initialized) {
> >          initialized = true;
> >          // Initialize and check QuickTime version
> > -        OSErr result = InitializeQTML(0);
> > +        OSErr result = InitializeQTML(kInitializeQTMLEnableDoubleBufferedSurface);
> 
> Does this have any impact on non-fullscreen video playback performance?

It doesn't seem to. I need to ask Eric when he gets back. I've opened a bug (https://bugs.webkit.org/show_bug.cgi?id=32889)

> > +    // Set the size of the box to preserve aspect ratio
> > +    Rect rect = m_private->m_fsWindow->portRect;
> > +
> > +    float movieRatio = static_cast<float>(m_private->m_width) / m_private->m_height;
> > +    int windowWidth =  rect.right - rect.left;
> > +    int windowHeight = rect.bottom - rect.top;
> > +    float windowRatio = static_cast<float>(windowWidth) / windowHeight;
> > +    int actualWidth = (windowRatio > movieRatio) ? (windowHeight * movieRatio) : windowWidth;
> > +    int actualHeight = (windowRatio < movieRatio) ? (windowWidth / movieRatio) : windowHeight;
> > +    int offsetX = (windowWidth - actualWidth) / 2;
> > +    int offsetY = (windowHeight - actualHeight) / 2;
> 
> Ah, the old "resize maintaining aspect ratio" logic. I think we have that 3-4
> times in WebKit. Maybe make a method on IntRect or FloatRect and use it?

If you can identify other places where it's used I'd be happy to fix it in a separate bug :-)

[Chris Marrin]

Created attachment 45851 [details]
Replacement patch

This addresses all of Simon and Darin's comments. In particular, I've collapsed WebVideoFSController and WebVideoFSHUDController into a single class FullscreenVideoController. The name is short enough to make MSDev happy and avoids abbreviations. And these two classes had a convoluted way of communicating with each other, so this also makes things a bit simpler.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/win/FullscreenVideoController.h:39:  Inner-style forward declarations are invalid.  Remove this line.  [build/forward_decl] [5]
WebKit/win/FullscreenVideoController.h:139:  More than one command on the same line  [whitespace/newline] [4]
WebKit/win/WebView.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/win/WebView.cpp:92:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/win/WebView.cpp:5657:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 5

[Simon Fraser (smfr)]

(In reply to comment #13)
> > > @@ -159,6 +164,10 @@ QTMovieWinPrivate::QTMovieWinPrivate()
> > >  #if !ASSERT_DISABLED
> > >      , m_scaleCached(false)
> > >  #endif
> > > +    , m_fsWindow(0)
> > > +    , m_fsOrigGWorld(0)
> > > +    , m_fsClient(0)
> > > +    , m_fsRestoreState(0)
> > 
> > Initialize m_fsRect?
> 
> I'm not sure how to initialize a struct.

Is there a const empty rect value you can use? Or:
m_fsRect(0, 0, 0, 0) or  m_fsRect( { 0, 0, 0, 0 }) perhaps.
> 
> > 
> > > @@ -1040,7 +1049,7 @@ bool QTMovieWin::initializeQuickTime() 
> > >      if (!initialized) {
> > >          initialized = true;
> > >          // Initialize and check QuickTime version
> > > -        OSErr result = InitializeQTML(0);
> > > +        OSErr result = InitializeQTML(kInitializeQTMLEnableDoubleBufferedSurface);
> > 
> > Does this have any impact on non-fullscreen video playback performance?
> 
> It doesn't seem to.

Did you look at Task Monitor to determine this?

[Chris Marrin]

Created attachment 45933 [details]
Replacement patch

This resolves all the issues from the style bot and expands all the remaining 'fs' names. The only ones left are in the icon filenames. If we rename those, it should be in a separate patch to avoid confusion.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/win/FullscreenVideoController.h:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1

[Adam Roben (:aroben)]

Comment on attachment 45933 [details]
Replacement patch

> @@ -159,11 +161,17 @@ QTMovieWinPrivate::QTMovieWinPrivate()
>  #if !ASSERT_DISABLED
>      , m_scaleCached(false)
>  #endif
> +    , m_fullscreenWindow(0)
> +    , m_fullscreenOrigGWorld(0)
> +    , m_fullscreenClient(0)
> +    , m_fullscreenRestoreState(0)
>  {
>  }

I think it would be good to initialize m_fullscreenRect, even if you can't do it in an initializer.

> +LRESULT QTMovieWin::fullscreenWndProc(HWND wnd, UINT message, UINT wParam, LONG lParam)

Is there a reason not to use the WPARAM and LPARAM types here?

> +{
> +    QTMovieWin* movie = reinterpret_cast<QTMovieWin*>(GetWindowLongPtr(wnd, GWL_USERDATA));
> +    return movie->m_private->m_fullscreenClient->fullscreenClientWndProc(wnd, message, wParam, lParam);
> +}

It seems possible that this function could be called before you have set up the QTMovieWin pointer. Perhaps you should null-check movie before dereferencing it?

> +HWND QTMovieWin::enterFullscreen(QTMovieWinFullscreenClient* client)
> +{
> +    m_private->m_fullscreenClient = client;
> +    
> +    BeginFullScreen(&m_private->m_fullscreenRestoreState, 0, 0, 0, &m_private->m_fullscreenWindow, 0, fullScreenAllowEvents); 

Where is fullScreenAllowEvents defined? I can't find it.

> +    QTMLSetWindowWndProc(m_private->m_fullscreenWindow, fullscreenWndProc);
> +    CreatePortAssociation(GetPortNativeWindow(m_private->m_fullscreenWindow), 0, 0L);

Is the L suffix really needed here?

> +    GetMovieBox(m_private->m_movie, &m_private->m_fullscreenRect);
> +    GetMovieGWorld(m_private->m_movie, &m_private->m_fullscreenOrigGWorld, 0);
> +    SetMovieGWorld(m_private->m_movie, (CGrafPtr)m_private->m_fullscreenWindow, GetGWorldDevice((CGrafPtr)m_private->m_fullscreenWindow));

C++-style casts would be better here.

> +    // Set the 'this' pointer on the HWND
> +    HWND wnd = static_cast<HWND>(GetPortNativeWindow(m_private->m_fullscreenWindow));
> +    SetWindowLongPtr(wnd, GWL_USERDATA, (LONG)this);

A C++-style cast would be better.

It isn't safe to set GWL_USERDATA on a window of some unknown window class. QuickTime might be using GWL_USERDATA for its own purposes, or the window class might not reserve any space GWL_USERDATA at all!

A safe way to do this would be to use SetProp to set the pointer, GetProp to retrieve it, and RemoveProp to remove it when WM_DESTROY is received or when you're done with the window, whichever comes first.

> +void QTMovieWin::exitFullscreen()
> +{
> +    if (!m_private->m_fullscreenWindow)
> +        return;
> +
> +    HWND wnd = (HWND) GetPortNativeWindow(m_private->m_fullscreenWindow);

A C++-style cast would be better.

> +    SetWindowLongPtr(wnd, GWL_USERDATA, 0);
> +    DestroyPortAssociation((CGrafPtr) m_private->m_fullscreenWindow);

C++ cast, please.

> +class QTMovieWinFullscreenClient {
> +public:
> +    virtual LRESULT fullscreenClientWndProc(HWND, UINT message, UINT wParam, LONG lParam) = 0;

Same question here about using WPARAM and LPARAM.

> +++ WebKit/win/ChangeLog	(working copy)
> @@ -1,3 +1,44 @@
> [phone]  Chris Marrin  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Implement full-screen video for Windows
> +        https://bugs.webkit.org/show_bug.cgi?id=31318
> +        
> +        This adds a full-screen controller, FullscreenVideoController, 
> +        which manages going in and out of full-screen. The acual 

Typo: acual

> +        I've also updated the icons to make them position correctly
> +        in the HUD.

Seems strange that this had to be done by modifying the icons, as opposed to the code.

> + * Copyright (C) 2009 Apple Inc. All rights reserved.

It's 2010, you know. :-)

> +HUDButton::HUDButton(HUDButtonType type, const WebCore::IntPoint& position)

No need for the WebCore:: qualifier here or anywhere else in this file.

> +    : HUDWidget(IntRect(position, IntSize(0, 0)))

No need for the parameters to the IntSize constructor.

> +    if (buttonResource) {
> +        m_buttonImage = Image::loadPlatformResource(buttonResource);
> +        m_buttonImage->ref();
> +        m_rect.setWidth(m_buttonImage->width());
> +        m_rect.setHeight(m_buttonImage->height());
> +    }
> +    if (buttonResourceAlt) {
> +        m_buttonImageAlt = Image::loadPlatformResource(buttonResourceAlt);
> +        m_buttonImageAlt->ref();
> +    }

Seems like these extra ref()s are unnecessary and will lead to leaks.

> +void HUDButton::draw(GraphicsContext& context)
> +{
> +    if (m_showAltButton && m_buttonImageAlt)
> +        context.drawImage(m_buttonImageAlt.get(), DeviceColorSpace, m_rect.location());
> +    else if (m_buttonImage)
> +        context.drawImage(m_buttonImage.get(), DeviceColorSpace, m_rect.location());
> +}

A local Image* variable might make this a little clearer.

> +void HUDSlider::draw(WebCore::GraphicsContext& context)
> +{
> +    // Draw gutter
> +    IntSize radius(m_rect.height() / 2, m_rect.height() / 2);
> +    context.fillRoundedRect(m_rect, radius, radius, radius, radius, Color(sliderGutterColor), DeviceColorSpace);
> +
> +    // Draw button
> +    context.setStrokeColor(Color(sliderButtonColor), DeviceColorSpace);
> +    context.setFillColor(Color(sliderButtonColor), DeviceColorSpace);
> +
> +    if (m_buttonShape == RoundButton)
> +        context.drawEllipse(IntRect(m_rect.location().x() + m_sliderPosition, m_rect.location().y() - (m_buttonSize - m_rect.height()) / 2, m_buttonSize, m_buttonSize));
> +    else {

An early return after drawEllipse would make this a bit cleaner.

> +        // Draw a diamond
> +        FloatPoint points[4];
> +        float half = m_buttonSize / 2;

The right-hand side of this statement is doing integer math. Is that what you want?

> +        points[0].setX(half + m_rect.location().x() + m_sliderPosition);
> +        points[0].setY(m_rect.location().y());
> +        points[1].setX(m_buttonSize + m_rect.location().x() + m_sliderPosition);
> +        points[1].setY(half + m_rect.location().y());

I like to order expressions like this in left-to-right and bottom-to-top order, so:

points[1].setX(m_rect.location().x() + m_sliderPosition + m_buttonSize);
points[1].setY(m_rect.location().y() + half);

But that's really just a personal preference.

> +void HUDSlider::drag(int x, int y, bool start)

Could you use an IntPoint here?

> +{
> +    if (start) {
> +        // When we start, we need to snap the slider position to the x position if we clicked the gutter.
> +        // But if we click the button, we need to drag relative to where we clicked down. We only need
> +        // to check X because we would not even get here unless Y were already inside.
> +        int relativeX = x - m_rect.location().x();
> +        if (relativeX >= m_sliderPosition && relativeX <= m_sliderPosition + m_buttonSize)
> +            m_dragStartOffset = x - m_sliderPosition;
> +        else
> +            m_dragStartOffset = m_rect.location().x() + m_buttonSize / 2;
> +    }
> +
> +    m_sliderPosition = x - m_dragStartOffset;
> +    if (m_sliderPosition < 0)
> +        m_sliderPosition = 0;
> +    else if (m_sliderPosition > m_rect.width() - m_buttonSize)
> +        m_sliderPosition = m_rect.width() - m_buttonSize;

min() and max() might make this a little clearer:

m_sliderPosition = max(0, min(m_rect.width() - m_buttonSize, m_sliderPosition));

> +void FullscreenVideoController::setMediaElement(HTMLMediaElement* mediaElement)
> +{
> +    m_mediaElement = mediaElement;
> +    if (!m_mediaElement) {
> +        // Can't do full-screen, just get out
> +        if (movie())
> +            movie()->exitFullscreen();
> +    }
> +}

Calling our own exitFullscreen() function seems slightly better here.




> +
> +void FullscreenVideoController::enterFullscreen()
> +{
> +    if (movie()) {
> +        m_videoWindow = movie()->enterFullscreen(this);
> +    
> +        RECT windowRect;
> +        GetClientRect(m_videoWindow, &windowRect);
> +        m_fullscreenSize.setWidth(windowRect.right - windowRect.left + 1);
> +        m_fullscreenSize.setHeight(windowRect.bottom - windowRect.top + 1);
> +
> +        createHUDWindow();
> +    }
> +}

An early return would get rid of the nesting here.

> +void FullscreenVideoController::exitFullscreen()
> +{
> +    if (movie())
> +        movie()->exitFullscreen();
> +}

Do we need to clear m_videoWindow and destroy the HUD window here?

> +const LPCWSTR fullscreenVideeoHUDWindowClassName = L"fullscreenVideeoHUDWindowClass";

This should be marked static.

> +void FullscreenVideoController::createHUDWindow()
> +{
> +    m_hudPosition.setX((m_fullscreenSize.width() - windowWidth) / 2);
> +    m_hudPosition.setY(m_fullscreenSize.height() * 0.9 - windowHeight / 2);

I think you should use a constant for 0.9 here.

> +static String timeToString(float time)
> +{
> +    if (!isfinite(time))
> +        time = 0;
> +    int seconds = fabsf(time); 
> +    int hours = seconds / (60 * 60);
> +    int minutes = (seconds / 60) % 60;
> +    seconds %= 60;
> +
> +    if (hours) {
> +        if (hours > 9)
> +            return String::format("%s%02d:%02d:%02d", (time < 0 ? "-" : ""), hours, minutes, seconds);
> +        return String::format("%s%01d:%02d:%02d", (time < 0 ? "-" : ""), hours, minutes, seconds);
> +    }
> +
> +    return String::format("%s%02d:%02d", (time < 0 ? "-" : ""), minutes, seconds);
> +}

Do we need to localize these format strings?

> +void FullscreenVideoController::draw()
> +{
> +    HDC windowDC = GetDC(m_hudWindow);
> +    HDC bitmapDC = CreateCompatibleDC(windowDC);
> +    SelectObject(bitmapDC, m_bitmap.get());
> +
> +    GraphicsContext context(bitmapDC, true);
> +
> +    context.save();

Do we really need to save the context just after creating it?

> +    // Draw the background
> +    IntSize outerRadius(borderRadius, borderRadius);
> +    IntRect outerSize(0, 0, windowWidth, windowHeight);
> +    IntSize innerRadius(borderRadius - borderThickness, borderRadius - borderThickness);
> +    IntRect innerSize(borderThickness, borderThickness, windowWidth - borderThickness * 2, windowHeight - borderThickness * 2);

It's strange to have IntRect variables with names that end in "Size".

> +    // Left string
> +    s = timeToString(currentTime());
> +    TextRun leftText(s);
> +    context.setFillColor(Color(textColor), DeviceColorSpace);
> +    context.drawText(font, leftText, IntPoint(windowWidth / 2 - timeSliderWidth / 2 - margin - font.width(leftText), windowHeight - margin - sliderHeight / 2 + 3));
> +
> +    // Right string
> +    s = timeToString(currentTime() - duration());
> +    TextRun rightText(s);
> +    context.setFillColor(Color(textColor), DeviceColorSpace);
> +    context.drawText(font, rightText, IntPoint(windowWidth / 2 + timeSliderWidth / 2 + margin, windowHeight - margin - sliderHeight / 2 + 3));

What are these "+ 3"s for? Can the 3 be put in a constant?

> +    // Copy to the window
> +    BLENDFUNCTION blendFunction = {AC_SRC_OVER, 0, 255, AC_SRC_ALPHA};
> +    SIZE size = { windowWidth, windowHeight };
> +    POINT sourcePoint = {0, 0};
> +    POINT destPoint = { m_hudPosition.x(), m_hudPosition.y() };
> +    BOOL result = UpdateLayeredWindow(m_hudWindow, windowDC, &destPoint, &size, bitmapDC, &sourcePoint, 0, &blendFunction, ULW_ALPHA);

The second parameter to UpdateLayeredWindow is supposed to be a DC for the screen, not for the window. But you can just pass in 0 instead.

The result variable is unused, which might cause problems in release builds.

> +    DWORD error = GetLastError();

I think this code shouldn't be checked in.

> +LRESULT FullscreenVideoController::hudWndProc(HWND wnd, UINT message, UINT wParam, LONG lParam)
> +{
> +    LONG_PTR longPtr = GetWindowLongPtr(wnd, 0);
> +    FullscreenVideoController* controller = reinterpret_cast<FullscreenVideoController*>(longPtr);
> +    if (!controller)
> +        return DefWindowProc(wnd, message, wParam, lParam);
> +
> +    switch (message) {
> +    case WM_PAINT:
> +        PAINTSTRUCT ps;
> +        BeginPaint(wnd, &ps);
> +        controller->draw();
> +        EndPaint(wnd, &ps);
> +        break;

As <http://msdn.microsoft.com/en-us/library/ms997507.aspx> says, you don't need to respond to WM_PAINT.

> +void FullscreenVideoController::onChar(int c)
> +{
> +    if (c == 0x1b) {
> +        // ESC exits full-screen
> +        if (m_mediaElement)
> +            m_mediaElement->exitFullscreen();
> +    } else if (c == 0x20) {
> +        // SPACE toggles play/pause
> +        togglePlay();
> +    }
> +}

VK_ESCAPE and VK_SPACE would be better than 0x1b and 0x20, respectively.

> +void FullscreenVideoController::onMouseDown(int x, int y)
> +{
> +    fullScreenToHUDCoordinates(x, y);
> +
> +    // Don't bother hit testing if we're outside the bounds of the window
> +    if (x < 0 || x >= windowWidth || y < 0 || y >= windowHeight)
> +        return;
> +
> +    m_pickedWidget = 0;

I think a name like m_mouseDownWidget or m_pressedWidget might be clearer.

> +void FullscreenVideoController::onMouseMove(int x, int y)
> +{
> +    fullScreenToHUDCoordinates(x, y);
> +
> +    if (m_pickedWidget) {
> +        m_pickedWidget->drag(x, y, false);
> +        if (m_pickedWidget == &m_volumeSlider)
> +            setVolume(m_volumeSlider.value());
> +        else if (m_pickedWidget == &m_timeSlider)
> +            setCurrentTime(m_timeSlider.value() * duration());
> +        draw();
> +    } else if (m_movingWindow) {
> +        m_hudPosition.move(x - m_moveOffset.x(), y - m_moveOffset.y());
> +        draw();
> +    }

Why do we have to redraw when we're just moving the HUD window?

> +void FullscreenVideoController::onMouseUp(int x, int y)
> +{
> +    fullScreenToHUDCoordinates(x, y);
> +    m_movingWindow = false;
> +
> +    // We are running a timer during mouse moves. We want to also run it
> +    // while the video is playing to update the time slider. So don't stop
> +    // it if the video is running
> +    if (canPlay())
> +        m_timer.stop();
> +
> +    if (m_pickedWidget) {
> +        if (m_playPauseButton.hitTest(x, y))
> +            togglePlay();

If I press the mouse down on one button, drag over to another button, and then release the mouse, this code will activate the dragged-to button. Is that what we want?

> +        else if (m_volumeUpButton.hitTest(x, y)) {
> +            setVolume(1);
> +            m_volumeSlider.setValue(1);
> +        } else if (m_volumeDownButton.hitTest(x, y)) {
> +            setVolume(0);
> +            m_volumeSlider.setValue(0);
> +        } else if (m_pickedWidget == &m_timeSlider)
> +            endScrubbing();
> +        else if (m_exitFullscreenButton.hitTest(x, y)) {
> +            if (m_mediaElement)
> +                m_mediaElement->exitFullscreen();
> +            return;
> +        }
> +    }
> +
> +    m_pickedWidget = 0;
> +    draw();
> +}

Why don't we want to clear m_pickedWidget in the "exit fullscreen" case?

> Property changes on: WebKit/win/FullscreenVideoController.cpp
> ___________________________________________________________________
> Added: svn:executable
>    + *

Please remove this property.

> +    HUDWidget(const WebCore::IntRect& rect)
> +        : m_rect(rect)
> +    { }

Either put the entire constructor on a single line, or put each brace on its own line.

> +    HUDButton(HUDButtonType type, const WebCore::IntPoint&);

No need for the "type" parameter name here.

> +class HUDSlider : public HUDWidget {
> +public:
> +    enum HUDSliderButtonShape { RoundButton, DiamondButton };
> +
> +    HUDSlider(HUDSliderButtonShape, int buttonSize, const WebCore::IntRect& rect);
> +    ~HUDSlider() { }
> +
> +    virtual void draw(WebCore::GraphicsContext&);
> +    virtual void drag(int x, int y, bool start);
> +    float value() const { return static_cast<float>(m_sliderPosition) / (m_rect.width() - m_buttonSize); }
> +    void setValue(float value) { m_sliderPosition = static_cast<int>(value * (m_rect.width() - m_buttonSize)); }
> +
> +private:
> +    HUDSliderButtonShape m_buttonShape;
> +    int m_buttonSize;
> +    int m_sliderPosition;

I think it would be better to name this variable m_buttonPosition. The whole class is the slider, so it seems weird for it to keep track of a "slider position".

> +    RefPtr<WebCore::HTMLMediaElement> m_mediaElement;
> +
> +    HWND m_hudWindow, m_videoWindow;
> +    OwnPtr<HBITMAP> m_bitmap;
> +    WebCore::IntSize m_fullscreenSize;
> +    WebCore::IntPoint m_hudPosition;
> +
> +    // HUD Support
> +    void fullScreenToHUDCoordinates(int& x, int& y) const
> +    {
> +        x -= m_hudPosition.x();
> +        y -= m_hudPosition.y();
> +    }

Can this take and return an IntPoint?

We normally put all data members after all function members.

> +    static void registerHUDWindowClass();
> +    static LRESULT CALLBACK hudWndProc(HWND, UINT message, WPARAM wParam, LPARAM lParam);

No need for the "wParam" and "lParam" parmeter names.

> +    void onMouseDown(int x, int y);
> +    void onMouseMove(int x, int y);
> +    void onMouseUp(int x, int y);

Can these take IntPoints?

> Property changes on: WebKit/win/FullscreenVideoController.h
> ___________________________________________________________________
> Added: svn:executable
>    + *

Please remove this property.

> +void WebView::enterFullscreenForNode(Node* node)
> +{
> +    if (!node->hasTagName(WebCore::HTMLNames::videoTag))
> +        return;

No need for the "WebCore::" qualifier here.

> +    HTMLMediaElement* videoElement = static_cast<HTMLMediaElement*>(node);
> +
> +    if (m_fullscreenController) {
> +        if (m_fullscreenController->mediaElement() == videoElement) {
> +            // The backend may just warn us that the underlaying plaftormMovie()
> +            // has changed. Just force an update.
> +            m_fullscreenController->setMediaElement(videoElement);
> +            return; // No more to do.
> +        }
> +
> +        // First exit Fullscreen for the old mediaElement.
> +        m_fullscreenController->mediaElement()->exitFullscreen();
> +        // This previous call has to trigger exitFullscreen,
> +        // which has to clear m_fullscreenController.
> +        ASSERT(!m_fullscreenController);
> +    }
> +    if (!m_fullscreenController) {

Given the first if and the assertion above, this if is guaranteed to be true, right?

> +        m_fullscreenController = new FullscreenVideoController();

No need for the () here.

> +bool WebChromeClient::supportsFullscreenForNode(const Node* node)
> +{
> +    return node->hasTagName(WebCore::HTMLNames::videoTag);
> +}

No need for the "WebCore::" qualifier here.

[Chris Marrin]

> > +static String timeToString(float time)
> > +{
> > +    if (!isfinite(time))
> > +        time = 0;
> > +    int seconds = fabsf(time); 
> > +    int hours = seconds / (60 * 60);
> > +    int minutes = (seconds / 60) % 60;
> > +    seconds %= 60;
> > +
> > +    if (hours) {
> > +        if (hours > 9)
> > +            return String::format("%s%02d:%02d:%02d", (time < 0 ? "-" : ""), hours, minutes, seconds);
> > +        return String::format("%s%01d:%02d:%02d", (time < 0 ? "-" : ""), hours, minutes, seconds);
> > +    }
> > +
> > +    return String::format("%s%02d:%02d", (time < 0 ? "-" : ""), minutes, seconds);
> > +}
> 
> Do we need to localize these format strings?

This is a duplicate of code in MediaControlTimeDisplayElement, which doesn't do any localization. There is a bug opened to unify these (https://bugs.webkit.org/show_bug.cgi?id=33281)

> 
> > +void FullscreenVideoController::draw()
> > +{
> > +    HDC windowDC = GetDC(m_hudWindow);
> > +    HDC bitmapDC = CreateCompatibleDC(windowDC);
> > +    SelectObject(bitmapDC, m_bitmap.get());
> > +
> > +    GraphicsContext context(bitmapDC, true);
> > +
> > +    context.save();
> 
> Do we really need to save the context just after creating it?

This was recommended by Simon as the desired style.

> > +void FullscreenVideoController::onMouseDown(int x, int y)
> > +{
> > +    fullScreenToHUDCoordinates(x, y);
> > +
> > +    // Don't bother hit testing if we're outside the bounds of the window
> > +    if (x < 0 || x >= windowWidth || y < 0 || y >= windowHeight)
> > +        return;
> > +
> > +    m_pickedWidget = 0;
> 
> I think a name like m_mouseDownWidget or m_pressedWidget might be clearer.

I named it m_hitWidget, riffing off the change I made for Simon from isPicked() to hitTest().

[Chris Marrin]

Created attachment 46014 [details]
Patch with Adam's issues addressed

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46014 [details] without any errors.

[Adam Roben (:aroben)]

Comment on attachment 46014 [details]
Patch with Adam's issues addressed

This patch has no ChangeLogs!

> +LRESULT QTMovieWin::fullscreenWndProc(HWND wnd, UINT message, WPARAM wParam, LPARAM lParam)
> +{
> +    QTMovieWin* movie = reinterpret_cast<QTMovieWin*>(GetProp(wnd, fullscreenQTMovieWinPointerProp));

I think static_cast will work here, since HANDLE is just a void*.

> +    // Set the 'this' pointer on the HWND
> +    HWND wnd = static_cast<HWND>(GetPortNativeWindow(m_private->m_fullscreenWindow));
> +    SetProp(wnd, fullscreenQTMovieWinPointerProp, reinterpret_cast<HANDLE>(this));

...and here.

> +class QTMovieWinFullscreenClient {
> +public:
> +    virtual LRESULT fullscreenClientWndProc(HWND, UINT message, WPARAM wParam, LPARAM lParam) = 0;

No need for the wParam and lParam parameter names.

> +void HUDSlider::drag(IntPoint point, bool start)

point can be a reference-to-const (const IntPoint&). (Sorry for not being explicit about this last time.)

> +FullscreenVideoController::~FullscreenVideoController()
> +{
> +    if (movie())
> +        movie()->exitFullscreen();
> +
> +    m_bitmap.clear();
> +}

m_bitmap.clear() will happen automatically in OwnPtr's destructor.

> +void FullscreenVideoController::setMediaElement(HTMLMediaElement* mediaElement)
> +{
> +    m_mediaElement = mediaElement;
> +    if (!m_mediaElement) {
> +        // Can't do full-screen, just get out
> +        exitFullscreen();
> +    }
> +}

Is it safe for someone to call setMediaElement(0) twice in a row? Will exitFullscreen() do something bad in that case?

The normal way we avoid this is to check to see that the media element is actually changing:

if (mediaElement == m_mediaElement)
    return;
...do the rest...

> +void FullscreenVideoController::enterFullscreen()
> +{
> +    if (!movie())
> +        return;
> +
> +    m_videoWindow = movie()->enterFullscreen(this);
> +
> +    RECT windowRect;
> +    GetClientRect(m_videoWindow, &windowRect);
> +    m_fullscreenSize.setWidth(windowRect.right - windowRect.left + 1);
> +    m_fullscreenSize.setHeight(windowRect.bottom - windowRect.top + 1);

What's with the "+ 1"s here?

> +void FullscreenVideoController::exitFullscreen()
> +{
> +    if (movie())
> +        movie()->exitFullscreen();
> +
> +    m_videoWindow = 0;
> +    SetWindowLongPtr(m_hudWindow, 0, 0);
> +    DestroyWindow(m_hudWindow);
> +}

Seems like we should clear out m_hudWindow at this point.

> +void FullscreenVideoController::registerHUDWindowClass()
> +{
> +    static bool haveRegisteredHUDWindowClass = false;

We normally omit the "= false" on statics like this, since it happens automatically.

> +void FullscreenVideoController::createHUDWindow()
> +{
> +    m_hudPosition.setX((m_fullscreenSize.width() - windowWidth) / 2);
> +    m_hudPosition.setY(m_fullscreenSize.height() * initialHUDPositionY - windowHeight / 2);
> +
> +    // Local variable that will hold the returned pixels. No need to cleanup this value. It
> +    // will get cleaned up when m_bitmap is destroyed in the dtor
> +    void* pixels;
> +    BitmapInfo bitmapInfo = BitmapInfo::createBottomUp(IntSize(windowWidth, windowHeight));
> +    m_bitmap.set(::CreateDIBSection(0, &bitmapInfo, DIB_RGB_COLORS, &pixels, 0, 0));
> +    
> +    // Dirty the window so the HUD draws
> +    RECT clearRect = { m_hudPosition.x(), m_hudPosition.y(), m_hudPosition.x() + windowWidth - 1, m_hudPosition.y() + windowHeight - 1 };

What's with these "- 1"s?

> +void FullscreenVideoController::draw()
> +{
> +    HDC windowDC = GetDC(m_hudWindow);
> +    HDC bitmapDC = CreateCompatibleDC(windowDC);

You can release windowDC right here.

> +    bool hitTest(WebCore::IntPoint point) const { return point.x() >= m_rect.x() && point.x() < m_rect.x() + m_rect.width() && point.y() >= m_rect.y() && point.y() < m_rect.y() + m_rect.height(); }

point can be a reference-to-const. I think you can just use IntRect::contains.

> +    virtual void drag(WebCore::IntPoint, bool start) { }

You should use a reference-to-const.

> +    WebCore::IntPoint fullScreenToHUDCoordinates(WebCore::IntPoint point) const
> +    {
> +        return WebCore::IntPoint(point.x()- m_hudPosition.x(), point.y() - m_hudPosition.y());
> +    }

point can be a reference-to-const.

[Chris Marrin]

Created attachment 46102 [details]
Another replacement patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46102 [details] without any errors.

[Adam Roben (:aroben)]

Comment on attachment 46102 [details]
Another replacement patch

> +    static LRESULT fullscreenWndProc(HWND, UINT message, WPARAM wParam, LPARAM lParam);

No need for the wParam and lParam parameter names.

> +    bool hitTest(const WebCore::IntPoint& point) const { return point.x() >= m_rect.x() && point.x() < m_rect.x() + m_rect.width() && point.y() >= m_rect.y() && point.y() < m_rect.y() + m_rect.height(); }

I still think this could just be: return m_rect.contains(point);

r=me

[Chris Marrin]

Landed in http://trac.webkit.org/changeset/52998