Bug 77555 - [chromium] Add setNeedsRedraw to WebWidget
Summary: [chromium] Add setNeedsRedraw to WebWidget
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Backer
URL:
Keywords:
Depends on: 76805 77994
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-01 07:33 PST by Nat Duca
Modified: 2012-02-09 07:40 PST (History)
7 users (show)

See Also:


Attachments
Patch (2.94 KB, patch)
2012-02-01 07:34 PST, Nat Duca
no flags Details | Formatted Diff | Diff
Patch (12.24 KB, patch)
2012-02-03 10:40 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff
Patch (8.07 KB, patch)
2012-02-03 12:18 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff
Patch (7.96 KB, patch)
2012-02-03 14:26 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff
Patch (7.93 KB, patch)
2012-02-06 10:33 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff
Patch (8.07 KB, patch)
2012-02-07 09:06 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff
Patch (8.81 KB, patch)
2012-02-08 06:11 PST, Jonathan Backer
no flags Details | Formatted Diff | Diff
Patch for landing (9.01 KB, patch)
2012-02-09 06:32 PST, W. James MacLean
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nat Duca 2012-02-01 07:33:27 PST
[chromium] Add didReceiveDamage to WebWidget
Comment 1 Nat Duca 2012-02-01 07:34:01 PST
Created attachment 124949 [details]
Patch
Comment 2 WebKit Review Bot 2012-02-01 07:36:40 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Nat Duca 2012-02-01 07:38:59 PST
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.
Comment 4 Darin Fisher (:fishd, Google) 2012-02-01 09:45:15 PST
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?
Comment 5 Jonathan Backer 2012-02-02 05:51:24 PST
(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.
Comment 6 Darin Fisher (:fishd, Google) 2012-02-02 09:54:51 PST
(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.
Comment 7 Jonathan Backer 2012-02-02 12:10:24 PST
Taking over from nduca.
Comment 8 Jonathan Backer 2012-02-03 10:40:13 PST
Created attachment 125361 [details]
Patch
Comment 9 Jonathan Backer 2012-02-03 10:41:38 PST
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.
Comment 10 Shawn Singh 2012-02-03 11:06:58 PST
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)
Comment 11 Nat Duca 2012-02-03 11:43:15 PST
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.
Comment 12 Jonathan Backer 2012-02-03 12:18:23 PST
Created attachment 125382 [details]
Patch
Comment 13 Jonathan Backer 2012-02-03 12:20:17 PST
Sorry for merging CLs. That makes it much harder to review.

Updated based on off-line discussions.
Comment 14 Antoine Labour 2012-02-03 12:36:55 PST
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.
Comment 15 Nat Duca 2012-02-03 12:43:31 PST
(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.
Comment 16 Nat Duca 2012-02-03 13:38:22 PST
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.
Comment 17 Jonathan Backer 2012-02-03 14:26:47 PST
Created attachment 125414 [details]
Patch
Comment 18 Jonathan Backer 2012-02-03 14:31:46 PST
(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.
Comment 19 Darin Fisher (:fishd, Google) 2012-02-05 20:27:43 PST
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.
Comment 20 Jonathan Backer 2012-02-06 10:33:11 PST
Created attachment 125666 [details]
Patch
Comment 21 Jonathan Backer 2012-02-06 10:33:59 PST
(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.
Comment 22 James Robinson 2012-02-06 21:23:00 PST
Comment on attachment 125666 [details]
Patch

Darin's out for a while, but this looks OK to me for now.
Comment 23 WebKit Review Bot 2012-02-07 07:17:11 PST
Comment on attachment 125666 [details]
Patch

Clearing flags on attachment: 125666

Committed r106935: <http://trac.webkit.org/changeset/106935>
Comment 24 WebKit Review Bot 2012-02-07 07:17:16 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Tony Gentilcore 2012-02-07 08:44:55 PST
This was rolled out in https://bugs.webkit.org/show_bug.cgi?id=77994 . See there for details.
Comment 26 Jonathan Backer 2012-02-07 09:06:26 PST
Created attachment 125856 [details]
Patch
Comment 27 Jonathan Backer 2012-02-07 09:08:56 PST
The CCLayerTreeHostImpl's sometimes do not have rootLayer(). Updated the CL to drop forceFullDamageNextUpdate in this case. This passes the webkit_unit_tests.
Comment 28 Shawn Singh 2012-02-07 10:41:35 PST
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...
Comment 29 Jonathan Backer 2012-02-07 11:26:19 PST
(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?
Comment 30 James Robinson 2012-02-07 11:44:42 PST
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.
Comment 31 Jonathan Backer 2012-02-08 06:11:18 PST
Created attachment 126069 [details]
Patch
Comment 32 Jonathan Backer 2012-02-08 06:12:54 PST
(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.
Comment 33 James Robinson 2012-02-08 13:37:55 PST
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
Comment 34 W. James MacLean 2012-02-09 06:32:05 PST
Created attachment 126295 [details]
Patch for landing
Comment 35 WebKit Review Bot 2012-02-09 07:40:49 PST
Comment on attachment 126295 [details]
Patch for landing

Clearing flags on attachment: 126295

Committed r107243: <http://trac.webkit.org/changeset/107243>
Comment 36 WebKit Review Bot 2012-02-09 07:40:55 PST
All reviewed patches have been landed.  Closing bug.