Bug 218881 – Cleanup InternalSettings

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Cleanup InternalSettings

[Sam Weinig]

Created attachment 413991 [details]
Patch

[Sam Weinig]

Created attachment 413993 [details]
Patch

[Sam Weinig]

Created attachment 413998 [details]
Patch

[Tim Horton]

Comment on attachment 413998 [details]
Patch

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

> LayoutTests/fast/scrolling/rtl-scrollbars-text-selection-scrolled.html:5
> -    internals.settings.setUserInterfaceDirectionPolicy("View");
> +    internals.settings.setUserInterfaceDirectionPolicy("System");

Huh, do these tests still pass if you change the preference on your machine, now?

[Sam Weinig]

(In reply to Tim Horton from comment #4)
> Comment on attachment 413998 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=413998&action=review
> 
> > LayoutTests/fast/scrolling/rtl-scrollbars-text-selection-scrolled.html:5
> > -    internals.settings.setUserInterfaceDirectionPolicy("View");
> > +    internals.settings.setUserInterfaceDirectionPolicy("System");
> 
> Huh, do these tests still pass if you change the preference on your machine,
> now?

All I changed was the spelling of the enum value. I don’t think anything else will have changed.

[Sam Weinig]

(In reply to Sam Weinig from comment #5)
> (In reply to Tim Horton from comment #4)
> > Comment on attachment 413998 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=413998&action=review
> > 
> > > LayoutTests/fast/scrolling/rtl-scrollbars-text-selection-scrolled.html:5
> > > -    internals.settings.setUserInterfaceDirectionPolicy("View");
> > > +    internals.settings.setUserInterfaceDirectionPolicy("System");
> > 
> > Huh, do these tests still pass if you change the preference on your machine,
> > now?
> 
> All I changed was the spelling of the enum value. I don’t think anything
> else will have changed.

To be even clearer, here was the old code that mapped View to System:

-    if (equalLettersIgnoringASCIICase(policy, "content")) {
-        settings().setUserInterfaceDirectionPolicy(UserInterfaceDirectionPolicy::Content);
-        return { };
-    }
-    if (equalLettersIgnoringASCIICase(policy, "view")) {
-        settings().setUserInterfaceDirectionPolicy(UserInterfaceDirectionPolicy::System);
-        return { };
-    }
-    return Exception { InvalidAccessError };

Now it just maps "System" to System directly.

[EWS]

Committed r269784: <https://trac.webkit.org/changeset/269784>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413998 [details].

[Radar WebKit Bug Importer]

<rdar://problem/71374775>

[Tim Horton]

(In reply to Sam Weinig from comment #6)
> (In reply to Sam Weinig from comment #5)
> > (In reply to Tim Horton from comment #4)
> > > Comment on attachment 413998 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=413998&action=review
> > > 
> > > > LayoutTests/fast/scrolling/rtl-scrollbars-text-selection-scrolled.html:5
> > > > -    internals.settings.setUserInterfaceDirectionPolicy("View");
> > > > +    internals.settings.setUserInterfaceDirectionPolicy("System");
> > > 
> > > Huh, do these tests still pass if you change the preference on your machine,
> > > now?
> > 
> > All I changed was the spelling of the enum value. I don’t think anything
> > else will have changed.
> 
> To be even clearer, here was the old code that mapped View to System:
> 
> -    if (equalLettersIgnoringASCIICase(policy, "content")) {
> -       
> settings().setUserInterfaceDirectionPolicy(UserInterfaceDirectionPolicy::
> Content);
> -        return { };
> -    }
> -    if (equalLettersIgnoringASCIICase(policy, "view")) {
> -       
> settings().setUserInterfaceDirectionPolicy(UserInterfaceDirectionPolicy::
> System);
> -        return { };
> -    }
> -    return Exception { InvalidAccessError };
> 
> Now it just maps "System" to System directly.

Oh! Naively assumed "system" meant "app wide" and "view" meant "scoped to the view", since it is possible for those to differ. Now I wonder which it really is :D Anyway, making them all the same is A++.