[webkit-reviews] review denied: [Bug 55985] [chromium] Make updateAndDrawLayers argumentless. : [Attachment 85112] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Nat Duca <nduca at chromium.org>'s
request for review:
Bug 55985: [chromium] Make updateAndDrawLayers argumentless.
https://bugs.webkit.org/show_bug.cgi?id=55985

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=85112&action=review

Looks great, R-ing for nits as usual.

i wonder if setting the viewport should implicitly invalidate rather than
requiring WebViewImpl to specify the viewport and dirty things up.  Resizing
the viewport to a different size will always invalidate the entire content
area, won't it?

> Source/WebCore/ChangeLog:7
> +

really need some words here about what's going on :)

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:184
> +    m_rootLayerContentTiler->update(*m_rootLayerContentPaint.get(),
m_viewportVisibleRect);

nit: "*m_rootLayerContentPaint.get()" ==> "*m_rootLayerContentPaint"

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:197
> +	   m_verticalScrollbarTiler->update(*m_rootLayerScrollbarPaint.get(),
m_viewportVisibleRect);

same nit as above - *foo.get() is the same as *foo but more characters

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:209
> +	   m_horizontalScrollbarTiler->update(*m_rootLayerScrollbarPaint.get(),
m_viewportVisibleRect);

here as well

> Source/WebKit/chromium/ChangeLog:7
> +

needs at least something here about what's changing

> Source/WebKit/chromium/src/WebViewImpl.cpp:2302
> +class WebViewImplContentPaintInterface : public TilePaintInterface {
>  public:

this should have a WTF_MAKE_NONCOPYABLE() doohicky before public:

also it's kind of odd that this is called ...Interface since this is an
implementation.  Mebbe WebViewImplPainter?

> Source/WebKit/chromium/src/WebViewImpl.cpp:2306
> +    explicit WebViewImplContentPaintInterface(WebViewImpl* webViewImpl)
>	   : m_webViewImpl(webViewImpl)
>      {
>      }

technically this should be in the private section and the public section should
have a static PassOwnPtr<> create() function to force people to stick this in
an OwnPtr<>

> Source/WebKit/chromium/src/WebViewImpl.cpp:2323
> -
>  class WebViewImplScrollbarPaintInterface : public TilePaintInterface {
>  public:
>      explicit WebViewImplScrollbarPaintInterface(WebViewImpl* webViewImpl)

comments about WebViewImplContentPaintInterface apply here as well (it should
be WTF_MAKE_NONCOPYABLE, should have a create() and no public c'tor, probably
shouldn't have 'Interface' in the class name).

> Source/WebKit/chromium/src/WebViewImpl.cpp:2354
> +	       m_layerRenderer->finish(); // finish all GL rendering before we
hide the window?

since you are moving this code, should this be a FIXME or a bug or something? 
It's quite an odd comment.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2391
>      // The visibleRect includes scrollbars whereas the contentRect doesn't.

seems that this comment can go bye-bye now as well, or get moved down to
updateLayerRendererViewport(). it doesn't make much sense here any more

> Source/WebKit/chromium/src/WebViewImpl.cpp:2409
> +    RefPtr<LayerRendererChromium> layerRenderer =
LayerRendererChromium::create(newContext, adoptPtr(new
WebViewImplContentPaintInterface(this)), adoptPtr(new
WebViewImplScrollbarPaintInterface(this)));

instead of having adoptPtr() here WebViewImplContentPaintInterface should be
have a static PassOwnPtr<> create() function of its own.