[webkit-reviews] review requested: [Bug 88384] [chromium] Certain settings in CCSettings could be global : [Attachment 146666] Patch

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 88384: [chromium] Certain settings in CCSettings could be global
https://bugs.webkit.org/show_bug.cgi?id=88384

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

------- Additional Comments from vollick at chromium.org
(In reply to comment #13)
> (From update of attachment 146558 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=146558&action=review
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:147
> > +	 // Update settings based on capabilities that we got back from the
renderer.
>
> Ah yes, I think this is a (potential) problem - these capabilities are
specific to the context used by this compositor instance, so I think it's
(slightly) wrong to have them change global settings.  I say slightly because
it would only matter if different contexts had different capabilities - it's
really hard to think of a case where that would happen.
>
> I could imagine these changing over time - for instance, on a dual-GPU system
if we switched from one to the other I could imagine the maximum texture size
changing.  Today if that happens we lose all contexts on the "old" device and
then have to reinitialize them all.
>
> Perhaps some of these really should be CCLayerTreeSettings - such as
usingAcceleratedPainting, maxPartialTextureUpdates, maxUntiledLayerSize.
That sounds safe. I've made these CCLayerTreeSettings.
>
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:150
> > +	
CCSettings::setDefaultTileSize(IntSize(min(CCSettings::defaultTileSize().width(
), m_proxy->layerRendererCapabilities().maxTextureSize),
>
> It's a bit strange to set the default value based on a capability - but maybe
this setting is just poorly named?  I think we can safely assume that the max
texture size will always be >=2k since that's required by DX9, so maybe we
don't need this?
I'm not sure about the name, but at first glance it seems reasonable to bound
the default tile size by the maximum possible texture size.