[webkit-reviews] review denied: [Bug 49998] Allow ports finer-grain control of when to enable accelerated compositing : [Attachment 75074] Patch adjusted per reviewer comments

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Vangelis Kokkevis
<vangelis at chromium.org>'s request for review:
Bug 49998: Allow ports finer-grain control of when to enable accelerated
compositing
https://bugs.webkit.org/show_bug.cgi?id=49998

Attachment 75074: Patch adjusted per reviewer comments
https://bugs.webkit.org/attachment.cgi?id=75074&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=75074&action=review

> WebCore/page/ChromeClient.h:250
> +	   enum CompositorTrigger {
> +	       ThreeDTransformTrigger,
> +	       VideoTrigger,
> +	       PluginTrigger,
> +	       CanvasTrigger,
> +	       AnimationTrigger,
> +	       CompositorTriggerLast
> +	   };
> +	   // Returns true if the accelerated compositor should be triggered in
the
> +	   // presence of a given element in the page.
> +	   virtual bool doAcceleratedCompositingFor(CompositorTrigger) const {
return true; }

I think this would be better named as allowAcceleratedCompositingFor().

I also think that it would be better for it to simply return a bitfield (or
WTF::Bitmap), so that only one call is required. Since there are no other uses
of WTF::BitMap in WebCore that I can find, maybe an old-schoold bitmap is more
appropriate. In that case, I'd declare the enum like:

enum CompositingTrigger {
  ThreeDTransformTrigger = 1 << 0,
  VideoTrigger = 1 << 1,
  ...
  AllTriggers = 0xFFFFFFFF;
};
typedef unsigned CompositingTriggerFlags;

virtual CompositingTriggerFlags allowedCompositingTriggers() const;

If we do this, then we can eliminate allowsAcceleratedCompositing() too.

You should probably have separate enum values for 2D canvas and WebGL.

> WebCore/rendering/RenderLayer.h:292
> +    bool canRender3dTransforms() const;

We normally use 3DTransforms, not 3dTransforms

> WebCore/rendering/RenderLayerCompositor.cpp:154
> +	       for (unsigned i = 0; i < ChromeClient::CompositorTriggerLast;
++i) {
> +		   if
(chromeClient->doAcceleratedCompositingFor(static_cast<ChromeClient::Compositor
Trigger>(i)))
> +		       compositorTriggers.set(i);

If we change ChromeClient to return a bitfield, then there's no need to loop
here.

> WebCore/rendering/RenderLayerCompositor.cpp:163
> -    if (hasAcceleratedCompositing != m_hasAcceleratedCompositing ||
showDebugBorders != m_showDebugBorders || showRepaintCounter !=
m_showRepaintCounter)
> +    if (hasAcceleratedCompositing != m_hasAcceleratedCompositing ||
showDebugBorders != m_showDebugBorders || showRepaintCounter !=
m_showRepaintCounter || compositorTriggersChanged)
>	   setCompositingLayersNeedRebuild();

I don't like the fact that ChromeClient can return different flags (hence the
need for compositorTriggersChanged). I think the contract should be that the
flags can never change. If they can change, then there needs to be a new entry
point that gets called when they do. If the flags would only ever change for
developers toggling some debug settings, then this is OK.

> WebCore/rendering/RenderLayerCompositor.h:240
> +    typedef WTF::Bitmap<ChromeClient::CompositorTriggerLast>
CompositorTriggerBitfield;
> +    CompositorTriggerBitfield m_compositorTriggers;

This could just be the CompositingTriggerFlags returned from ChromeClient,
though it's a bit weird to reuse ChromeClient's enum everywhere.