Bug 77555 – [chromium] Add setNeedsRedraw to WebWidget

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nat Duca]

[chromium] Add didReceiveDamage to WebWidget

[Nat Duca]

Created attachment 124949 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Nat Duca]

This method already was required on the WebWidgetUpdateController in my upcoming WebWidget and serves to notify the WebWidget that its surface was damaged. My inversion patch is still a few weeks out, and @backer has requested this call in the near term to enable the scissoring optimization and start linting that out on the canaries.

Thus, I put up this patch as a stopgap measure. Hopefully it will be OK to you as well. Its all going away in a few weeks.

[Darin Fisher (:fishd, Google)]

Comment on attachment 124949 [details]
Patch

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

> Source/WebKit/chromium/public/WebWidget.h:113
> +    virtual void didReceiveDamage() { }

if you look at the implementation of themeChanged, it would appear to be very similar
to this.  it is just a notification that the WebWidget needs to redraw everything.
maybe we could combine these two methods?

also, why isn't this similarly implemented using FrameView::invalidateRect?  or, 
why don't we have special code in the accelerated case for themeChanged?

would it make sense to rename themeChanged to "invalidate()" and then use that
for both purposes?  or does FrameView::invalidateRect() cause more work to be
done than you need in this new case?

[Jonathan Backer]

(In reply to comment #4)
> (From update of attachment 124949 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124949&action=review
> 
> > Source/WebKit/chromium/public/WebWidget.h:113
> > +    virtual void didReceiveDamage() { }
> 
> if you look at the implementation of themeChanged, it would appear to be very similar
> to this.  it is just a notification that the WebWidget needs to redraw everything.
> maybe we could combine these two methods?
> 
> also, why isn't this similarly implemented using FrameView::invalidateRect?  or, 
> why don't we have special code in the accelerated case for themeChanged?
> 
> would it make sense to rename themeChanged to "invalidate()" and then use that
> for both purposes?  or does FrameView::invalidateRect() cause more work to be
> done than you need in this new case?

I think that we need a different pathway for this. Under accelerated compositing, we distinguish between two different actions in the renderer:
- painting: rasterizing the textures (typically via Skia)
- drawing: compositing the textures to screen (via the GPU process)

In the case of didReceiveDamage (perhaps poorly named), we just want to invalidate/damage a region for the drawing cycle --- not the paint cycle, which should remain unaffected (no new textures to update).

WebViewImpl::themeChanged() in particular seems to indicate that we need a new paint and drawing cycle, so it is inappropriate for our purposes.

[Darin Fisher (:fishd, Google)]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 124949 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=124949&action=review
> > 
> > > Source/WebKit/chromium/public/WebWidget.h:113
> > > +    virtual void didReceiveDamage() { }
> > 
> > if you look at the implementation of themeChanged, it would appear to be very similar
> > to this.  it is just a notification that the WebWidget needs to redraw everything.
> > maybe we could combine these two methods?
> > 
> > also, why isn't this similarly implemented using FrameView::invalidateRect?  or, 
> > why don't we have special code in the accelerated case for themeChanged?
> > 
> > would it make sense to rename themeChanged to "invalidate()" and then use that
> > for both purposes?  or does FrameView::invalidateRect() cause more work to be
> > done than you need in this new case?
> 
> I think that we need a different pathway for this. Under accelerated compositing, we distinguish between two different actions in the renderer:
> - painting: rasterizing the textures (typically via Skia)
> - drawing: compositing the textures to screen (via the GPU process)
> 
> In the case of didReceiveDamage (perhaps poorly named), we just want to invalidate/damage a region for the drawing cycle --- not the paint cycle, which should remain unaffected (no new textures to update).
> 
> WebViewImpl::themeChanged() in particular seems to indicate that we need a new paint and drawing cycle, so it is inappropriate for our purposes.

Thanks for the explanation.  setNeedsRedraw seems like a pretty decent choice: matches the lower level function call name and it includes the keyword "redraw".  It would be even better if this method names somehow conveyed the fact that it only applies when we are in compositing mode.

[Jonathan Backer]

Taking over from nduca.

[Jonathan Backer]

Created attachment 125361 [details]
Patch

[Jonathan Backer]

CCDamageTracker goo stolen from https://bugs.webkit.org/show_bug.cgi?id=76805

Confirmed that this fixes the tab switching problem and window expose problem with --enable-partial-swap.

[Shawn Singh]

Comment on attachment 125361 [details]
Patch


Here are my comments.  Please note I did not look carefully at the WebWidget side of things, since I'm not very familiar with that code.
Should I go ahead and mark the original bug as a duplicate?


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

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:136
> +    if (m_forceFullDamageNextUpdate) {
> +        m_currentDamageRect = FloatRect(layer->targetRenderSurface()->contentRect());
> +        m_forceFullDamageNextUpdate = false;
> +        return;
> +    }

We can't return early without computing damage from layers, because the damage tracker needs to update the state of layers in the three lines of code after this.   I suppose you can fix this by putting this immediately after the next 3 lines of code, but after that point, an early return is moot anyway.  Note, the other existing early exit is also wrong - https://bugs.webkit.org/show_bug.cgi?id=76924 - can you please check to make sure this passes that unit test included that patch? (you'll have to apply the full patch, though)

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.h:46
> +    void forceFullDamageNextUpdate() { m_forceFullDamageNextUpdate = true; }

Could we add a unit test that checks if things are fully damaged under the conditions that should trigger the call to forceFullDamageNextUpdate() ?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:585
> +    m_layerTreeHostImpl->rootLayer()->renderSurface()->damageTracker()->forceFullDamageNextUpdate();

Personally in my opinion, there should be a CCLayerTreeHostImpl::forceFullDamageOnRootSurface() wrapper, because that wrapper will be useful in at least one other case that requires full damage from the impl thread itself (such as Nat's first comment on https://bugs.webkit.org/show_bug.cgi?id=76668)

[Nat Duca]

Wait, these are different patches and should be. There should be one patch to add reset capability to damage tracker. There should be a patch for plumbing.

They should not be comingled.

[Jonathan Backer]

Created attachment 125382 [details]
Patch

[Jonathan Backer]

Sorry for merging CLs. That makes it much harder to review.

Updated based on off-line discussions.

[Antoine Labour]

Comment on attachment 125382 [details]
Patch

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

> Source/WebKit/chromium/public/WebWidget.h:113
> +    virtual void didReceiveDamage() { }

Wouldn't we want to pass in the damaged rect? It looks like we have the info on the chrome side, and we can still be conservative on the webkit side and optimize later, without involving 2-way patches.

[Nat Duca]

(In reply to comment #14)
> (From update of attachment 125382 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125382&action=review
> 
> > Source/WebKit/chromium/public/WebWidget.h:113
> > +    virtual void didReceiveDamage() { }
> 
> Wouldn't we want to pass in the damaged rect? It looks like we have the info on the chrome side, and we can still be conservative on the webkit side and optimize later, without involving 2-way patches.

Fine by me.

[Nat Duca]

Comment on attachment 125382 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:261
> +void CCThreadProxy::performSetNeedsRedrawOnImplThread()

Not something to hold an LGTM on, but is there somethign that communicates the role of this function vs the other one more clearly? I know I gave you this name, I'm really sorry.

Some words
- main thread initiated

Or make setNeedsRedrawOnImplThread take a bool
Or maybe post two messages
  resetDamageTrackerOnImplThread()
  setNeedsRedrawOnImplThread

I'm not sure... this is just non-obvious to read.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:266
> +        renderSurface->damageTracker()->forceFullDamageNextUpdate();

As I understand it, this is going to be a method on the renderSurface saying "resetDamageTracker," yes?

> Source/WebKit/chromium/src/WebViewImpl.h:116
> +    virtual void didReceiveDamage();

Darin's feedback was to call this setNeedsRedraw. I think we're converging on setNeedsRedraw()

Wrt Piman's comment, I think I'd prefer that to be another function. The problem otherwise is that the client code starts making assumptions about webwidget size AND you get people abusing this API for all sorts of things. I have to add a per-rect API soon anyway, so I'll take on the burden of the extra API or revising this one.

[Jonathan Backer]

Created attachment 125414 [details]
Patch

[Jonathan Backer]

(In reply to comment #16)
> (From update of attachment 125382 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125382&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:261
> > +void CCThreadProxy::performSetNeedsRedrawOnImplThread()
> 
> Not something to hold an LGTM on, but is there somethign that communicates the role of this function vs the other one more clearly? I know I gave you this name, I'm really sorry.
> 
> Some words
> - main thread initiated
> 
> Or make setNeedsRedrawOnImplThread take a bool
> Or maybe post two messages
>   resetDamageTrackerOnImplThread()
>   setNeedsRedrawOnImplThread

I like. Done.

> 
> I'm not sure... this is just non-obvious to read.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:266
> > +        renderSurface->damageTracker()->forceFullDamageNextUpdate();
> 
> As I understand it, this is going to be a method on the renderSurface saying "resetDamageTracker," yes?

Absolutely. I'll sync with the other patch as it evolves.

> 
> > Source/WebKit/chromium/src/WebViewImpl.h:116
> > +    virtual void didReceiveDamage();
> 
> Darin's feedback was to call this setNeedsRedraw. I think we're converging on setNeedsRedraw()

Done.

> 
> Wrt Piman's comment, I think I'd prefer that to be another function. The problem otherwise is that the client code starts making assumptions about webwidget size AND you get people abusing this API for all sorts of things. I have to add a per-rect API soon anyway, so I'll take on the burden of the extra API or revising this one.

I agree. The WebWidget change is temporary (it will change in the next few weeks). We're introducing it just so that enabling scissoring isn't blocked.

[Darin Fisher (:fishd, Google)]

Comment on attachment 125414 [details]
Patch

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

> Source/WebKit/chromium/public/WebWidget.h:110
> +    // Temporary method for the embedder to notify the WebWidget that the widget

nit: usually a good idea to reference a bug number in such comments.

[Jonathan Backer]

Created attachment 125666 [details]
Patch

[Jonathan Backer]

(In reply to comment #19)
> (From update of attachment 125414 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125414&action=review
> 
> > Source/WebKit/chromium/public/WebWidget.h:110
> > +    // Temporary method for the embedder to notify the WebWidget that the widget
> 
> nit: usually a good idea to reference a bug number in such comments.

Done.

[James Robinson]

Comment on attachment 125666 [details]
Patch

Darin's out for a while, but this looks OK to me for now.

[WebKit Review Bot]

Comment on attachment 125666 [details]
Patch

Clearing flags on attachment: 125666

Committed r106935: <http://trac.webkit.org/changeset/106935>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Gentilcore]

This was rolled out in https://bugs.webkit.org/show_bug.cgi?id=77994 . See there for details.

[Jonathan Backer]

Created attachment 125856 [details]
Patch

[Jonathan Backer]

The CCLayerTreeHostImpl's sometimes do not have rootLayer(). Updated the CL to drop forceFullDamageNextUpdate in this case. This passes the webkit_unit_tests.

[Shawn Singh]

Comment on attachment 125856 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:203
> +    CCLayerImpl* rootLayer = m_layerTreeHostImpl->rootLayer();
> +    if (rootLayer) {
> +        CCRenderSurface* renderSurface = m_layerTreeHostImpl->rootLayer()->renderSurface();
> +        if (renderSurface)
> +            renderSurface->damageTracker()->forceFullDamageNextUpdate();
> +    }

Would it be reasonable to merge this code with the exact same impl thread code below?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:287
> +    CCLayerImpl* rootLayer = m_layerTreeHostImpl->rootLayer();
> +    if (rootLayer) {
> +        CCRenderSurface* renderSurface = m_layerTreeHostImpl->rootLayer()->renderSurface();
> +        if (renderSurface)
> +            renderSurface->damageTracker()->forceFullDamageNextUpdate();
> +    }

Comment above in reference to this code...

[Jonathan Backer]

(In reply to comment #28)
> (From update of attachment 125856 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125856&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:203
> > +    CCLayerImpl* rootLayer = m_layerTreeHostImpl->rootLayer();
> > +    if (rootLayer) {
> > +        CCRenderSurface* renderSurface = m_layerTreeHostImpl->rootLayer()->renderSurface();
> > +        if (renderSurface)
> > +            renderSurface->damageTracker()->forceFullDamageNextUpdate();
> > +    }
> 
> Would it be reasonable to merge this code with the exact same impl thread code below?

The natural place to put this is in CCProxy, but I think that CCProxy is an interface class. A static helper function in CCProxy is a possibility, but I'm not sure.

@jamesr: care to weigh in?

[James Robinson]

Comment on attachment 125856 [details]
Patch

The proxy should just be for coordination between the two threads, not for logic that's specific to many proxy implementations. In this case I think the functionality really belongs on CCLayerTreeHostImpl and the proxy should just be a pass-through to that function on the correct thread.

[Jonathan Backer]

Created attachment 126069 [details]
Patch

[Jonathan Backer]

(In reply to comment #30)
> (From update of attachment 125856 [details])
> The proxy should just be for coordination between the two threads, not for logic that's specific to many proxy implementations. In this case I think the functionality really belongs on CCLayerTreeHostImpl and the proxy should just be a pass-through to that function on the correct thread.

Done.

[James Robinson]

Comment on attachment 126069 [details]
Patch

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

R=me with one comment

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:116
> +    void setFullRootLayerDamage();

Please document this. From the function name I can't tell why this is different than calling setNeedsDisplay() on the root layer

[W. James MacLean]

Created attachment 126295 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 126295 [details]
Patch for landing

Clearing flags on attachment: 126295

Committed r107243: <http://trac.webkit.org/changeset/107243>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.