Bug 99083 – [chromium] Pass canPaintLCDText to WebContentLayerClient::paintContents

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Alok Priyadarshi]

Stage 1 for LCD text on composited layers. Respect LCD text setting on layers instead of turning it off for all composited layers. For now WebLayer::canUseLCDText should return false, which should not change anything functionally.

[James Robinson]

WebContentLayer has setUseLCDText(bool).  I don't see why you need something on WebLayer.

[Alok Priyadarshi]

Created attachment 168271 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Alok Priyadarshi]

(In reply to comment #1)
> WebContentLayer has setUseLCDText(bool).  I don't see why you need something on WebLayer.

The new function is not enabling/disabling the LCD text. It is rather asking if it is ok to use LCD text on a particular layer. It will be clearer to review this patch along with the chromium patch here - http://codereview.chromium.org/11027045/

The chromium patch looks at draw-transform and draw-opacity of a layer to decide whether LCD text can be used. This patch looks at the setting on the layer and content opaqueness to make the final decision whether to use LCD text or not.

In subsequent patches, I will be further extend by not just looking at content opaqueness, but rather tracking if text is being painted on opaque region.

I will finally delete WebContentLayer::setUseLCDText(). There is no need to change this setting from the WebKit side of the fence.

[Alok Priyadarshi]

ping!

[Adam Barth]

@jamesr: Would you be willing to review this patch?

[James Robinson]

Comment on attachment 168271 [details]
Patch

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

> Source/Platform/chromium/public/WebLayer.h:206
> +    // Returns true if text can be painted into this layer with LCD anti-aliasing.
> +    // Typically LCD text is allowed if the layer is opaque and is not rotated
> +    // or scaled.

I don't think this API belongs on WebLayer - WebLayers don't have painted content (in general), WebContentLayers do.  It doesn't make any sense to talk about LCD text on a video or external texture layer.

> Source/Platform/chromium/public/WebLayer.h:207
> +    // TODO(alokp): Make pure virtual after implementing the impl side.

Don't use TODO(person) in WebKit comments

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:649
> +    WebLayer* childHost = platformLayer();

This change seems unrelated

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:871
> +    // TODO(alokp): Rename PlatformContextSkia::setDrawingToImageBuffer

Why don't you just do that?

> Source/WebKit/chromium/src/NonCompositedContentHost.cpp:53
> +    // TODO(alokp): Remove LCD text setting after WebLayer::canUseLCDText is implemented.

FIXME

> Source/WebKit/chromium/src/NonCompositedContentHost.cpp:175
> +    // TODO(alokp): Remove the LCD logic from here.

we don't use TODO(person) in WebKit - just FIXME and optionally cite a WebKit bug if you have one filed

[James Robinson]

I'm not sure having a bit on the layer makes sense at all since it's specific to a particular paint call.  What about passing this bit through the paint call on WebContentLayerClient?

[Alok Priyadarshi]

(In reply to comment #8)
> I'm not sure having a bit on the layer makes sense at all since it's specific to a particular paint call.  What about passing this bit through the paint call on WebContentLayerClient?

You are right this function should be moved to WebContentLayer.

I will need to add a bit about lcd-text to cc::Layer anyway because it needs to be determined and cached during calculateDrawTransforms traversal. This function would just fetch that value.

Passing this bit would work but it would need to be plumbed through a long list of interfaces:
cc::TiledLayerChromium::updateTileTextures
cc::BitmapCanvasLayerTextureUpdater::prepareToUpdate
cc::CanvasLayerTextureUpdater::paintContents
cc::ContentLayerPainter::paint
WebKit::WebContentLayerImpl::paintContents
WebCore::WebContentLayerClient::paintContents

[James Robinson]

That is a long path.  We could try to simplify things on the chromium side, but as far as WebKit is concerned the only place where it's needed is on the WebContentLayerClient call.

[Alok Priyadarshi]

This is in preparation for supporting LCD text on composited layers.

[Alok Priyadarshi]

Created attachment 169256 [details]
Patch

[Alok Priyadarshi]

@jamesr: PTAL. I passed the LCD bit to WebContentLayerClient::paintContents().

[Alok Priyadarshi]

+danakj: Could you also take a look. The patch touches some of your code.

[Dana Jansens]

Comment on attachment 169256 [details]
Patch

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

Thanks Alok, this looks like a great first step.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:302
> +    m_opaqueRectTrackingContentLayerDelegate->setOpaque(opaque);

nit: pass m_contentsOpaque in like the above line?

I'm thinking we should make the content layer call this via the WebContentLayerClient api from ContentLayer::setContentsOpaque(), so that non-web content can also set this flag. Idk if it's easiest to do that here, before, or after.

[Alok Priyadarshi]

(In reply to comment #15)
> (From update of attachment 169256 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169256&action=review
> 
> Thanks Alok, this looks like a great first step.
> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:302
> > +    m_opaqueRectTrackingContentLayerDelegate->setOpaque(opaque);
> 
> nit: pass m_contentsOpaque in like the above line?

I would prefer not to pass this bit in WebContentLayerClient::paintContents(). contentsOpaque is different from canPaintLCDText. It is not specific to a particular paint call. It is more like a property of the layer.

> I'm thinking we should make the content layer call this via the WebContentLayerClient api from ContentLayer::setContentsOpaque(), so that non-web content can also set this flag. Idk if it's easiest to do that here, before, or after.

I like this suggestion. But it would mean adding a WebContentLayerClient::setContentsOpaque call, which seems wrong on a class named Client. Usually FooClient has a reference to Foo, and can query for a property as needed. Looking at it more closely this class should rather be named WebContentLayerDelegate. Anyways, I think this can more easily be done in a separate patch. I do not need it now.

Thanks for your review Dana!

[Alok Priyadarshi]

Created attachment 170717 [details]
Patch

[Alok Priyadarshi]

Chromium patch that makes use of the new interface: http://codereview.chromium.org/11274057/

[Alok Priyadarshi]

Created attachment 170933 [details]
Patch

[Alok Priyadarshi]

I misunderstood comment #15. It is now addressed in the new patch.

[James Robinson]

Comment on attachment 170933 [details]
Patch

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

I believe this will completely break aura - you should test that.

To land an API change like this, what you probably should do is either add a #define guard for the signature change and land chromium-side code first that checks for the #define and adjusts accordingly.  After the patch lands you can remove the guards from chromium code and then remove the #define from the WebKit header (although this step is often neglected, as you can tell from the #define WEBCONTENTLAYERCLIENT_HAS_OPAQUE / _FLOAT_OPAQUE_RECT #defines in WCLC.h).

R- since this will probably break chromeos completely.  Otherwise, looks fine.

> Source/Platform/chromium/public/WebContentLayerClient.h:48
> +    // FIXME: Remove this version of paintContents after chromium starts using the
> +    // new version with canPaintLCDText.
> +    void paintContents(WebCanvas* canvas, const WebRect& clip, WebFloatRect& opaque)

Did you see the WebContentLayerClient override in ui/compositor/layer.h in the chromium tree?  This change won't work for that

[Alok Priyadarshi]

Created attachment 171310 [details]
Patch

[Alok Priyadarshi]

Comment on attachment 170933 [details]
Patch

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

>> Source/Platform/chromium/public/WebContentLayerClient.h:48
>> +    void paintContents(WebCanvas* canvas, const WebRect& clip, WebFloatRect& opaque)
> 
> Did you see the WebContentLayerClient override in ui/compositor/layer.h in the chromium tree?  This change won't work for that

Good catch. I added "virtual" back in the new patch.

[James Robinson]

Comment on attachment 171310 [details]
Patch

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

> Source/Platform/chromium/public/WebContentLayerClient.h:51
> +    // FIXME: Remove this version of paintContents after chromium starts using the
> +    // new version with canPaintLCDText.
> +    virtual void paintContents(WebCanvas* canvas, const WebRect& clip, WebFloatRect& opaque)
> +    {
> +        paintContents(canvas, clip, false, opaque);
> +    }

Now this is just weird - you're using implementation inheritance.  I really think you should add the new parameter with a guard and do the rolls that way instead of trying to rely on the default implementation.  Any code that calls the 4-arg version of paintContents() will silently break chromium code that implements only the 3-arg version today.

[Alok Priyadarshi]

Created attachment 171491 [details]
Patch

[Alok Priyadarshi]

Comment on attachment 171310 [details]
Patch

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

Chromium patch has landed and DEPS has been rolled.

>> Source/Platform/chromium/public/WebContentLayerClient.h:51
>> +    }
> 
> Now this is just weird - you're using implementation inheritance.  I really think you should add the new parameter with a guard and do the rolls that way instead of trying to rely on the default implementation.  Any code that calls the 4-arg version of paintContents() will silently break chromium code that implements only the 3-arg version today.

Added parameter guard.

[Alok Priyadarshi]

ping!

[Adam Barth]

(In reply to comment #27)
> ping!

I presume you're looking for jamesr here.  It's helpful to mention the person's name in your message so they know to take a look.

[James Robinson]

Comment on attachment 171491 [details]
Patch

Seems ok, although aving an options struct might be better. If you need to add anything else to this interface please do that instead of adding another parameter.

[WebKit Review Bot]

Comment on attachment 171491 [details]
Patch

Clearing flags on attachment: 171491

Committed r133122: <http://trac.webkit.org/changeset/133122>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen White]

Reverted r133122 for reason:

Broke Win, Android, ChromeOS builds

Committed r133125: <http://trac.webkit.org/changeset/133125>

[Stephen White]

Comment on attachment 171491 [details]
Patch

Let's try again.  r=me

[WebKit Review Bot]

Comment on attachment 171491 [details]
Patch

Clearing flags on attachment: 171491

Committed r133499: <http://trac.webkit.org/changeset/133499>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.