Bug 87600 – [BlackBerry] Make custom compositing thread layers more flexible

• Component: WebKit BlackBerry
• Status: RESOLVED FIXED

[Arvid Nilsson]

Introduce a LayerCompositingThreadClient that's used to fine tune the
    behaviour of custom layers. Let the LayerTiler be a
    LayerCompositingThreadClient and thus decouple it from
    LayerCompositingThread. Adjust method signatures to allow a one-to-many
    relationship between Client and Layer.
    
    Remove the old LayerCompositingThread::drawCustom() in favour of this new
    Client interface.
    
    PR #156812

Please note that this has not been internally reviewed yet, so it needs actual review. And also, it has a dependency that needs to be landed first.

[Arvid Nilsson]

Created attachment 144242 [details]
Patch

[Yong Li]

Comment on attachment 144242 [details]
Patch

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

Looks good to me except:

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:-449
> -    if (m_texID || pluginView())
> -        return;
> -
> -#if ENABLE(VIDEO)
> -    if (mediaPlayer())
> -        return;
> -#endif

Any reason for this?

[Arvid Nilsson]

(In reply to comment #2)
> (From update of attachment 144242 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144242&action=review
> 
> Looks good to me except:
> 
> > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:-449
> > -    if (m_texID || pluginView())
> > -        return;
> > -
> > -#if ENABLE(VIDEO)
> > -    if (mediaPlayer())
> > -        return;
> > -#endif
> 
> Any reason for this?

Thanks for reviewing :) In fact, if m_texId (I.e. Canvas or webgl layer) or pluginview or mediaplayer, m_client will be 0, so they might as well just fall through.

[Rob Buis]

Comment on attachment 144242 [details]
Patch

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

Looks good.

> Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:297
> +void LayerCompositingThread::drawMissingTextures(int positionLocation, int texCoordLocation, const FloatRect& visibleRect)

Is visibleRect needed? If it is going to be later maybe mark it as unused.

[Arvid Nilsson]

(In reply to comment #4)
> (From update of attachment 144242 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144242&action=review
> 
> Looks good.
> 
> > Source/WebCore/platform/graphics/blackberry/LayerCompositingThread.cpp:297
> > +void LayerCompositingThread::drawMissingTextures(int positionLocation, int texCoordLocation, const FloatRect& visibleRect)
> 
> Is visibleRect needed? If it is going to be later maybe mark it as unused.

It was added for symmetry with drawTextures - I anticipated that this method would need to do similar calculations as drawTextures, only drawing checkerboard instead of actual contents. However, in the future it might be that visibleRect will be removed from both methods instead, since it's only used for media layers and the calculations there look a bit sketchy...

[Arvid Nilsson]

Created attachment 144392 [details]
Patch

[Arvid Nilsson]

Comment on attachment 144392 [details]
Patch

New patch, adressing Rob's comment about visibleRect

[Rob Buis]

Comment on attachment 144392 [details]
Patch

Looks fine now :)

[WebKit Review Bot]

Comment on attachment 144392 [details]
Patch

Clearing flags on attachment: 144392

Committed r118706: <http://trac.webkit.org/changeset/118706>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.