[webkit-reviews] review denied: [Bug 54685] Remove flakiness from media/video-poster.html (in service of http://crbug.com/60845) : [Attachment 83241] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Ami Fischman
<fischman at chromium.org>'s request for review:
Bug 54685: Remove flakiness from media/video-poster.html (in service of
http://crbug.com/60845)
https://bugs.webkit.org/show_bug.cgi?id=54685

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

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

Thanks for the quick response!

It looks like this diff is against r78778, not TOT, so I don't think the commit
bot will work.	Noted stylistic changes it would be good to have as long as
this needs to be revised anyway.


> LayoutTests/media/video-poster.html:50
> +		 if (video.clientWidth == expectedWidth && 
> +		     video.clientHeight == expectedHeight) {

I don't think it helps readability to break the test into two lines

> LayoutTests/media/video-poster.html:53
> +		   callback();
> +		   return;
> +		 }

An early return when the rest of the function only has one line seems
unnecessary, I think an "else" would be easier to follow.

> LayoutTests/media/video-poster.html:57
> +		 window.setTimeout(listenForWidthAndHeight, 20, 
> +				   expectedWidth, expectedHeight, callback);

No need to break the function onto two lines.

> LayoutTests/media/video-poster.html:59
> +		   

As long as you are editing the file, can you kill this extra white-space?

> LayoutTests/media/video-poster.html:89
> +		   listenForWidthAndHeight(
> +		     currentPoster.width, currentPoster.height, testPoster);

No line break please.