Bug 171402 - Web Inspector: RTL: inherit system layout direction by default
Summary: Web Inspector: RTL: inherit system layout direction by default
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-27 16:14 PDT by BJ Burg
Modified: 2018-04-29 22:53 PDT (History)
6 users (show)

See Also:


Attachments
Patch (11.30 KB, patch)
2017-04-27 16:28 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Patch (9.55 KB, patch)
2017-04-27 16:39 PDT, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2017-04-27 16:14:46 PDT
Ready, set, file bugs!
Comment 1 BJ Burg 2017-04-27 16:15:13 PDT
<rdar://problem/30753626>
Comment 2 BJ Burg 2017-04-27 16:28:42 PDT
Created attachment 308473 [details]
Patch
Comment 3 Joseph Pecoraro 2017-04-27 16:38:45 PDT
Comment on attachment 308473 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Base/Setting.js:115
> -    // FIXME: change initial value to 'system' once we are happy with RTL support.
> -    // This will cause Web Inspector to use the system user interface layout direction.
> -    layoutDirection: new WebInspector.Setting("layout-direction", "ltr"),
> +    layoutDirectionOverride: new WebInspector.Setting("layout-direction-override", "system"),

I realize the storage name needs to change in order for us to get the new default value of "system" going forward.

However I think WebInspector.settings.layoutDirection is way better than WebInspector.settings.layoutDirectionOverride. I suggest we keep that.

This brings to light that in the future we should name settings that are in experimental stages with a storage name "...-experimental" so that when we turn it on by default and rename it we can get the better name when it graduates!
Comment 4 BJ Burg 2017-04-27 16:39:37 PDT
Created attachment 308474 [details]
Patch
Comment 5 BJ Burg 2017-04-27 16:41:10 PDT
(In reply to Joseph Pecoraro from comment #3)
> Comment on attachment 308473 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308473&action=review
> 
> r=me
> 
> > Source/WebInspectorUI/UserInterface/Base/Setting.js:115
> > -    // FIXME: change initial value to 'system' once we are happy with RTL support.
> > -    // This will cause Web Inspector to use the system user interface layout direction.
> > -    layoutDirection: new WebInspector.Setting("layout-direction", "ltr"),
> > +    layoutDirectionOverride: new WebInspector.Setting("layout-direction-override", "system"),
> 
> I realize the storage name needs to change in order for us to get the new
> default value of "system" going forward.
> 
> However I think WebInspector.settings.layoutDirection is way better than
> WebInspector.settings.layoutDirectionOverride. I suggest we keep that.
> 
> This brings to light that in the future we should name settings that are in
> experimental stages with a storage name "...-experimental" so that when we
> turn it on by default and rename it we can get the better name when it
> graduates!

OK!
Comment 6 BJ Burg 2017-04-27 16:45:00 PDT
Committed r215906: <http://trac.webkit.org/changeset/215906>
Comment 7 Matt Baker 2017-05-01 16:28:07 PDT
(In reply to Joseph Pecoraro from comment #3)
> This brings to light that in the future we should name settings that are in
> experimental stages with a storage name "...-experimental" so that when we
> turn it on by default and rename it we can get the better name when it
> graduates!

We could either:

1) Create an ExperimentalSetting class which adds the storage name.
2) Add an `experimental` property (default false) to Setting which does the same thing.
Comment 8 Daniel Bates 2018-04-29 22:53:48 PDT
Comment on attachment 308474 [details]
Patch

Clearing review flag as this patch was landed per comment 6.