Bug 209008 – Cleanup RenderMediaControls.cpp and RenderMediaControlElements.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Peng Liu]

Now RenderMediaControls.cpp is for windows only. We need to merge them.

[Peng Liu]

Created attachment 393554 [details]
Patch

[Daniel Bates]

Comment on attachment 393554 [details]
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

[Peng Liu]

Comment on attachment 393554 [details]
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.

Agree, thanks!

>> 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

Right, thanks!

>> 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

Absolutely correct!

[WebKit Commit Bot]

Comment on attachment 393554 [details]
Patch

Clearing flags on attachment: 393554

Committed r258469: <https://trac.webkit.org/changeset/258469>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.