Web Inspector: [Chromium] DevTools does not highlight elements when accelerated compositing is on.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Vsevolod Vlasov]

DevTools does not highlight elements when accelerated compositing is on.

[Vsevolod Vlasov]

Created attachment 96124 [details]
Patch

[James Robinson]

Comment on attachment 96124 [details]
Patch

This should not be conflated with the CCHeadsUpDisplay, that's for debugging.

[James Robinson]

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.

[Vangelis Kokkevis]

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 ?

[James Robinson]

Comment on attachment 96124 [details]
Patch

resetting review? flag to review-

[Vangelis Kokkevis]

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

[James Robinson]

It shouldn't take much - just provide an implementation of GraphicsLayerClient and the compositor can treat it like a normal content layer.

[Vsevolod Vlasov]

Created attachment 96253 [details]
Patch with fixes

[Vsevolod Vlasov]

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.

[Vangelis Kokkevis]

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?

[James Robinson]

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

[Vsevolod Vlasov]

Created attachment 96815 [details]
patch with fixes

[Vsevolod Vlasov]

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

[James Robinson]

Comment on attachment 96815 [details]
patch with fixes

Great, R=me.  Thanks for doing this!

[WebKit Review Bot]

Comment on attachment 96815 [details]
patch with fixes

Clearing flags on attachment: 96815

Committed r88582: <http://trac.webkit.org/changeset/88582>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.