Bug 209008 - Cleanup RenderMediaControls.cpp and RenderMediaControlElements.cpp
Summary: Cleanup RenderMediaControls.cpp and RenderMediaControlElements.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari 13
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-03-12 12:17 PDT by Peng Liu
Modified: 2020-03-14 12:20 PDT (History)
14 users (show)

See Also:


Attachments
Patch (24.25 KB, patch)
2020-03-13 16:33 PDT, Peng Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2020-03-12 12:17:12 PDT
Now RenderMediaControls.cpp is for windows only. We need to merge them.
Comment 1 Peng Liu 2020-03-13 16:33:08 PDT
Created attachment 393554 [details]
Patch
Comment 2 Daniel Bates 2020-03-14 00:09:06 PDT
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
Comment 3 Peng Liu 2020-03-14 11:11:54 PDT
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!
Comment 4 WebKit Commit Bot 2020-03-14 12:19:20 PDT
Comment on attachment 393554 [details]
Patch

Clearing flags on attachment: 393554

Committed r258469: <https://trac.webkit.org/changeset/258469>
Comment 5 WebKit Commit Bot 2020-03-14 12:19:22 PDT
All reviewed patches have been landed.  Closing bug.