[webkit-reviews] review denied: [Bug 45092] [chromium] Accelerated Compositing: screen garbage when scrolling : [Attachment 66768] Patch

Tue Sep 7 15:36:42 PDT 2010

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Nat Duca
<nduca at chromium.org>'s request for review:
Bug 45092: [chromium] Accelerated Compositing: screen garbage when scrolling
https://bugs.webkit.org/show_bug.cgi?id=45092

Attachment 66768: Patch
https://bugs.webkit.org/attachment.cgi?id=66768&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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