612840 - Clean up layer text flags to prepare for subpixel-AA changes

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I have some patches that simplify the code involved in my changes for how we handle subpixel AA. I rebased my patches for bug 363861 and bug 593604 on top of these patches.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Remove CONTENT_NO_TEXT and ShouldRetainTransparentSurface

ShouldRetainTransparentSurface is not going to be needed; we will simply converge all platforms on the behavior that when text is over opaque pixels in an RGBA surface, it can be rendered accurately with subpixel AA. As you can see from the code removal, this is already true for GDI, Quartz and Xlib, which are the only surfaces we use BasicLayers for now that D2D goes with D3D10, so this doesn't change anything. It simplifies things to remove ShouldRetainTransparentSurface now.

After getting rid of ShouldRetainTransparentSurface, we can also get rid of the CONTENT_NO_TEXT flag because it's unused. We might need to re-add it one day, but I haven't found a use for it yet.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Remove unnecessary opacity check

I had a check for opacity == 1.0 here on the grounds that there's no point in taking the "non-retained ThebesLayer" path if we're just going to do a PushGroup for opacity and give up our chance to draw directly to the backbuffer. However, with the PushGroupAndCopyBackground stuff in bug 363861 our PushGroup can copy up the background from the backbuffer so we can draw into it with component-alpha compositing. So simplify things by taking out the opacity == 1.0 optimization now.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: Make Layer::CONTENT_NO_TEXT_OVER_TRANSPARENT applicable to all layer types

This is bug 593604 parts 3 and 6 folded together and rebased to the above patches, already r=vlad/tnikkel.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4: Rename CONTENT_NO_TEXT_OVER_TRANSPARENT to CONTENT_COMPONENT_ALPHA

It turns out to be simpler to think about a flag that means "to represent this layer properly you need component alpha" instead of just "there is text over transparent pixels". In particular, later we'll want to force some text to be grayscale (e.g., when it's over transparent parts of a window), and then we want FrameLayerBuilder to *not* set this flag.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This blocks blockers.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/ca873b14eb71
http://hg.mozilla.org/mozilla-central/rev/53bc550efbcc
http://hg.mozilla.org/mozilla-central/rev/bc423fb0aa13
http://hg.mozilla.org/mozilla-central/rev/8019b50e514c

Comment 7

[Timothy Nikkel (:tnikkel)]

Looks like part 3 never had review requested on it.

Comment 8

[Timothy Nikkel (:tnikkel)]

Comment on attachment 491133 [details] [diff] [review]
Part 3: Make Layer::CONTENT_NO_TEXT_OVER_TRANSPARENT applicable to all layer types

>+   * @param aTextContentFlags if all child layers have CONTENT_NO_TEXT, adds
>+   * CONTENT_NO_TEXT to *aTextContentFlags.
>+   * Likewise for CONTENT_NO_TEXT_OVER_TRANSPARENT.

In the implementation it looks it replaces the value of aTextContentFlags completely, not adds to it.

>+  PRUint32 textContentFlags = Layer::CONTENT_NO_TEXT_OVER_TRANSPARENT;
>+

>+      if (!layer->GetVisibleRegion().IsEmpty()) {
>+        textContentFlags &= layer->GetContentFlags();
>+      }

Seems like we will never get the CONTENT_NO_TEXT flag set this way, which doesn't match the comment above.

Comment 9

[Timothy Nikkel (:tnikkel)]

Oh, that patch was already reviewed in another bug.

Comment 10

[Timothy Nikkel (:tnikkel)]

My comments still seem relevant, although seeing as part 1 removes CONTENT_NO_TEXT that part should just be removed.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, I need to fix the comment there.

Comment 12

[Timothy Nikkel (:tnikkel)]

Comment on attachment 491133 [details] [diff] [review]
Part 3: Make Layer::CONTENT_NO_TEXT_OVER_TRANSPARENT applicable to all layer types

Looks like these have been addressed.