DevTools does not highlight elements when accelerated compositing is on.
Created attachment 96124 [details] Patch
Comment on attachment 96124 [details] Patch This should not be conflated with the CCHeadsUpDisplay, that's for debugging.
Comment on attachment 96124 [details] Patch For compositor debugging, I should say. The inspector is for debugging as well, but of a rather different source. WebKit2 makes a GraphicsLayer for the PageOverlay and inserts it into the compositing tree at the appropriate place. That seems like a more sound approach, could you try that? Look at how LayerTreeHostCA handles it.
Comment on attachment 96124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96124&action=review > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:90 > + hudSize.setWidth(m_layerRenderer->viewportSize().width()); still need to clamp to the max texture size . An example can be found in DrawingBuffer::reset() (DrawingBuffer.cpp) > Source/WebKit/chromium/src/PageOverlay.cpp:84 > + if (!m_viewImpl->isAcceleratedCompositingActive()) { isAcceleratedCompositingActive() needs to be protected by USE(ACCELERATED_COMPOSITING) guards. > Source/WebKit/chromium/src/PageOverlay.cpp:105 > +void PageOverlay::webFramePainted(GraphicsContext& gc) The method should probably be named using an active verb. paintWebFrame ?
Comment on attachment 96124 [details] Patch resetting review? flag to review-
(In reply to comment #3) > (From update of attachment 96124 [details]) > For compositor debugging, I should say. The inspector is for debugging as well, but of a rather different source. > > WebKit2 makes a GraphicsLayer for the PageOverlay and inserts it into the compositing tree at the appropriate place. That seems like a more sound approach, could you try that? Look at how LayerTreeHostCA handles it. Good suggestion. It sounds like it would require some additional plumbing the the creation of a new GraphicsLayer type for overlay content that has the appropriate Painter.
It shouldn't take much - just provide an implementation of GraphicsLayerClient and the compositor can treat it like a normal content layer.
Created attachment 96253 [details] Patch with fixes
I have uploaded a new patch with the GraphicsLayer approach. Since there is no way to get GraphicsLayer from LayerChromium, I changed ChromeClientImpl to pass root GraphicsLayer to WebViewImpl, where I save it and use for overlay manipulations. (In reply to comment #4) > (From update of attachment 96124 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96124&action=review > > > Source/WebKit/chromium/src/PageOverlay.cpp:84 > > + if (!m_viewImpl->isAcceleratedCompositingActive()) { > > isAcceleratedCompositingActive() needs to be protected by USE(ACCELERATED_COMPOSITING) guards. I don't think it needs that, I made it the same way I saw in WebViewImpl, (e.g. WebViewImpl::resize) > > Source/WebKit/chromium/src/PageOverlay.cpp:105 > > +void PageOverlay::webFramePainted(GraphicsContext& gc) > > The method should probably be named using an active verb. paintWebFrame ? Done.
Comment on attachment 96253 [details] Patch with fixes View in context: https://bugs.webkit.org/attachment.cgi?id=96253&action=review Looks good overall to me (minus the one comment). James can you please have a look too? > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:286 > + m_webViewImpl->setPageOverlayClient(this); highlight seems like a method that will get called often. setPageOverlayClient() seems to blindly create a new PageOverlay instance. Isn't that excessive?
Comment on attachment 96253 [details] Patch with fixes View in context: https://bugs.webkit.org/attachment.cgi?id=96253&action=review This is looking great overall. My main concern is the same as Vangelis' - it seems that this code will create a new PageOverlay on every call to highlightNode(), which is super frequent. Can you look into that? > Source/WebKit/chromium/src/PageOverlay.cpp:57 > + update(); is this call just to run the code at lines 129-132? can you just move that cleanup code directly to here? i realize that probably means adding an #if USE(ACCELERATED_COMPOSITING) block here but that feels less weird than calling a member function inside a destructor > Source/WebKit/chromium/src/PageOverlay.h:60 > + PageOverlay(WebViewImpl*, PageOverlayClient*); > + WebViewImpl* m_viewImpl; super-nitpicky nitpick: would prefer a newline between the last member function and the first member variable, just to make this a bit easier to scan
Created attachment 96815 [details] patch with fixes
(In reply to comment #11) > (From update of attachment 96253 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96253&action=review > > This is looking great overall. My main concern is the same as Vangelis' - it seems that this code will create a new PageOverlay on every call to highlightNode(), which is super frequent. Can you look into that? I am only setting PageOverlayClient now, if m_pageOverlay object is not null. > > Source/WebKit/chromium/src/PageOverlay.cpp:57 > > + update(); > > is this call just to run the code at lines 129-132? can you just move that cleanup code directly to here? i realize that probably means adding an #if USE(ACCELERATED_COMPOSITING) block here but that feels less weird than calling a member function inside a destructor As I said on IRC, WebFrame invalidation is also needed for non-composited case. I have added a clear() method to handle highlighting hiding. > > Source/WebKit/chromium/src/PageOverlay.h:60 > > + PageOverlay(WebViewImpl*, PageOverlayClient*); > > + WebViewImpl* m_viewImpl; > > super-nitpicky nitpick: would prefer a newline between the last member function and the first member variable, just to make this a bit easier to scan Done.
Comment on attachment 96815 [details] patch with fixes Great, R=me. Thanks for doing this!
Comment on attachment 96815 [details] patch with fixes Clearing flags on attachment: 96815 Committed r88582: <http://trac.webkit.org/changeset/88582>
All reviewed patches have been landed. Closing bug.