Bug 168222 – Web Inspector: add DebugUI setting to manually override UI layout direction

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

Let's see how messed up everything is!

[Blaze Burg]

Created attachment 301333 [details]
Proposed Fix

[Devin Rousso]

Comment on attachment 301333 [details]
Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Base/Main.js:2144
> +    if (!allowedValues.has(value))

I think you can express this simpler as `Object.values(WebInspector.LayoutDirection).includes(value)`.  This way it will also support future changes to WI.LayoutDirection (although I think that's probably not going to happen).

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:202
> +                let options = new Map()

While I am not against this style, I personally would find it clearer to just use the nested array syntax:

let options = new Map([
    [WebInspector.LayoutDirection.System, WebInspector.unlocalizedString("System Default")],
    [WebInspector.LayoutDirection.LTR, WebInspector.unlocalizedString("Left to Right (LTR)")],
    [WebInspector.LayoutDirection.RTL, WebInspector.unlocalizedString("Right to Left (RTL)")]
]);

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:207
> +                for (var [setting, label] of options) {

NIT: let instead of var
Also, I think the name "value" or "key" is better than "setting", as the latter implies it's a WebInspector.Setting object.

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:208
> +                    let option = select.createChild("option");

OK now I'm confused.  Are we using `createChild` or are we trying to avoid it?  In some patches I've done in the past I've been told to use `element.appendChild(document.createElement(...))`.

[Joseph Pecoraro]

> > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:208
> > +                    let option = select.createChild("option");
> 
> OK now I'm confused.  Are we using `createChild` or are we trying to avoid
> it?  In some patches I've done in the past I've been told to use
> `element.appendChild(document.createElement(...))`.

I would rather we just use the pure DOM methods. But I have no reasoning other then clarity and personal preference.

[Matt Baker]

(In reply to comment #3)
> > > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:208
> > > +                    let option = select.createChild("option");
> > 
> > OK now I'm confused.  Are we using `createChild` or are we trying to avoid
> > it?  In some patches I've done in the past I've been told to use
> > `element.appendChild(document.createElement(...))`.
> 
> I would rather we just use the pure DOM methods. But I have no reasoning
> other then clarity and personal preference.

I agree that using createChild makes the code less clear, for a fairly insignificant increase in convenience. We should strike it from the code base, IMO.

[Blaze Burg]

(In reply to comment #2)
> Comment on attachment 301333 [details]
> Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301333&action=review
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:2144
> > +    if (!allowedValues.has(value))
> 
> I think you can express this simpler as
> `Object.values(WebInspector.LayoutDirection).includes(value)`.  This way it
> will also support future changes to WI.LayoutDirection (although I think
> that's probably not going to happen).
> 
> > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:202
> > +                let options = new Map()
> 
> While I am not against this style, I personally would find it clearer to
> just use the nested array syntax:
> 
> let options = new Map([
>     [WebInspector.LayoutDirection.System,
> WebInspector.unlocalizedString("System Default")],
>     [WebInspector.LayoutDirection.LTR, WebInspector.unlocalizedString("Left
> to Right (LTR)")],
>     [WebInspector.LayoutDirection.RTL, WebInspector.unlocalizedString("Right
> to Left (RTL)")]
> ]);
> 
> > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:207
> > +                for (var [setting, label] of options) {
> 
> NIT: let instead of var
> Also, I think the name "value" or "key" is better than "setting", as the
> latter implies it's a WebInspector.Setting object.
> 
> > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:208
> > +                    let option = select.createChild("option");
> 
> OK now I'm confused.  Are we using `createChild` or are we trying to avoid
> it?  In some patches I've done in the past I've been told to use
> `element.appendChild(document.createElement(...))`.

This was copy-pasted from the above code. Let's clean up the entire file at once and not stall this feature.

[Blaze Burg]

<rdar://problem/30456742>

[Blaze Burg]

Comment on attachment 301333 [details]
Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:202
>> +                let options = new Map()
> 
> While I am not against this style, I personally would find it clearer to just use the nested array syntax:
> 
> let options = new Map([
>     [WebInspector.LayoutDirection.System, WebInspector.unlocalizedString("System Default")],
>     [WebInspector.LayoutDirection.LTR, WebInspector.unlocalizedString("Left to Right (LTR)")],
>     [WebInspector.LayoutDirection.RTL, WebInspector.unlocalizedString("Right to Left (RTL)")]
> ]);

OK

>> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:207
>> +                for (var [setting, label] of options) {
> 
> NIT: let instead of var
> Also, I think the name "value" or "key" is better than "setting", as the latter implies it's a WebInspector.Setting object.

OK (though i think it's less clear locally)

>> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:208
>> +                    let option = select.createChild("option");
> 
> OK now I'm confused.  Are we using `createChild` or are we trying to avoid it?  In some patches I've done in the past I've been told to use `element.appendChild(document.createElement(...))`.

I'll switch it.

[Blaze Burg]

Created attachment 301436 [details]
PATCH v2

[Joseph Pecoraro]

Comment on attachment 301436 [details]
PATCH v2

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

r=me

> Source/WebInspectorUI/UserInterface/Base/Main.js:54
> +}

Nit: Semicolon

> Source/WebInspectorUI/UserInterface/Base/Main.js:2135
> +}

Nit: Semicolon

> Source/WebInspectorUI/UserInterface/Base/Main.js:2146
> +    window.location.reload();

Just `location.reload()` is fine.

> Source/WebInspectorUI/UserInterface/Base/Main.js:2147
> +}

Nit: Semicolon

[Blaze Burg]

(In reply to comment #9)
> Comment on attachment 301436 [details]
> PATCH v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301436&action=review
> 
> r=me
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:54
> > +}
> 
> Nit: Semicolon
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:2135
> > +}
> 
> Nit: Semicolon
> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:2146
> > +    window.location.reload();
> 
> Just `location.reload()` is fine.

I really, really prefer using `window` to avoid needing to whitelist more global variables in my brain and in ESLint. We tend to use window.location in most places. And, in many places `location` is shadowed by a local variable.

> 
> > Source/WebInspectorUI/UserInterface/Base/Main.js:2147
> > +}
> 
> Nit: Semicolon

[Blaze Burg]

To test:
 - Go to settings tabs
 - Toggle DebugUI in engineering build (Cmd-Shift-Opt + D)
 - Use the select to change layout direction value

Expected Behavior:
 - Turning DebugUI on should make the setting appear immediately.
 - Turning DebugUI back off should make the setting disappear immediately.
 - Changing the value should cause the inspector frontend to reload immediately.
 - The reloaded UI should have the specified layout direction.

[Blaze Burg]

Created attachment 301784 [details]
[Screenshot] After - DebugUI turned on

[Blaze Burg]

Committed r212445: <http://trac.webkit.org/changeset/212445>