[webkit-reviews] review granted: [Bug 195933] Selected element on Web Inspector is not highlighted with CPU Rendering. : [Attachment 397825] patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Tomoki Imai
<tomoki.imai at sony.com>'s request for review:
Bug 195933: Selected element on Web Inspector is not highlighted with CPU
Rendering.
https://bugs.webkit.org/show_bug.cgi?id=195933

Attachment 397825: patch

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




--- Comment #7 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 397825
  --> https://bugs.webkit.org/attachment.cgi?id=397825
patch

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

r=me, with a few comments

Should there also be a FIXME (with bug link) inside
`WebInspectorClient::showPaintRect` as well?

> Source/WebCore/inspector/InspectorController.h:95
> +    WEBCORE_EXPORT bool shouldShowOverlay() const;

NIT: I'd put this above `drawHighlight` as one should be calling
`shouldShowOverlay` before they call `drawHighlight`

> Source/WebKit/ChangeLog:10
> +	   WebInspectorClient::highlight and WebInspectorClient::hideHighlight
currently requests re-paint whole web page, but it should be able to optimize
by only updating dirty rects in the future.

FYI, `InspectorOverlay` expects to be a `PageOverlay::OverlayType::View`,
meaning that it is outside/above the scrolling view.  The reason for this is
that Web Inspector draws lots of fixed UI in the overlay, such as the
highlighted node information, page rulers, and page size.  It is probably
possible for us to rework the overlay to be a
`PageOverlay::OverlayType::Document`, but that would likely require a lot more
logic to ensure that those fixed UI don't move when scrolling.	I'm not sure
how to test this change on macOS as (I've just been informed that) we require
accelerated compositing in order to render, so I would suggest that you test
this change with scrolling and all of the various overlay settings
<https://webkit.org/web-inspector/page-overlay/#overlay-settings>.

> Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp:115
> +	   m_page->drawingArea()->setNeedsDisplay();

This should probably be wrapped in a `#if PLATFORM(GTK) || PLATFORM(WIN)`.

> Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp:139
> +	   m_page->drawingArea()->setNeedsDisplay();

Ditto (115)

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1814
> +    if (!m_page->settings().acceleratedCompositingEnabled() &&
m_page->inspectorController().enabled() &&
m_page->inspectorController().shouldShowOverlay()) {

Ditto (WebInspectorClient.cpp:115)