[webkit-reviews] review denied: [Bug 187684] IO surfaces and 3D graphics contexts should update the display mask when the window is moved to another screen. : [Attachment 345077] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Per Arne Vollan <pvollan at apple.com>'s
request for review:
Bug 187684: IO surfaces and 3D graphics contexts should update the display mask
when the window is moved to another screen.
https://bugs.webkit.org/show_bug.cgi?id=187684

Attachment 345077: Patch

https://bugs.webkit.org/attachment.cgi?id=345077&action=review




--- Comment #11 from Dean Jackson <dino at apple.com> ---
Comment on attachment 345077
  --> https://bugs.webkit.org/attachment.cgi?id=345077
Patch

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

>> Source/WebCore/page/WindowObserver.h:31
>> +class WindowObserver {
> 
> I think this needs a different name. "Window" has multiple meanings in
WebCore already (DOMWindow, the JS "window" object etc). Maybe this should be
HostWindowObserver or something.

Agreed.

> Source/WebCore/page/WindowObserver.h:34
> +    virtual void windowScreenDidChange(uint32_t) { };

Shouldn't this be just screenDidChange? It's probably also worth naming the
parameter.

> Source/WebCore/platform/HostWindow.h:82
> +    virtual void registerWindowObserver(WindowObserver&) = 0;
> +    virtual void unregisterWindowObserver(WindowObserver&) = 0;

registerObserver() ?

> Source/WebCore/platform/graphics/ImageBuffer.h:69
> -    WEBCORE_EXPORT static std::unique_ptr<ImageBuffer> create(const
FloatSize&, RenderingMode, float resolutionScale = 1, ColorSpace =
ColorSpaceSRGB, const HostWindow* = nullptr);
> +    WEBCORE_EXPORT static std::unique_ptr<ImageBuffer> create(const
FloatSize&, RenderingMode, float resolutionScale = 1, ColorSpace =
ColorSpaceSRGB, HostWindow* = nullptr);

Why is this necessary?

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:475
> +#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> +void IOSurface::windowScreenDidChange(unsigned displayID)
> +{
> +    ASSERT(m_cgContext);
> +    if (!m_cgContext)
> +	   return;
> +    CGIOSurfaceContextSetDisplayMask(m_cgContext.get(),
displayMaskForDisplay(displayID));
> +}
> +#endif

It seems it would be better to have a single observer and have it talk to all
the IOSurface objects, rather than an observer for each IOSurface.