Bug 185458 - Use StyleColor::Options in more places
Summary: Use StyleColor::Options in more places
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-08 16:56 PDT by Timothy Hatcher
Modified: 2018-05-09 08:18 PDT (History)
8 users (show)

See Also:


Attachments
Patch (28.31 KB, patch)
2018-05-08 16:59 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (29.89 KB, patch)
2018-05-08 17:15 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff
Patch (31.96 KB, patch)
2018-05-08 19:31 PDT, Timothy Hatcher
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2018-05-08 16:56:17 PDT
Using StyleColor::Options will avoid some confusing bools.

rdar://problem/39853798
Comment 1 Timothy Hatcher 2018-05-08 16:59:39 PDT
Created attachment 339903 [details]
Patch
Comment 2 Megan Gardner 2018-05-08 17:12:01 PDT
Comment on attachment 339903 [details]
Patch

R+ from me as well
Comment 3 Timothy Hatcher 2018-05-08 17:15:33 PDT
Created attachment 339906 [details]
Patch
Comment 4 Simon Fraser (smfr) 2018-05-08 17:18:33 PDT
Comment on attachment 339906 [details]
Patch

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

> Source/WebCore/dom/Document.h:548
> +    bool defaultAppearance() const;

This function name is inscrutable. It sounds like it should return an "appearance" thing but returns a bool?
Comment 5 Timothy Hatcher 2018-05-08 17:23:51 PDT
(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 339906 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339906&action=review
> 
> > Source/WebCore/dom/Document.h:548
> > +    bool defaultAppearance() const;
> 
> This function name is inscrutable. It sounds like it should return an
> "appearance" thing but returns a bool?

This matches the name in other parts of the code. We should rename this everywhere in a separate step.
Comment 6 Brent Fulgham 2018-05-08 17:29:37 PDT
Comment on attachment 339906 [details]
Patch

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

> Source/WebCore/platform/mac/LocalDefaultSystemAppearance.mm:51
> +    [NSAppearance setCurrentAppearance:m_savedSystemAppearance.get()];

We should be careful these calls are not happening in the WebContent process. We are locking down CGSWindowServer access, which may make these calls fail. They should be messaged back and forth to the UIProcess.
Comment 7 Tim Horton 2018-05-08 17:32:28 PDT
(In reply to Brent Fulgham from comment #6)
> Comment on attachment 339906 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339906&action=review
> 
> > Source/WebCore/platform/mac/LocalDefaultSystemAppearance.mm:51
> > +    [NSAppearance setCurrentAppearance:m_savedSystemAppearance.get()];
> 
> We should be careful these calls are not happening in the WebContent
> process. We are locking down CGSWindowServer access, which may make these
> calls fail. They should be messaged back and forth to the UIProcess.

Not accurate in this case.
Comment 8 Timothy Hatcher 2018-05-08 19:31:31 PDT
Created attachment 339920 [details]
Patch
Comment 9 WebKit Commit Bot 2018-05-09 08:17:14 PDT
Comment on attachment 339920 [details]
Patch

Clearing flags on attachment: 339920

Committed r231557: <https://trac.webkit.org/changeset/231557>
Comment 10 WebKit Commit Bot 2018-05-09 08:17:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2018-05-09 08:18:22 PDT
<rdar://problem/40092603>