REGRESSION(r248391): Web Inspector: changing Layout Direction Debug setting no longer adds dir="ltr" to body element

[Nikita Vasilyev]

This has to be recent.

[Nikita Vasilyev]

  WI.resolvedLayoutDirection = function()
  {
      let layoutDirection = WI.settings.debugLayoutDirection.value;
->    if (!WI.isDebugUIEnabled() || layoutDirection === WI.LayoutDirection.System)
          layoutDirection = InspectorFrontendHost.userInterfaceLayoutDirection();
      return layoutDirection;
  };


WI.isDebugUIEnabled returns undefined:

    WI.isDebugUIEnabled = function()
    {
        return WI.showDebugUISetting && WI.showDebugUISetting.value;
    };

WI.showDebugUISetting is undefined. This is where it's created:

    // This function is invoked after the inspector has loaded and has a backend target.
    WI.runBootstrapOperations = function() {
        WI.showDebugUISetting = new WI.Setting("show-debug-ui", false);

[Nikita Vasilyev]

Broke in https://trac.webkit.org/changeset/248391/webkit#file1

[Nikita Vasilyev]

This line broke it.

Main.js:
- 2707    if (layoutDirection === WI.LayoutDirection.System)
+ 2707    if (!WI.isDebugUIEnabled() || layoutDirection === WI.LayoutDirection.System)

[Devin Rousso]

This is something we don't want to expose outside of Web Inspector's debug mode (⌥⇧⌘D).

Are you saying that the layout direction doesn't work when NOT in the debug mode (⌥⇧⌘D)?

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #4)
> This is something we don't want to expose outside of Web Inspector's debug
> mode (⌥⇧⌘D).
> 
> Are you saying that the layout direction doesn't work when NOT in the debug
> mode (⌥⇧⌘D)?

It doesn't work in Debug mode.

[Devin Rousso]

Created attachment 375897 [details]
Patch

[Nikita Vasilyev]

This indeed fixes the regression. I'd r+.

Nit: I noticed that several settings are instantiated in WI.loaded() while none in Bootstrap.js. Would it be better to add this one to WI.loaded() as well for consistency?

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #7)
> Nit: I noticed that several settings are instantiated in WI.loaded() while none in Bootstrap.js. Would it be better to add this one to WI.loaded() as well for consistency?
`WI.loaded` exists just as a way to say "run this stuff once all the scripts have loaded, but potentially before we're done creating the DOM".  We primarily use it now for initial state setup (both UI and protocol).  Frankly, we could probably just inline all of that code inside the last script in Main.html/Test.html (or distribute it, as the comment in AppControllerBase.js suggests).

In the case of `WI.showDebugUISetting`, most of our other global state `WI.Setting` are created in Setting.js itself (e.g. when the script is parsed/executed, not in some event), I think it's fine to do a similar thing in Bootstrap.js.  Putting it in `WI.loaded` would have the same effect, just delayed slightly more.

[Joseph Pecoraro]

Comment on attachment 375897 [details]
Patch

rs=me

[WebKit Commit Bot]

Comment on attachment 375897 [details]
Patch

Clearing flags on attachment: 375897

Committed r248590: <https://trac.webkit.org/changeset/248590>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54240508>