Now RenderMediaControls.cpp is for windows only. We need to merge them.
Created attachment 393554 [details] Patch
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 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 on attachment 393554 [details] Patch Clearing flags on attachment: 393554 Committed r258469: <https://trac.webkit.org/changeset/258469>
All reviewed patches have been landed. Closing bug.