Bug 191767 - [iOS] Adopt ScreenProperties class.
Summary: [iOS] Adopt ScreenProperties class.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-16 13:07 PST by Per Arne Vollan
Modified: 2020-03-26 10:04 PDT (History)
2 users (show)

See Also:


Attachments
Patch (26.46 KB, patch)
2018-11-16 13:30 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (25.46 KB, patch)
2018-11-16 14:51 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (26.03 KB, patch)
2018-11-27 13:56 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (26.01 KB, patch)
2018-11-27 14:34 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (31.33 KB, patch)
2020-03-25 15:50 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (31.83 KB, patch)
2020-03-25 16:12 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (32.02 KB, patch)
2020-03-26 07:08 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (31.28 KB, patch)
2020-03-26 08:16 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2018-11-16 13:07:19 PST
On macOS, the ScreenProperties class is used to collect screen properties in the UI process and forward these to the Web process. We should do this on iOS as well.
Comment 1 Per Arne Vollan 2018-11-16 13:30:22 PST
Created attachment 355109 [details]
Patch
Comment 2 Brent Fulgham 2018-11-16 13:43:45 PST
Comment on attachment 355109 [details]
Patch

I think this looks right, but its hard to tell with the patch not applying to trunk. Could you re-upload your patch after rebasing your sources?
Comment 3 Per Arne Vollan 2018-11-16 14:51:52 PST
Created attachment 355130 [details]
Patch
Comment 4 Per Arne Vollan 2018-11-27 13:56:39 PST
Created attachment 355775 [details]
Patch
Comment 5 Per Arne Vollan 2018-11-27 14:34:53 PST
Created attachment 355785 [details]
Patch
Comment 6 Brent Fulgham 2018-11-27 17:28:57 PST
Comment on attachment 355785 [details]
Patch

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

> Source/WebCore/platform/ScreenProperties.h:40
> +#if PLATFORM(COCOA)

Do we have any build that was PLATFORM(MAC) previously, but not PLATFORM(COCOA)? I don’t think this macro is needed for anything building with this file, unless someone is includIn ScreeenProperties unconditionally. Or, just make the file PLATFORM(COCOA).

Maybe that’s why WinCairo is failing to build?

> Source/WebKit/Shared/WebProcessCreationParameters.cpp:161
>      encoder << screenProperties;

If this is now unconditional, it seems like WinCairo and GTK need to implement ScreenProperties. Maybe this should be PLATFORM(COCOA)

> Source/WebKit/Shared/WebProcessCreationParameters.h:33
> +#include <WebCore/ScreenProperties.h>

If you do this, GTK and WinCairo need stub implementations of everything. They probably *should* adopt this stuff, so I do think this is the right change, but you likely need to add stubs for those platforms,
Comment 7 Per Arne Vollan 2020-03-25 15:50:22 PDT
Created attachment 394554 [details]
Patch
Comment 8 Per Arne Vollan 2020-03-25 16:12:03 PDT
Created attachment 394559 [details]
Patch
Comment 9 Per Arne Vollan 2020-03-26 07:08:14 PDT
Created attachment 394603 [details]
Patch
Comment 10 Per Arne Vollan 2020-03-26 08:16:27 PDT
Created attachment 394610 [details]
Patch
Comment 11 Per Arne Vollan 2020-03-26 08:32:45 PDT
(In reply to Brent Fulgham from comment #6)
> Comment on attachment 355785 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355785&action=review
> 
> > Source/WebCore/platform/ScreenProperties.h:40
> > +#if PLATFORM(COCOA)
> 
> Do we have any build that was PLATFORM(MAC) previously, but not
> PLATFORM(COCOA)? I don’t think this macro is needed for anything building
> with this file, unless someone is includIn ScreeenProperties
> unconditionally. Or, just make the file PLATFORM(COCOA).
> 
> Maybe that’s why WinCairo is failing to build?
>

I believe you are right, I have made this patch COCOA specific.

> > Source/WebKit/Shared/WebProcessCreationParameters.cpp:161
> >      encoder << screenProperties;
> 
> If this is now unconditional, it seems like WinCairo and GTK need to
> implement ScreenProperties. Maybe this should be PLATFORM(COCOA)
> 

Fixed.

> > Source/WebKit/Shared/WebProcessCreationParameters.h:33
> > +#include <WebCore/ScreenProperties.h>
> 
> If you do this, GTK and WinCairo need stub implementations of everything.
> They probably *should* adopt this stuff, so I do think this is the right
> change, but you likely need to add stubs for those platforms,


Thanks for reviewing!
Comment 12 Radar WebKit Bug Importer 2020-03-26 09:34:48 PDT
<rdar://problem/60922442>
Comment 13 Brent Fulgham 2020-03-26 09:37:28 PDT
Comment on attachment 394610 [details]
Patch

r=me
Comment 14 Per Arne Vollan 2020-03-26 09:52:01 PDT
Comment on attachment 394610 [details]
Patch

Thanks for reviewing!
Comment 15 EWS 2020-03-26 10:04:20 PDT
Committed r259049: <https://trac.webkit.org/changeset/259049>

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