Let's see how messed up everything is!
Created attachment 301333 [details] Proposed Fix
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(...))`.
> > 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.
(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.
(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.
<rdar://problem/30456742>
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.
Created attachment 301436 [details] PATCH v2
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
(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
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.
Created attachment 301784 [details] [Screenshot] After - DebugUI turned on
Committed r212445: <http://trac.webkit.org/changeset/212445>