[webkit-reviews] review denied: [Bug 82754] video doesn't display when <video> size is same as video resource size : [Attachment 134843] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied wjia at chromium.org's request
for review:
Bug 82754: video doesn't display when <video> size is same as video resource
size
https://bugs.webkit.org/show_bug.cgi?id=82754

Attachment 134843: Patch
https://bugs.webkit.org/attachment.cgi?id=134843&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=134843&action=review


Marking r- because this needs a test.

>> Source/WebCore/ChangeLog:8
>> +	    No new tests. (OOPS!)
> 
> You should remove the 'No new tests' and either add and list tests, or
explain why no new tests were possible.  [changelog/nonewtests] [5]

It is definitely worth creating a test for this!

> Source/WebCore/rendering/RenderVideo.cpp:129
> +	   if (!size.isEmpty()) {
> +	       if (!m_hasVideoSourceSize) {
> +		   m_hasVideoSourceSize = true;
> +		   m_forceUpdateIntrinsicSize = true;
> +	       }
>	       return size;
> +	   }

Instead of using two bools, you could use a single enum. Something like:

enum IntrinsicSizeUpdateRequired { None, Pending, Done };

if (!size.isEmpty()) {
    if (m_intrinsicSizeUpdateRequired == None)
	m_intrinsicSizeUpdateRequired = Pending;
    ...

etc.