[webkit-reviews] review granted: [Bug 23140] <video> poster should scale like a video frame : [Attachment 46260] updated patch

[bugzilla-daemon at webkit.org]

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