[webkit-reviews] review granted: [Bug 209008] Cleanup RenderMediaControls.cpp and RenderMediaControlElements.cpp : [Attachment 393554] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Peng Liu <peng.liu6 at apple.com>'s
request for review:
Bug 209008: Cleanup RenderMediaControls.cpp and RenderMediaControlElements.cpp
https://bugs.webkit.org/show_bug.cgi?id=209008

Attachment 393554: Patch

https://bugs.webkit.org/attachment.cgi?id=393554&action=review




--- Comment #2 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 393554
  --> https://bugs.webkit.org/attachment.cgi?id=393554
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393554&action=review

> Source/WebCore/rendering/RenderMediaControls.cpp:75
> +static const int minWidthToDisplayTimeDisplays = 45 + 100 + 45;

Ok as-is. No change needed. Optimal solution is to make constexpr.

> Source/WebCore/rendering/RenderMediaControls.cpp:88
>  void RenderMediaControls::adjustMediaSliderThumbSize(RenderStyle& style)

This patch OK as-is. The optimal clean up would break out each class into its
own named file and remove this file. The former because:

1. Makes it easy to find these classes since each class is in its own file.
2. Because of (1) can remove the need for ------- comments

 The latter because:
1. This function does nothing meaningful  (It crashes in debug!).
2. Because of (1), it can be removed.
3. Because of (2), this file is empty of impl

> LayoutTests/media/track/track-cue-rendering-rtl.html:-24
> -	   function testCueStyle()

This ok as-is. No change needed. The optimal patch would break this diff out
into its own bug because:

1. Keeps patch focused on one thing