[webkit-reviews] review denied: [Bug 31318] Windows: Implement full screen mode for <video> : [Attachment 46014] Patch with Adam's issues addressed

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 31318: Windows: Implement full screen mode for <video>
https://bugs.webkit.org/show_bug.cgi?id=31318

Attachment 46014: Patch with Adam's issues addressed
https://bugs.webkit.org/attachment.cgi?id=46014&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
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.