[Qt] Register individual WebGraphicsLayer to LayerTreeHost instead of handling the tree as a whole.
Created attachment 125842 [details] Patch
Comment on attachment 125842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125842&action=review General direction is good, have some nitpicks. > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h:48 > +class WebGraphicsLayerRemoteClient { Not sure about the name... Maybe just WebGraphicsLayerClient? The implementation is remote, but the interface doesn't have any trace of remote-ness. > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h:60 > + virtual void didDeleteLayer(WebCore::WebGraphicsLayer*) = 0; Maybe we should rename this to unregister, since we call it outside of deletion. > Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:245 > + m_registeredLayers.remove(m_registeredLayers.find(layer)); Expensive. Maybe this should be a HashSet instead of a vector? > Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:428 > + for (int i = 0; i < m_registeredLayers.size(); ++i) > + m_registeredLayers[i]->setVisibleContentRectTrajectoryVector(trajectoryVector); This is a change in behavior that belongs in a different patch. We shouldn't calculate trajectory for all layers, because the trajectory has to be transformed. > Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.h:119 > + float m_contentsScale; Shouldn't this new member be initialized?
Created attachment 125873 [details] Patch Addressed comments, renamed didDeleteLayer to releaseLayer since it doesn't just unregister it.
Created attachment 125881 [details] Patch Removed the delegation to child layers of setVisibleContentsRectAndScale.
Comment on attachment 125881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125881&action=review > Source/WebKit2/ChangeLog:8 > + The LayerTreeHost association had to be maintained between re-paranted layers and it would be parented* > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:165 > - toWebGraphicsLayer(newChild)->setVisibleContentRectAndScale(m_pageVisibleRect, m_contentsScale); > + toWebGraphicsLayer(newChild)->setWebGraphicsLayerClient(m_webGraphicsLayerClient); Maybe it should just be called setGraphicsLayerClient? > Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h:60 > + virtual void registerLayer(WebCore::WebGraphicsLayer*) = 0; > + virtual void releaseLayer(WebCore::WebGraphicsLayer*) = 0; This is a bit confusing, register vs release. Add a comment ?
Comment on attachment 125881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125881&action=review Let's fix the naming nitpicks and Kenneth's other comments; otherwise we're good to go. >> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:165 >> + toWebGraphicsLayer(newChild)->setWebGraphicsLayerClient(m_webGraphicsLayerClient); > > Maybe it should just be called setGraphicsLayerClient? I don't think so... there's already a GraphicsLayerClient class. Maybe there's a better name for this? I just didn't like "remote" in there because it made the code confusing to read. >> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h:60 >> + virtual void releaseLayer(WebCore::WebGraphicsLayer*) = 0; > > This is a bit confusing, register vs release. Add a comment ? Maybe attachLayer / detachLayer? > Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp:61 > + for (HashSet<WebCore::WebGraphicsLayer*>::iterator iter = registeredLayers.begin(); iter != end; ++iter) > + (*iter)->setWebGraphicsLayerClient(0); iter -> it
> >> Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp:165 > >> + toWebGraphicsLayer(newChild)->setWebGraphicsLayerClient(m_webGraphicsLayerClient); > > > > Maybe it should just be called setGraphicsLayerClient? > > I don't think so... there's already a GraphicsLayerClient class. > Maybe there's a better name for this? I just didn't like "remote" in there because it made the code confusing to read. Yeah and proxy would just make it confusing as well. It is probably ok, and we can fix the names later.. maybe discuss it at the code camp
Created attachment 126059 [details] Patch - register/release -> attach/detach - iter -> it
Committed r107233: <http://trac.webkit.org/changeset/107233>