[Webkit-unassigned] [Bug 45092] [chromium] Accelerated Compositing: screen garbage when scrolling

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=45092





--- Comment #15 from Nat Duca <nduca at chromium.org>  2010-09-07 18:16:23 PST ---
> it seems like scrollRootLayer should also end with "Rect" to match the style of invalidateRootLayerRect.

Interesting thought. The one difference is that scrollLayer isn't actually sending a rectangle. I personally lean away from adding a rect suffix, but I'm ok either way. :)


> nit: elsewhere you enclose the isAcceleratedCompositingActive() call within #if USE(ACCELERATED_COMPOSITING).  might be good to be consistent with that.
The rule I'm [trying] to follow is as follows...

If the code depends on accelerated compositing being ON, but does not itself use any code that is conditional on USE(ACCELERATED_COMPOSITING), I use isAcceleratedCompositingActive. E.g. render_widget.cc::DoDeferredUpdate's uses isAccelerated because the methods it dispatches to are always present.

In contrast, if the code uses functions or variables that are conditional on USE(ACCELERATED_COMPOSITING), I enclose the just the "dependent" part of the function in a USE(ACCELERERATED_COMPOSITING) test, leaving the isAccelerated test outside. E.g.:
  if(isAcceleratedCompositing()) {
#if USE(ACCELERATED_COMPOSITING)
    someCompositorOnlyFunction();
#endif
  } else {
    // software path
  }
To me at least, this seemed better than:
#if USE(ACCELERATED_COMPOSITING)
  if(isAcceleratedCompositing()) {
    someCompositorOnlyFunction();
  } else {
#endif
    // software path
#if USE(ACCELERATED_COMPOSITING)
  }
#endif

I can change approach if there are strong opinions. :)


> WebKit/chromium/src/WebViewImpl.cpp:956
> +        invalidateRootLayerRect(rect);
> why do we need to invalidate here?  it seems strange to invalidate during painting.
I have no clue! Deleted! :)

> in the case where this paint method is being called to capture a thumbnail
> for the new tab page in chromium, shouldn't we avoid the present step?

Good idea. I'll push the present() that is in the doComposite flow up to the ::paint function.
The placeholder for James' readback code will also move up to ::paint.



> WebViewImpl::themeChanged:  you could also call FrameView::invalidate() on the main FrameView
I switched over to this code. Afaik, I can call this directly, and it will route back to the view if needed. Does this seem right?




>> WebKit/chromium/src/WebViewImpl.cpp:2356
>> +    // FIXME: this is being called too early, we need to defer calling this 
>isn't this what handling the finish flag will mean?  is this FIXME redundant with that FIXME?
Yes it is. Deleted.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.