Bug 191767 – [iOS] Adopt ScreenProperties class.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Per Arne Vollan]

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.

[Per Arne Vollan]

Created attachment 355109 [details]
Patch

[Brent Fulgham]

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?

[Per Arne Vollan]

Created attachment 355130 [details]
Patch

[Per Arne Vollan]

Created attachment 355775 [details]
Patch

[Per Arne Vollan]

Created attachment 355785 [details]
Patch

[Brent Fulgham]

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,

[Per Arne Vollan]

Created attachment 394554 [details]
Patch

[Per Arne Vollan]

Created attachment 394559 [details]
Patch

[Per Arne Vollan]

Created attachment 394603 [details]
Patch

[Per Arne Vollan]

Created attachment 394610 [details]
Patch

[Per Arne Vollan]

(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!

[Radar WebKit Bug Importer]

<rdar://problem/60922442>

[Brent Fulgham]

Comment on attachment 394610 [details]
Patch

r=me

[Per Arne Vollan]

Comment on attachment 394610 [details]
Patch

Thanks for reviewing!

[EWS]

Committed r259049: <https://trac.webkit.org/changeset/259049>

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