Bug 55985 – [chromium] Make updateAndDrawLayers argumentless.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nat Duca]

[chromium] Make updateAndDrawLayers argumentless.

[Nat Duca]

Created attachment 85112 [details]
Patch

[James Robinson]

Comment on attachment 85112 [details]
Patch

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.

[Nat Duca]

Created attachment 85123 [details]
Patch

[James Robinson]

Comment on attachment 85123 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85123&action=review

Looks great!  R=me, vangelis or enne can chime in if they have any feedback before I flip cq+

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:147
> +    LayerTexture* getOffscreenLayerTexture();
> +    void copyOffscreenTextureToDisplay();

i approve of making this private but it seems that getOffscreenLayerTexture() has no callers at all. le sigh...

[Adrienne Walker]

(In reply to comment #4)
> (From update of attachment 85123 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85123&action=review
> 
> Looks great!  R=me, vangelis or enne can chime in if they have any feedback before I flip cq+

Nat talked to me about this patch in person and it sounded great.  There's no reason that all that tilers and rects had to be passed as function args rather than stored as member variables.

> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:147
> > +    LayerTexture* getOffscreenLayerTexture();
> > +    void copyOffscreenTextureToDisplay();
> 
> i approve of making this private but it seems that getOffscreenLayerTexture() has no callers at all. le sigh...

I am a little suspicious of this change, though.  +wjmaclean

[James Robinson]

(In reply to comment #5)
> > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:147
> > > +    LayerTexture* getOffscreenLayerTexture();
> > > +    void copyOffscreenTextureToDisplay();
> > 
> > i approve of making this private but it seems that getOffscreenLayerTexture() has no callers at all. le sigh...
> 
> I am a little suspicious of this change, though.  +wjmaclean

it's changing an uncalled public function to an uncalled private one.

[W. James MacLean]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 85123 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=85123&action=review
> > 
> > Looks great!  R=me, vangelis or enne can chime in if they have any feedback before I flip cq+
> 
> Nat talked to me about this patch in person and it sounded great.  There's no reason that all that tilers and rects had to be passed as function args rather than stored as member variables.
> 
> > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:147
> > > +    LayerTexture* getOffscreenLayerTexture();
> > > +    void copyOffscreenTextureToDisplay();
> > 
> > i approve of making this private but it seems that getOffscreenLayerTexture() has no callers at all. le sigh...
> 
> I am a little suspicious of this change, though.  +wjmaclean

In the long run I had originally imagined this might be called by something outside LayerRendererChromium, but I'm fine with it being internalized at this point.

I assume the patch doesn't break --enable-composite-to-texture on chrome, which is my main concern, which it doesn't look like it will.

[WebKit Commit Bot]

Comment on attachment 85123 [details]
Patch

Clearing flags on attachment: 85123

Committed r80785: <http://trac.webkit.org/changeset/80785>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.