[webkit-reviews] review granted: [Bug 193165] Factor legacy WK1 code for fixed and scrolling layers into their own helper class : [Attachment 358403] Patch

[bugzilla-daemon at webkit.org]

Frédéric Wang (:fredw) <fred.wang at free.fr> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 193165: Factor legacy WK1 code for fixed and scrolling layers into their
own helper class
https://bugs.webkit.org/show_bug.cgi?id=193165

Attachment 358403: Patch

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




--- Comment #7 from Frédéric Wang (:fredw) <fred.wang at free.fr> ---
Comment on attachment 358403
  --> https://bugs.webkit.org/attachment.cgi?id=358403
Patch

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

> Source/WebCore/page/ChromeClient.h:265
> +    virtual void updateViewportConstrainedLayers(HashMap<PlatformLayer*,
std::unique_ptr<ViewportConstraints>>&, const HashMap<PlatformLayer*,
PlatformLayer*>&) { }

It seems this is causing a style error but it was probably already present
before this change.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:280
> +	   m_legacyScrollingLayerCoordinator =
std::make_unique<LegacyWebKitScrollingLayerCoordinator>(page().chrome().client(
), m_renderView.frameView().frame().isMainFrame());

It seems you can just use isMainFrameCompositor(). The old code for
registerAllViewportConstrainedLayers/unregisterAllViewportConstrainedLayers
also used to do an early return when scrollingCoordinator() is non-null, can
you please explain in the ChangeLog why it is safe to remove it? I guess this
because it is always null on WK1 (i.e. when
m_renderView.frameView().platformWidget() is non-null)?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3994
> +    ASSERT(m_renderView.document().pageCacheState() ==
Document::NotInPageCache);

We used to do an early return and now just ASSERT. Can you please explain this
in the ChangeLog too?

> Source/WebCore/rendering/RenderLayerCompositor.h:118
> +    bool m_coordinateViewportConstainedLayers;

It looks like that member can be const.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:70
> +//	 ASSERT(m_creationThread.ptr() == &Thread::current());

It seems this change was not intentional.