[webkit-reviews] review granted: [Bug 137164] Move PageOverlay[Controller] to WebCore : [Attachment 239096] add some null checks

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has granted Tim Horton
<thorton at apple.com>'s request for review:
Bug 137164: Move PageOverlay[Controller] to WebCore
https://bugs.webkit.org/show_bug.cgi?id=137164

Attachment 239096: add some null checks
https://bugs.webkit.org/attachment.cgi?id=239096&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=239096&action=review


> Source/WebCore/page/PageOverlay.cpp:36
> +#include <wtf/CurrentTime.h>

Should use <chrono>, but you don't have to do it as part of this patch.

> Source/WebCore/page/PageOverlay.cpp:40
> +static const double fadeAnimationDuration = 0.2;

This should be std::chrono::milliseconds (but you don't have to do it as part
of this patch).

> Source/WebCore/page/PageOverlayController.cpp:310
> +    if (!m_pageOverlays.size())

if (m_pageOverlays.isEmpty())

> Source/WebCore/page/PageOverlayController.cpp:311
> +	   return Vector<String>();

Can just be

return { };

> Source/WebCore/page/PageOverlayController.cpp:319
> +    return Vector<String>();

Can just be

return { };

> Source/WebCore/page/PageOverlayController.cpp:324
> +    for (auto it = m_overlayGraphicsLayers.begin(), end =
m_overlayGraphicsLayers.end(); it != end; ++it) {

Modern for loop.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:648
> +    return layerCount > (rootLayer.isComposited() ? 1 : 0);

This reads a little weird, but OK.

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:929
> +    // FIXME: If we want view-relative page overlays in WebKit1, this would
be the place to hook them up.

Legacy WebKit.

> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:757
> +    // FIXME: If we want view-relative page overlays in WebKit1 on Windows,
this would be the place to hook them up.

Legacy WebKit.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.cpp:139
> +	   WKRetainPtr<WKTypeRef> wkType =
m_accessibilityClient.client().copyAccessibilityAttributeValue(toAPI(&pageOverl
ay), toCopiedAPI(attribute), WKPointCreate(WKPointMake(parameter.x(),
parameter.y())), m_accessibilityClient.client().base.clientInfo);

auto wkType =

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.cpp:150
> +	   WKRetainPtr<WKTypeRef> wkType =
m_accessibilityClient.client().copyAccessibilityAttributeValue(toAPI(&pageOverl
ay), toCopiedAPI(attribute), WKPointCreate(WKPointMake(parameter.x(),
parameter.y())), m_accessibilityClient.client().base.clientInfo);

auto wkType =

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.cpp:162
> +	   WKRetainPtr<WKArrayRef> wkNames =
m_accessibilityClient.client().copyAccessibilityAttributeNames(toAPI(&pageOverl
ay), paramerizedNames, m_accessibilityClient.client().base.clientInfo);

auto wkNames =

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.cpp:197
> -   
static_cast<PageOverlayClientImpl*>(toImpl(bundlePageOverlayRef)->client())->se
tAccessibilityClient(client);
> +   
static_cast<PageOverlayClientImpl*>(&toImpl(bundlePageOverlayRef)->client())->s
etAccessibilityClient(client);

Can static_cast to PageOverlayClientImpl& here instead.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:844
> +    if (auto drawingArea = m_page->drawingArea())
> +	   return drawingArea->graphicsLayerFactory();

Not liking that drawingArea can return null...