[webkit-reviews] review granted: [Bug 23140] <video> poster should scale like a video frame : [Attachment 46260] updated patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jan 11 10:36:10 PST 2010
Simon Fraser (smfr) <simon.fraser at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 23140: <video> poster should scale like a video frame
https://bugs.webkit.org/show_bug.cgi?id=23140
Attachment 46260: updated patch
https://bugs.webkit.org/attachment.cgi?id=46260&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 53064)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,85 @@
> +2010-01-10 Eric Carlson <eric.carlson at apple.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + rdar://5684062
Should use a valid URL as copied from Radar.
> Index: WebCore/html/HTMLMediaElement.h
> ===================================================================
> --- WebCore/html/HTMLMediaElement.h (revision 53038)
> +++ WebCore/html/HTMLMediaElement.h (working copy)
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (C) 2007, 2008, 2009 Apple Inc. All rights reserved.
> + * Copyright (C) 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
> *
> * Redistribution and use in source and binary forms, with or without
> * modification, are permitted provided that the following conditions
> @@ -86,7 +86,7 @@ public:
> MediaPlayer::MovieLoadType movieLoadType() const;
>
> bool inActiveDocument() const { return m_inActiveDocument; }
> -
> +
> // DOM API
> // error state
> PassRefPtr<MediaError> error() const;
Only whitespace and the copyright changed in this file.
> Index: WebCore/html/HTMLVideoElement.cpp
> ===================================================================
> + if (m_shouldDisplayPoster) {
I have a minor preference for calling this m_shouldDisplayPosterImage
> Index: WebCore/html/HTMLVideoElement.h
> ===================================================================
> + const KURL& poster() const { return m_posterURL; }
> void setPoster(const String&);
Odd that the getter and setter use different types. I'd also prefer them to be
named posterURL() and setPosterURL().
> Index: WebCore/loader/ImageLoader.cpp
> ===================================================================
> - if (!renderer->isImage())
> + if (!renderer->isImage() && !renderer->isVideo())
> return;
> RenderImage* imageRenderer = toRenderImage(renderer);
Maybe this should be calling toRenderImage()?
> Index: WebCore/rendering/RenderImage.h
> ===================================================================
> + virtual void paint(GraphicsContext*, const IntRect&);
I'd prefer this be called paintIntoRect().
> inline RenderImage* toRenderImage(RenderObject* object)
> {
> - ASSERT(!object || object->isRenderImage());
> + ASSERT(!object || object->isRenderImage() || object->isVideo());
Shouldn't need this change.
> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog (revision 53064)
> +++ LayoutTests/ChangeLog (working copy)
> @@ -1,3 +1,17 @@
> +2010-01-10 Eric Carlson <eric.carlson at apple.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + rdar://5684062
> + https://bugs.webkit.org/show_bug.cgi?id=23094
> + Flash of white when switching from poster image to video playback
> +
> + https://bugs.webkit.org/show_bug.cgi?id=23140
> + <video> poster should scale like a video frame
> +
> + * media/video-poster-expected.txt: Remove blank line at beginning of
test result present
> + as a side effect of the media element using RenderImage to display
the poster.
> +
> 2010-01-10 Daniel Bates <dbates at webkit.org>
>
> Rubber-stamped by Eric Seidel.
> Index: LayoutTests/media/video-poster-expected.txt
> ===================================================================
> --- LayoutTests/media/video-poster-expected.txt (revision 53038)
> +++ LayoutTests/media/video-poster-expected.txt (working copy)
> @@ -1,4 +1,3 @@
> -
> EXPECTED (video.getAttribute('poster') == 'content/greenbox.png') OK
> EXPECTED (relativeURL(video.poster) == 'content/greenbox.png') OK
> EXPECTED (video.getAttribute('poster') == 'content/abe.png') OK
I'd like to see a new testcase that tests various configurations of poster
image size (width- and height-constrained aspect ratio sizing of poster image).
r=me
More information about the webkit-reviews
mailing list