Closed Bug 612840 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(4 files)

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.
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.
Attachment #491129 - Flags: review?(vladimir)
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.
Attachment #491131 - Flags: review?(jones.chris.g)
This is bug 593604 parts 3 and 6 folded together and rebased to the above patches, already r=vlad/tnikkel.
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.
Attachment #491135 - Flags: review?(vladimir)
Attachment #491131 - Flags: review?(jones.chris.g) → review+
Whiteboard: [needs landing]
This blocks blockers.
blocking2.0: --- → betaN+
Looks like part 3 never had review requested on it.
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.
Attachment #491133 - Flags: review?(tnikkel)
Oh, that patch was already reviewed in another bug.
My comments still seem relevant, although seeing as part 1 removes CONTENT_NO_TEXT that part should just be removed.
Yes, I need to fix the comment there.
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.
Attachment #491133 - Flags: review?(tnikkel)
You need to log in before you can comment on or make changes to this bug.