Move PageOverlay[Controller] to WebCore

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Tim Horton]

It's useful for both WebKits, and fairly Kit-agnostic at this point.

[Tim Horton]

WIP patch; needs a changelog and to test iOS and accessibility (I can't figure out how to use that part even before my patch).

[Tim Horton]

Created attachment 238773 [details]
patch

[Tim Horton]

Created attachment 238807 [details]
wip

iOS is good, now just need to test accessibility + changelog it

[Tim Horton]

Created attachment 238809 [details]
cmake

[Darin Adler]

Comment on attachment 238773 [details]
patch

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

Started reviewing this because I misunderstood the mail about the review flag being cleared. A few stray comments.

> Source/WebCore/loader/EmptyClients.h:185
> +    virtual void attachViewOverlayGraphicsLayer(Frame*, GraphicsLayer*) override { }

Please consider making these references instead of pointers if either is guaranteed to be non-null, even if that means not matching the older function above.

> Source/WebCore/page/Page.h:598
> +    std::unique_ptr<PageOverlayController> m_pageOverlayController;

You could consider putting this on MainFrame instead of Page. That makes it easier to get to with fewer null checks needed.

> Source/WebCore/page/PageOverlay.h:62
> +        virtual void pageOverlayDestroyed(PageOverlay*) = 0;
> +        virtual void willMoveToPage(PageOverlay*, Page*) = 0;
> +        virtual void didMoveToPage(PageOverlay*, Page*) = 0;
> +        virtual void drawRect(PageOverlay*, GraphicsContext&, const IntRect& dirtyRect) = 0;
> +        virtual bool mouseEvent(PageOverlay*, const PlatformMouseEvent&) = 0;
> +        virtual void didScrollFrame(PageOverlay*, Frame&) { }
> +
> +        virtual bool copyAccessibilityAttributeStringValueForPoint(PageOverlay*, String /* attribute */, FloatPoint /* parameter */, String& /* value */) { return false; }
> +        virtual bool copyAccessibilityAttributeBoolValueForPoint(PageOverlay*, String /* attribute */, FloatPoint /* parameter */, bool& /* value */)  { return false; }
> +        virtual Vector<String> copyAccessibilityAttributeNames(PageOverlay*, bool /* parameterizedNames */)  { return Vector<String>(); }

Should consider using references rather than pointers if any of these are things that can’t be null.

> Source/WebCore/page/PageOverlay.h:70
> +    static PassRefPtr<PageOverlay> create(Client*, OverlayType = OverlayType::View);

Should consider a reference rather than a pointer if Client can’t be null.

> Source/WebCore/page/PageOverlay.h:91
> +    Client* client() const { return m_client; }

Should consider a reference rather than a pointer if client can’t be null.

> Source/WebCore/page/PageOverlay.h:104
> +    GraphicsLayer* layer();

Should consider a reference rather than a pointer if layer can’t be null.

> Source/WebCore/page/PageOverlay.h:107
> +protected:
> +    explicit PageOverlay(Client*, OverlayType);

Why protected instead of private? Is there going to be a class derived from this one?

> Source/WebCore/page/PageOverlay.h:114
> +    Page* m_page;

Should be a reference rather than a pointer.

[Tim Horton]

(In reply to comment #5)
> (From update of attachment 238773 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238773&action=review
> 
> Started reviewing this because I misunderstood the mail about the review flag being cleared. A few stray comments.

It was mostly ready for review anyway, so this all works out!

> > Source/WebCore/loader/EmptyClients.h:185
> > +    virtual void attachViewOverlayGraphicsLayer(Frame*, GraphicsLayer*) override { }
> 
> Please consider making these references instead of pointers if either is guaranteed to be non-null, even if that means not matching the older function above.

I will make more things (this and the others) references, absolutely :)

> > Source/WebCore/page/Page.h:598
> > +    std::unique_ptr<PageOverlayController> m_pageOverlayController;
> 
> You could consider putting this on MainFrame instead of Page. That makes it easier to get to with fewer null checks needed.

That seems like a reasonable idea!

> > Source/WebCore/page/PageOverlay.h:107
> > +protected:
> > +    explicit PageOverlay(Client*, OverlayType);
> 
> Why protected instead of private? Is there going to be a class derived from this one?

I think not, in the end.

> > Source/WebCore/page/PageOverlay.h:114
> > +    Page* m_page;
> 
> Should be a reference rather than a pointer.

An overlay can exist without a page (and can move between pages), so I'm not sure about that one.

[Alexey Proskuryakov]

Looks like this patch breaks Mac WK2.

[Tim Horton]

Created attachment 238902 [details]
wip

[Tim Horton]

Created attachment 238946 [details]
windows

[Tim Horton]

Created attachment 238948 [details]
patch

[Tim Horton]

Created attachment 238971 [details]
windows

[Tim Horton]

GTK and EFL failures are only in WebKit2.

[Radar WebKit Bug Importer]

<rdar://problem/18508258>

[Tim Horton]

Created attachment 238985 [details]
patch

[Tim Horton]

Darin's comment about moving PageOverlayController to MainFrame is still not done, I need to poke around a bit.

[Tim Horton]

Created attachment 239042 [details]
on mainframe instead of page

[Tim Horton]

Created attachment 239045 [details]
fix iOS build

[Tim Horton]

Created attachment 239096 [details]
add some null checks

Found some issues while writing tests, adjusting the patch slightly to account for them (PageOverlayController::installPageOverlay, ::deviceScaleFactor, and ::notifyFlushRequired). Updated changelog as well.

[Anders Carlsson]

Comment on attachment 239096 [details]
add some null checks

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(&pageOverlay), 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(&pageOverlay), 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(&pageOverlay), paramerizedNames, m_accessibilityClient.client().base.clientInfo);

auto wkNames =

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePageOverlay.cpp:197
> -    static_cast<PageOverlayClientImpl*>(toImpl(bundlePageOverlayRef)->client())->setAccessibilityClient(client);
> +    static_cast<PageOverlayClientImpl*>(&toImpl(bundlePageOverlayRef)->client())->setAccessibilityClient(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...

[Tim Horton]

http://trac.webkit.org/changeset/174231

[Carlos Alberto Lopez Perez]

(In reply to comment #20)
> http://trac.webkit.org/changeset/174231

This broke both the GTK and EFL ports :\

[Tim Horton]

(In reply to comment #21)
> (In reply to comment #20)
> > http://trac.webkit.org/changeset/174231
> 
> This broke both the GTK and EFL ports :\

Hopefully only WebKit2, though? I thought I got the WebCore part building.