Bug 137164

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).

Created attachment 238773 [details] patch

Created attachment 238807 [details] wip iOS is good, now just need to test accessibility + changelog it

Created attachment 238809 [details] cmake

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.

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

Looks like this patch breaks Mac WK2.

Created attachment 238902 [details] wip

Created attachment 238946 [details] windows

Created attachment 238948 [details] patch

Created attachment 238971 [details] windows

GTK and EFL failures are only in WebKit2.

<rdar://problem/18508258>

Created attachment 238985 [details] patch

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

Created attachment 239042 [details] on mainframe instead of page

Created attachment 239045 [details] fix iOS build

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 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...

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

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