[webkit-reviews] review denied: [Bug 31318] Windows: Implement full screen mode for <video> : [Attachment 45933] Replacement patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jan 6 10:00:51 PST 2010
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 45933: Replacement patch
https://bugs.webkit.org/attachment.cgi?id=45933&action=review
------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> @@ -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 @@
> +2010-01-04 Chris Marrin <cmarrin at apple.com>
> +
> + 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.
More information about the webkit-reviews
mailing list