Bug 218881 - Cleanup InternalSettings
Summary: Cleanup InternalSettings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-12 18:41 PST by Sam Weinig
Modified: 2020-11-13 11:39 PST (History)
4 users (show)

See Also:


Attachments
Patch (109.60 KB, patch)
2020-11-12 18:45 PST, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (109.60 KB, patch)
2020-11-12 19:09 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (106.00 KB, patch)
2020-11-12 20:06 PST, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2020-11-12 18:41:41 PST
Cleanup InternalSettings
Comment 1 Sam Weinig 2020-11-12 18:45:20 PST Comment hidden (obsolete)
Comment 2 Sam Weinig 2020-11-12 19:09:47 PST Comment hidden (obsolete)
Comment 3 Sam Weinig 2020-11-12 20:06:46 PST
Created attachment 413998 [details]
Patch
Comment 4 Tim Horton 2020-11-12 21:31:36 PST
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?
Comment 5 Sam Weinig 2020-11-13 06:41:39 PST
(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.
Comment 6 Sam Weinig 2020-11-13 07:18:04 PST
(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.
Comment 7 EWS 2020-11-13 09:47:22 PST
Committed r269784: <https://trac.webkit.org/changeset/269784>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413998 [details].
Comment 8 Radar WebKit Bug Importer 2020-11-13 09:48:18 PST
<rdar://problem/71374775>
Comment 9 Tim Horton 2020-11-13 11:39:03 PST
(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++.