[webkit-reviews] review granted: [Bug 87301] [chromium] WebLayerTreeViewImpl should not hide methods in CCLayerTreeHost with signatures that match the Client interface : [Attachment 143659] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Dana Jansens
<danakj at chromium.org>'s request for review:
Bug 87301: [chromium] WebLayerTreeViewImpl should not hide methods in
CCLayerTreeHost with signatures that match the Client interface
https://bugs.webkit.org/show_bug.cgi?id=87301

Attachment 143659: Patch
https://bugs.webkit.org/attachment.cgi?id=143659&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=143659&action=review


R=me

We chatted offline about this.	The problem today the call sequence looks like:


WebViewImpl -> WebLayerTreeView::updateAnimations() ->
WebLayerTreeViewImpl::updateAnimations() ->
WebLayerTreeViewClient::updateAnimations() -> WebViewImpl::updateAnimations()

essentially it's short-circuiting in WLTVI.  This is a problem because
CCLayerTreeHost::updateAnimations isn't called, so CCLTH::m_animating is not
set and CCLTH::animateLayers() is not called.  CCLTH::animateLayers() updates
the main-thread animated state of layers, which isn't what is drawn (that's the
impl-thread animated state of layers) but is what determines painting /
uploading.

With this patch the call sequence becomes:

WebViewImpl -> WebLayerTreeView::updateAnimations() ->
WebLayerTreeViewImpl::updateAnimations() -> CCLayerTreeHost::updateAnimations()
-> CCLayerTreeHostClient::updateAnimations() ->
WebLayerTreeViewImpl::ClientConverter::updateAnimations() ->
WebLayerTreeViewClient::updateAnimations() -> WebViewImpl::updateAnimations()

for the win!

>>> Source/WebKit/chromium/src/WebLayerTreeViewImpl.h:46
>>> +	 class ClientConverter : public WebCore::CCLayerTreeHostClient {
>> 
>> does this have to be in the header (could it be in the cpp)?
> 
> I think the WebLayerTreeView needs to hold an instance of the class, so that
it can hold the WebLayerTreeViewClient*. So I think it needs to be in the
header?

WebLayerTreeViewImpl could just have an OwnPtr<ClientConverter> to not need to
see the definition, but it's a minor thing.

Our typical naming for this sort of type is "...Adapter"