Bug 195933 – Selected element on Web Inspector is not highlighted with CPU Rendering.

• Component: Web Inspector
• Status: RESOLVED FIXED

[Yousuke Kimoto]

On WebInspector's element view, users can see any element which is selected on the element view as a highlighted part on an inspected WebView.
But the highlighting effect doesn't work with CPU rendering on WinCario.

[Tomoki Imai]

Created attachment 397436 [details]
patch

Patch to enable highlighting elements in non accelerated compositing mode.

There is still a room to optimize the performance by using the dirty rects, eliminating the transparency layer when it's not needed.
But we'd like to request the review because we're not confident of the current design and whether it works on Mac port.

[Tomoki Imai]

Note:
I tested attachment 397436 [details] with 

- WinCairo MiniBrowser, which doesn't have ac implementation
- GTK MiniBrowser with "Hardware Acceleration Policy: never"
- GTK MiniBrowser with "Hardware Acceleration Policy: always"

[Devin Rousso]

Comment on attachment 397436 [details]
patch

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

> Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp:114
> +    if (!m_page->corePage()->settings().acceleratedCompositingEnabled()) {

Is this check still needed?  As far as I understood, we only needed this check before because the `InspectorOverlay` used to use an actual `WebCore::Page`, which had a `<canvas>`, and therefore could create additional layers (see r211141 from bug 165237).  Ever since r242019 (bug 105023), however, we use `GraphicsContext` functions directly, eliminating the `WebCore::Page` and anything that came with it.  As such, I wonder if GTK+ is able to handle this now?  If not, assuming that the `InspectorOverlay` works on PlayStation without accelerated compositing, perhaps we should just special case GTK+ here instead?

Also, what about any other accelerated compositing checks/paths (e.g. `WebInspectorClient::showPaintRect`)?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1814
> +    // FIXME: Is there any better way to see if we will draw highlight?

I'd look at `InspectorOverlay::shouldShowOverlay` which should do what you're asking for.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1816
> +    if (m_page->inspectorController().enabled()) {

Shouldn't this only be called if `!m_page->corePage()->settings().acceleratedCompositingEnabled()` too?

[Tomoki Imai]

(In reply to Devin Rousso from comment #3)
> Comment on attachment 397436 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397436&action=review
> 
> > Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp:114
> > +    if (!m_page->corePage()->settings().acceleratedCompositingEnabled()) {
> 
> Is this check still needed?  As far as I understood, we only needed this
> check before because the `InspectorOverlay` used to use an actual
> `WebCore::Page`, which had a `<canvas>`, and therefore could create
> additional layers (see r211141 from bug 165237).  Ever since r242019 (bug
> 105023), however, we use `GraphicsContext` functions directly, eliminating
> the `WebCore::Page` and anything that came with it.  As such, I wonder if
> GTK+ is able to handle this now?  If not, assuming that the
> `InspectorOverlay` works on PlayStation without accelerated compositing,
> perhaps we should just special case GTK+ here instead?
> 
> Also, what about any other accelerated compositing checks/paths (e.g.
> `WebInspectorClient::showPaintRect`)?
>

We still need them because creating GraphicsLayer, which is used in WebInspectorClient, seems to turn on accelerated compositing mode. (I'll take a look more later how it turns on, and comment)
To support accelerated compositing off in WinCairo/GTK/PlayStation, we'd like to keep !m_page->corePage()->settings().acceleratedCompositingEnabled() check.

> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:1814
> > +    // FIXME: Is there any better way to see if we will draw highlight?
> 
> I'd look at `InspectorOverlay::shouldShowOverlay` which should do what
> you're asking for.
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:1816
> > +    if (m_page->inspectorController().enabled()) {
> 
> Shouldn't this only be called if
> `!m_page->corePage()->settings().acceleratedCompositingEnabled()` too?

Thanks! I'll add these checks.

[Tomoki Imai]

Created attachment 397825 [details]
patch

This patch adds more check to stop creating unneeded transparency layer.

[Simon Fraser (smfr)]

Comment on attachment 397825 [details]
patch

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

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

GTK needs to get on the accelerated compositing bandwagon.

[Devin Rousso]

Comment on attachment 397825 [details]
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)

[Tomoki Imai]

Created attachment 398041 [details]
patch

Updated patch to reflect the comments.

[Tomoki Imai]

(In reply to Devin Rousso from comment #7)
> Comment on attachment 397825 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397825&action=review
> 
> r=me, with a few comments

Thanks for your kind comments!

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

I was not sure what this FIXME comment should describe.
Do we want to put something like following?

// FIXME: GraphicsLayer requires accelerated compositing mode, which is not available for some platform.
// https://bugs.webkit.org/show_bug.cgi?id=195933


> 
> > 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`

I agree, I moved in attachment 398041 [details].

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

Thanks, the ChangeLog comment was misleading, m_page->drawingArea()->setNeedsDisplay() in WebInspectorClient::hideHighlight/highlight re-paints the whole view (not web page).
Win/GTK/PlayStation DrawingAreaCoordinatedGraphics refers WebPage::bounds(), which returns WebPage::m_viewSize.
https://trac.webkit.org/browser/webkit/trunk/Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.cpp?rev=259283#L91
I fixed the ChangeLog and added FIXME comment on  WebInspectorClient::hideHighlight and  WebInspectorClient::highlight.

But I noticed marking the highlight rect dirty might be difficult and not so effective.
For example page ruler is displayed left edge and top edge of the view, which means the dirty rect would be whole view.

And thanks for your advice for testing, as far as I can see on WinCairo, everything seems to work fine.
- scrolling + always show rulers
- scrolling + show rulers during element selection
- scrolling + no rulers

> 
> > 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)

I wrapped with "#if PLATFORM(GTK) || PLATFORM(WIN) || PLATFORM(PLAYSTATION)".
PlayStation also supports non accelerated compositing mode, so we wanted to add it.

[Tomoki Imai]

(In reply to Simon Fraser (smfr) from comment #6)
> Comment on attachment 397825 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397825&action=review
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:1814
> > +    if (!m_page->settings().acceleratedCompositingEnabled() && m_page->inspectorController().enabled() && m_page->inspectorController().shouldShowOverlay()) {
> 
> GTK needs to get on the accelerated compositing bandwagon.

I personally don't know GTK future plan for accelerated compositing, but I thought non accelerated compositing is still supported because MiniBrowser has an option.

[Tomoki Imai]

Created attachment 399251 [details]
patch

Don let me know that minor change doesn't require another r+, so I replaced "Reviewed by NOBODY (OOPS!).".
attachment 397825 [details] got r+ by Devin, but I added platform guard and comments (comment 9)

[Tomoki Imai]

Sorry, I probably needed to use webkit-patch upload, and will try next time.
https://trac.webkit.org/wiki/Creating%20and%20Submitting%20Layout%20Tests%20and%20Patches

[EWS]

Committed r261614: <https://trac.webkit.org/changeset/261614>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399251 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63182514>