[webkit-reviews] review granted: [Bug 173989] Top controls bars should invert with right-to-left user interface layout direction locale : [Attachment 314161] Patch

Fri Jun 30 00:09:44 PDT 2017

Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at apple.com>'s
request for review:
Bug 173989: Top controls bars should invert with right-to-left user interface
layout direction locale
https://bugs.webkit.org/show_bug.cgi?id=173989

Attachment 314161: Patch

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

--- Comment #3 from Dean Jackson <dino at apple.com> ---
Comment on attachment 314161
  --> https://bugs.webkit.org/attachment.cgi?id=314161
Patch

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

> Source/WebCore/ChangeLog:17
> +	   * Modules/modern-media-controls/controls/icon-service.js: Add new
RTL variants for the mute and unmute icons.

Why didn't we just flip the existing ones?

> Source/WebCore/ChangeLog:18
> +	   * Modules/modern-media-controls/controls/inline-media-controls.css:
Invert the position of the two top controls

Most people use invert to mean flip vertically. I think "swap" makes more sense
here, but I'm sure no one will be confused so don't worry.

> Source/WebCore/Modules/modern-media-controls/images/iOS/Mute-RTL.svg:1
> +<?xml version="1.0" encoding="UTF-8"?>

No need for the encoding attribute.

> Source/WebCore/Modules/modern-media-controls/images/iOS/Mute-RTL.svg:2
> +<svg width="22px" height="15px" viewBox="0 0 22 15" version="1.1"
xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">

No need for the xlink namespace, or the version attribute. You should probably
remove the width and height too.

> Source/WebCore/Modules/modern-media-controls/images/iOS/Mute-RTL.svg:6
> +    <!-- Generator: Sketch 45 (43475) - http://www.bohemiancoding.com/sketch
-->
> +    <title>_Assets/Both/Mute RTL</title>
> +    <desc>Created with Sketch.</desc>
> +    <defs></defs>

All this is unnecessary.

> Source/WebCore/Modules/modern-media-controls/images/iOS/Mute-RTL.svg:7
> +    <g id="Media-Control-Symbols" stroke="none" stroke-width="1" fill="none"
fill-rule="evenodd">

stroke="none" is redundant.

Also, no point setting stroke-width if there is no stroke. 

And why set fill to none and then the only child sets it back to black. Just
remove that attribute.

> Source/WebCore/Modules/modern-media-controls/images/iOS/Mute-RTL.svg:8
> +	   <g id="_Assets/Both/Mute-RTL" fill="#000000">

No need for this if the parent g gets rid of its fill attribute.

Also, no need for the id attribute.

> Source/WebCore/Modules/modern-media-controls/images/iOS/Mute-RTL.svg:9
> +	       <path d="M15.0384401,12.4336332 L14.7151198,12.7445465
C14.3273368,13.1174489 14,12.9886171 14,12.44141 L14,11.8340896
L15.0384401,12.4336332 Z M14,7.79263767 L14,2.56259782 C14,2.02224368
14.328475,1.87372699 14.7336617,2.24618642 L17.7297388,5.00026821 C17.7787854,5
17.8276579,5 17.877,5 L20.123,5 C20.195,5 20.266,5 20.338,5.00083333
C20.398,5.00083333 20.459,5.00166667 20.519,5.0025 C20.651,5.00583333
20.783,5.01166667 20.913,5.03166667 C21.045,5.05166667 21.168,5.08333333
21.288,5.13416667 C21.406,5.18416667 21.514,5.25 21.607,5.3275
C21.701,5.40583333 21.779,5.495 21.839,5.59333333 C21.9,5.69333333
21.939,5.79583333 21.962,5.90583333 C21.986,6.01416667 21.993,6.125
21.997,6.23416667 C21.999,6.28416667 21.999,6.335 21.999,6.385 C22,6.445
22,6.50416667 22,6.56416667 L22,8.43583333 C22,8.49583333 22,8.55583333
21.999,8.615 C21.999,8.66583333 21.999,8.71583333 21.997,8.76583333
C21.993,8.87583333 21.986,8.98583333 21.962,9.09416667 C21.939,9.20416667
21.9,9.30666667 21.839,9.40666667 C21.779,9.505 21.701,9.595 21.607,9.6725
C21.514,9.75083333 21.406,9.81583333 21.288,9.86583333 C21.168,9.91666667
21.045,9.94916667 20.913,9.96833333 C20.783,9.98833333 20.651,9.99416667
20.519,9.9975 C20.459,9.99916667 20.398,9.99916667 20.338,10 L20.123,10
L17.877,10 L17.8232637,10 L14,7.79263767 Z M12.0984016,10.7362012
C12.0719556,10.7963525 12.0306958,10.8516898 11.975,10.8969802
C11.786,11.0514833 11.502,11.0295606 11.34,10.848959 C11.0688063,10.5458841
10.8359472,10.228546 10.6440423,9.89652647 L12.0984016,10.7362012 Z
M10.3551781,5.68829877 C10.5818609,5.1395229 10.9135304,4.62764579
11.34,4.15104101 C11.502,3.97043942 11.786,3.94851668 11.975,4.10301978
C12.165,4.25752287 12.187,4.52894723 12.026,4.70954882 C11.6287256,5.15341338
11.3293676,5.62791235 11.1397237,6.1412564 L10.3551781,5.68829877 Z
M6.00498141,7.21816342 L6.90494133,7.73775552 C6.9605099,9.41225607
7.60202565,11.0012512 8.725,12.307171 C8.882,12.4897218 8.852,12.7568441
8.659,12.90536 C8.466,13.0528445 8.182,13.0249978 8.025,12.842447
C6.719,11.3242845 6,9.45649073 6,7.5 C6,7.40583933 6.00166538,7.31188411
6.00498141,7.21816342 Z M6.95104701,3.72292276 C7.24532664,3.17072984
7.60477673,2.64604244 8.025,2.15755298 C8.182,1.97500219 8.466,1.94715546
8.659,2.09464 C8.852,2.2431559 8.882,2.51027825 8.725,2.69282905
C8.32972705,3.15249651 7.99410451,3.64723565 7.72268909,4.16843052
L6.95104701,3.72292276 Z M2.26185384,5.05706771 L3.08281215,5.53104821
C2.96197404,6.16994634 2.901,6.82684524 2.901,7.5 C2.901,10.0310229
3.763,12.3322339 5.428,14.3108837 C5.583,14.4953516 5.551,14.7622632
5.355,14.9075703 C5.161,15.0539079 4.877,15.0229915 4.722,14.8395542
C2.929,12.7083835 2,10.2247657 2,7.5 C2,6.66178829 2.08791538,5.84639747
2.26185384,5.05706771 Z M3.57096454,1.77143124 C3.90321848,1.21476886
4.28720592,0.677244684 4.722,0.160445787 C4.877,-0.0229915406
5.161,-0.0539079442 5.355,0.0924296995 C5.551,0.237736796 5.583,0.504648414
5.428,0.689116289 C5.01602607,1.17869716 4.6532138,1.68802602
4.34045694,2.21569788 L3.57096454,1.77143124 Z M0.348205106,1.99951896
C0.555310106,1.64080257 1.00825831,1.51457545 1.37243073,1.7248305
L21.3782047,13.2751695 C21.7367591,13.4821809 21.8609811,13.9382988
21.6524304,14.2995191 C21.4453254,14.6582355 20.9923772,14.7844627
20.6282047,14.5742076 L0.622430735,3.0238686 C0.263876405,2.81685716
0.139654415,2.36073935 0.348205106,1.99951896 Z" id="iOS/Fullscreen/Mute"
style="mix-blend-mode: normal;"></path>

This is way more significant digits than necessary.

Remove the mix-blend-mode style attribute. I don't even know why that is there.

And no need for the id attribute.

> Source/WebCore/Modules/modern-media-controls/images/iOS/VolumeHi-RTL.svg:1
> +<?xml version="1.0" encoding="UTF-8"?>

Ditto from the previous file.

> Source/WebCore/Modules/modern-media-controls/images/macOS/Mute-RTL.svg:1
> +<?xml version="1.0" encoding="UTF-8"?>

Ditto.

> Source/WebCore/Modules/modern-media-controls/images/macOS/VolumeHi-RTL.svg:1
> +<?xml version="1.0" encoding="UTF-8"?>

Ditto.