Bug 137164 - Move PageOverlay[Controller] to WebCore
Summary: Move PageOverlay[Controller] to WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on: 137384 137443 137848
Blocks:
  Show dependency treegraph
 
Reported: 2014-09-26 18:35 PDT by Tim Horton
Modified: 2014-10-21 07:41 PDT (History)
21 users (show)

See Also:


Attachments
patch (155.40 KB, patch)
2014-09-27 04:25 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
wip (159.36 KB, patch)
2014-09-28 00:26 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
cmake (160.69 KB, patch)
2014-09-28 00:34 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
wip (165.35 KB, patch)
2014-09-29 19:28 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
windows (167.70 KB, patch)
2014-09-30 12:24 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
patch (167.70 KB, patch)
2014-09-30 12:54 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
windows (169.18 KB, patch)
2014-09-30 15:30 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
patch (184.05 KB, patch)
2014-09-30 18:19 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
on mainframe instead of page (188.74 KB, patch)
2014-10-01 12:49 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
fix iOS build (188.99 KB, patch)
2014-10-01 13:46 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
add some null checks (189.73 KB, patch)
2014-10-02 02:31 PDT, Tim Horton
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2014-09-26 18:35:03 PDT
It's useful for both WebKits, and fairly Kit-agnostic at this point.
Comment 1 Tim Horton 2014-09-27 04:25:02 PDT
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).
Comment 2 Tim Horton 2014-09-27 04:25:18 PDT
Created attachment 238773 [details]
patch
Comment 3 Tim Horton 2014-09-28 00:26:42 PDT
Created attachment 238807 [details]
wip

iOS is good, now just need to test accessibility + changelog it
Comment 4 Tim Horton 2014-09-28 00:34:12 PDT
Created attachment 238809 [details]
cmake
Comment 5 Darin Adler 2014-09-28 17:22:26 PDT
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.
Comment 6 Tim Horton 2014-09-28 18:32:15 PDT
(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.
Comment 7 Alexey Proskuryakov 2014-09-29 12:18:45 PDT
Looks like this patch breaks Mac WK2.
Comment 8 Tim Horton 2014-09-29 19:28:59 PDT
Created attachment 238902 [details]
wip
Comment 9 Tim Horton 2014-09-30 12:24:35 PDT
Created attachment 238946 [details]
windows
Comment 10 Tim Horton 2014-09-30 12:54:44 PDT
Created attachment 238948 [details]
patch
Comment 11 Tim Horton 2014-09-30 15:30:17 PDT
Created attachment 238971 [details]
windows
Comment 12 Tim Horton 2014-09-30 16:17:31 PDT
GTK and EFL failures are only in WebKit2.
Comment 13 Radar WebKit Bug Importer 2014-09-30 16:17:31 PDT
<rdar://problem/18508258>
Comment 14 Tim Horton 2014-09-30 18:19:01 PDT
Created attachment 238985 [details]
patch
Comment 15 Tim Horton 2014-09-30 18:26:24 PDT
Darin's comment about moving PageOverlayController to MainFrame is still not done, I need to poke around a bit.
Comment 16 Tim Horton 2014-10-01 12:49:48 PDT
Created attachment 239042 [details]
on mainframe instead of page
Comment 17 Tim Horton 2014-10-01 13:46:48 PDT
Created attachment 239045 [details]
fix iOS build
Comment 18 Tim Horton 2014-10-02 02:31:00 PDT
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.
Comment 19 Anders Carlsson 2014-10-02 12:15:56 PDT
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...
Comment 20 Tim Horton 2014-10-02 14:08:27 PDT
http://trac.webkit.org/changeset/174231
Comment 21 Carlos Alberto Lopez Perez 2014-10-02 20:07:02 PDT
(In reply to comment #20)
> http://trac.webkit.org/changeset/174231

This broke both the GTK and EFL ports :\
Comment 22 Tim Horton 2014-10-02 20:18:00 PDT
(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.