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

Tue Sep 7 15:36:42 PDT 2010

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

Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #66768|review?                     |review-
               Flag|                            |

--- Comment #12 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2010-09-07 15:36:42 PST ---
(From update of attachment 66768)
View in context: https://bugs.webkit.org/attachment.cgi?id=66768&action=prettypatch

I only reviewed the bits in WebKit/chromium for now.

> WebKit/chromium/public/WebWidget.h:62
> +    
nit: it looks like there is some extra whitespace here

> WebKit/chromium/public/WebWidget.h:70
> +    virtual void paint(WebCanvas*, const WebRect& viewport) = 0;
nit: viewport -> viewPort

> WebKit/chromium/public/WebWidget.h:71
> +    
and here

> WebKit/chromium/public/WebWidget.h:73
> +    // The finish argument controls whether the compositor will waiting for the
nit: "will waiting" -> "will wait"

> WebKit/chromium/public/WebWidget.h:78
> +
and here

> WebKit/chromium/public/WebWidget.h:79
> +    // Called to inform the WebWidget of a change in native widget settings.
nit: "change in native widget settings" -> "change in theme"

> WebKit/chromium/public/WebWidgetClient.h:49
> +    // Called when a call to WebWidget::composite is required
nit: please end this comment with a period.  nit: please move scheduleComposite
down below didScrollRect since didInvalidateRect and didScrollRect really belong
together.

> WebKit/chromium/src/ChromeClientImpl.cpp:529
> +        m_webView->scrollRootLayer(scrollDelta, clipRect);
nit: it seems like scrollRootLayer should also end with "Rect" to match the style of invalidateRootLayerRect.

> WebKit/chromium/src/WebPopupMenuImpl.cpp:180
> +}
nit: please add a new line between method definitions

> WebKit/chromium/src/WebPopupMenuImpl.h:66
> +    virtual void themeChanged();
nit: the order of method declarations here should match the order in which they are declared in WebWidget.h

> WebKit/chromium/src/WebViewImpl.cpp:915
> +        if (isAcceleratedCompositingActive()) {
nit: elsewhere you enclose the isAcceleratedCompositingActive() call within #if USE(ACCELERATED_COMPOSITING).  might be good to be consistent with that.

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

> WebKit/chromium/src/WebViewImpl.cpp:957
> +        doComposite(canvas, false);
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?

i hadn't considered this issue when we talked about these API interfaces
before.

would it be meaningful to split up the composite step from the present
step?

> WebKit/chromium/src/WebViewImpl.cpp:2109
> +        WebRect damagedRect(0, 0, m_size.width, m_size.height);
nit: better to use IntRect for internal usage.  WebRect should just be
used at the interface level when passing things across the interface.

> WebKit/chromium/src/WebViewImpl.cpp:2192
> +    m_client->scheduleComposite();
perhaps we should re-order these?  imagine if the implementor of
scheduleComposite did something evil like call WebWidget::close!
in general, you should be extra defensive when you call out to
foreign code.

> WebKit/chromium/src/WebViewImpl.cpp:2197
> +void WebViewImpl::scrollRootLayer(const WebSize& scrollDelta, const WebRect& clipRect)
nit: recommend using IntSize and IntRect here

> WebKit/chromium/src/WebViewImpl.cpp:2222
> +                           contentRect.width(), contentRect.height());
nit: seems odd to line wrap all params except the last.  either wrap all or none?

> WebKit/chromium/src/WebViewImpl.cpp:2225
> +    // shared memory section.
nit: this comment mentions shared memory.  i don't see the painting step down below.

> WebKit/chromium/src/WebViewImpl.cpp:2227
> +        float dx = (float)scrollDelta.width;
nit: prefer C++ style static_cast<float> over C-style cast

> WebKit/chromium/src/WebViewImpl.cpp:2251
> +    IntRect iDamagedRect(damagedRect);
this is another reason to pass IntRect to this method instead of WebRect.

you could just write m_scrollDamage.unite(damagedRect), right?

> WebKit/chromium/src/WebViewImpl.cpp:2255
> +}
nit: add new line between method definitions

> WebKit/chromium/src/WebViewImpl.cpp:2256
> +void WebViewImpl::invalidateRootLayerRect(const WebRect& rect)
nit: we generally prefer to use WebCore types instead of API types for internal functions.
type conversion should happen on the barrier of the API.  WebRect only exists as a means to
hide IntRect (and the WebCore namespace) from the embedder (Chromium).

> WebKit/chromium/src/WebViewImpl.cpp:2262
> +    WebFrameImpl* webframe = mainFrameImpl();
it would probably be a bit more direct to go page()->mainFrame()->view() to get to the main FrameView.  in this case, you only need to null check page().

> WebKit/chromium/src/WebViewImpl.cpp:2269
> +    FloatRect lsRect(rect.x + view->scrollX(),
nit: lsRect is a mysterious name.  maybe contentRect would be better since you are taking scroll offset into account?

maybe you should be using ScrollView::windowToContents for this conversion?

(I'm concerned that all of this logic is only correct for the top-most FrameView, but we can worry about IFRAMEs in a separate bug.)

> WebKit/chromium/src/WebViewImpl.cpp:2288
> +        m_client->didInvalidateRect(damagedRect);
you could also call FrameView::invalidate() on the main FrameView.  that should be equivalent, but at least it would allow any future code that intercepts invalidations made on a FrameView to notice.

> WebKit/chromium/src/WebViewImpl.cpp:2302
> +    WebFrameImpl* webframe = mainFrameImpl();
nit: same comment about getting the FrameView from page()->mainFrame()->view().

> WebKit/chromium/src/WebViewImpl.cpp:2312
> +    IntRect viewport    = IntRect(0, 0, m_size.width, m_size.height);
nit: viewport -> viewPort and it is also not conventional in webkit style to line up the "="s like that.

> WebKit/chromium/src/WebViewImpl.cpp:2319
> +    damageRects.append(FloatRect(m_scrollDamage.x, m_scrollDamage.y, m_scrollDamage.width, m_scrollDamage.height));
if m_scrollDamage were stored as IntRect, then conversion to FloatRect would be implicit.

> WebKit/chromium/src/WebViewImpl.cpp:2322
> +        // the damage rects for the root layer is in layer space [e.g. unscrolled.]
nit: "the damage rects..." -> "The damage rects..."

> WebKit/chromium/src/WebViewImpl.cpp:2324
> +        const FloatRect ssRect = damageRects[i];
nit: prefer a more meaningful name than ssRect.  i'm not sure what the "ss" prefix means.

this looks like it could be the output of ScrollView::contentsToWindow.  maybe you should call the variable visibleDamage?  after intersecting with the viewPort that would seem to describe what it is.

> WebKit/chromium/src/WebViewImpl.cpp:2328
> +        
nit: avoid unnecessary whitespace.  blank lines should not have extra whitespace.

> WebKit/chromium/src/WebViewImpl.cpp:2334
> +        WebRect wrect(rect.x(), rect.y(), rect.width(), rect.height());
avoiding use of WebRect for internal interfaces should help eliminate the need for this WebRect temporary.

> WebKit/chromium/src/WebViewImpl.cpp:2346
> +    // Readback into the canvas, if requested.
nit: perhaps the readback step can be factored out into a helper function?

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

> WebKit/chromium/src/WebViewImpl.h:96
> +    virtual void themeChanged();
nit: list methods here in the same order they are declared in WebWidget.h

> WebKit/chromium/src/WebViewImpl.h:520
> +    WebRect m_scrollDamage;
this should be stored as IntRect

> WebKit/chromium/tests/PopupMenuTest.cpp:131
> +    virtual void themeChanged() { }
nit: list methods here in the same order they are declared in WebWidget.h

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