Closed Bug 579276 Opened 14 years ago Closed 14 years ago

White text on a fixed position background triggers Bug 363861 (bad ClearType) with GDI and retained layers

Categories

(Core :: Layout, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

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

People

(Reporter: the.decryptor, Assigned: roc)

References

Details

(Keywords: regression)

Attachments

(17 files, 3 obsolete files)

4.26 KB, text/html
Details
57.11 KB, image/png
Details
1.05 KB, text/html
Details
21.57 KB, patch
cjones
: review+
Details | Diff | Splinter Review
15.64 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
20.12 KB, patch
cjones
: review+
Details | Diff | Splinter Review
7.90 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
4.76 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
942 bytes, text/html
Details
4.78 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
3.91 KB, patch
cjones
: review+
Details | Diff | Splinter Review
960 bytes, patch
tnikkel
: review+
Details | Diff | Splinter Review
4.16 KB, patch
Details | Diff | Splinter Review
1.46 KB, patch
Details | Diff | Splinter Review
2.19 KB, patch
cjones
: review+
Details | Diff | Splinter Review
6.60 KB, patch
vlad
: review+
Details | Diff | Splinter Review
1.28 KB, patch
cjones
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100715 Minefield/4.0b2pre
Build Identifier: Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100715 Minefield/4.0b2pre

As the summary says, white text on a fixed position background image (but not solid colour fill) causes the text to lose it's sub-pixel anti-aliasing, but when using GDI instead of D2D this triggers Bug 363861.

I'll upload a testcase, but you can also reproduce it by scrolling the awesomebar results, or this cssZenGarden page.

http://csszengarden.com/?cssfile=http://www.mnwebfolios.com/schaefer/awards/styleAwardnight.css

Reproducible: Always
Attached file Example page
Simple example page for the bug
Sorry, I just noticed the page I attached works ok when loaded in an iframe (but shows the bug when scrolling), but it shows the problem when loaded normally.
Attached file testcase (obsolete) —
[STR]
1. Start Latest hourly Minefield (Landed retained layers)
http://hg.mozilla.org/mozilla-central/rev/7124132f0506
Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100715 Minefield/4.0b2pre ID:20100715211042
2. Open Testcase
3. Zoom-In (if necessary)

[Actual]
Color of text is ugly.
Blocks: 564991
Keywords: regression
Summary: White text on a fixed position background triggers Bug 363861 with retained layers → White text on a fixed position background triggers Bug 363861 (bad ClearType) with GDI and retained layers
Thanks Alice.

This is sort-of expected; the fixed-pos content goes in its own layer, and the layer is transparent, so bug 363861 is triggered.

We can mitigate it by
oops.

We can mitigate this in some cases --- where the transparent layer is over a solid-color part of another layer --- by copying that solid color into the transparent layer, making it opaque again. But in general we are going to have to address 363861, and make text in transparent surfaces look less awful on Windows. It seems to me subpixel AA should be turned off in transparent surfaces, but that's not happening on Windows in some cases?
Depends on: 363861
Attached file testcase
Attachment #457792 - Attachment is obsolete: true
Blocks: 583859
I confirm the given problem happens on Windows XP using:

2010-07-16-04-mozilla-central/firefox-4.0b2pre.en-US.win32.zip

The previous day nightly is o.k.:

2010-07-15-04-mozilla-central/firefox-4.0b2pre.en-US.win32.zip
Another testcase:

1. Load http://www.w3.org/TR/SVGFilterPrimer12/
2. Note the rendering of "Erik Dahlström" on Windows XP w/ Cleartype enabled
  - Firefox 3.6 = Cleartype anti-aliasing
  - FF4beta/trunk = grayscale anti-aliasing
Status: UNCONFIRMED → NEW
Depends on: 582223
Ever confirmed: true
Here is the basic plan for dealing with subpixel AA and retained layers:
https://wiki.mozilla.org/Gecko:TransparentLayersAndSubpixelAA
I have patches implementing the first three steps of the plan.
This patch actually addresses two issues:
1) Different layer clients want to store different types of values as userdata. We can support this safely by storing a key/value pair instead of just a value. The key identifies which client stored the data, and that way clients can safely make assumptions about what type the data is.
2) We need to clean up userdata when the layer/layermanager goes away. This patch does that by forcing userdata values to descend from a base class with a virtual destructor.
Assignee: nobody → roc
Attachment #468577 - Flags: superreview?(vladimir)
Attachment #468577 - Flags: review?
Attachment #468577 - Flags: review? → review?(jones.chris.g)
This is part one of the wiki plan. This is actually fairly straightforward to do.

Somehow an unrelated bug fix crept in here. We need to call RemoveDisplayItemDataForFrame for all frames when a LayerManagerData goes away, otherwise we don't be able to find those frames again when FrameLayerBuilder runs again.
Attachment #468578 - Flags: review?(tnikkel)
Instead of adding lots of accessors, it's easier to use a flags word. This is also easier to copy around in shadowlayers.

I'm introducing two new flags here, CONTENT_NO_TEXT and CONTENT_NO_TEXT_OVER_TRANSPARENT. Hopefully the meaning is clear enough from the comments.
Attachment #468580 - Flags: superreview?(vladimir)
Attachment #468580 - Flags: review?(jones.chris.g)
Instead of extracting everything from nsDisplayItem* and passing it through various functions down to Accumulate, just pass the nsDisplayItem* down. This makes it easier to add functionality to Accumulate.
Attachment #468581 - Flags: review?(tnikkel)
This is a good testcase for checking how well each platform can composite text into a transparent surface.

The first line of each block is the reference rendering. This is how the text should look.

The second line of each block is the rendering when the text is composited into a transparent surface. This will typically look different to the first line (it would only look the same as the first line if either the platform isn't doing subpixel AA, or the platform supports component alpha, or we do some tricky stuff to composite the background into the transparent surface before drawing the text on top of it).

The third line of each block is the rendering when the text is composited into a transparent surface, but the area inside the black box is filled with opaque white before the text is painted on top. One might hope that the part of the text over the box looks equivalent to the corresponding text on the first line.
We can classify the results of the second line and the third line (over the box) into three categories: "good" (text looks as good as the first line), "bad" (text looks OK but subpixel AA is lost or similar quality reduction), or "hideous" (bug 363861 or something similar is killing us).

Results for Windows:
Windows XP, Cleartype disabled: no antialiasing anywhere, so all "good".
Windows XP and 7, Cleartype enabled: second line "hideous", third line box "good"
Windows 7, DirectWrite enabled, both GDI and D2D: second line "bad", third line box "bad"
Results for Karl's Linux box: second line "bad", third line "good"
Results for my Macos 10.5 laptop: second line "bad", third line "good"
This patch creates an API that lets us expose knowledge about the platform font renderers as revealed by the previous testcase. We don't distinguish the "bad" and "hideous" classes, since we want to avoid both. Hopefully the comments are clear.

Currently no GetTextQualityInTransparentSurfaces implementation returns TEXT_QUALITY_OK, although on XP and Linux it might be worth checking if subpixel AA is enabled at all. If it isn't, then we can return TEXT_QUALITY_OK (meaning that transparent surfaces work just as well as opaque surfaces).
Attachment #468599 - Flags: superreview?(vladimir)
Attachment #468599 - Flags: review?(jmuizelaar)
Implements the third step of the wiki plan. Should be fairly straightforward.
Attachment #468600 - Flags: review?(jones.chris.g)
Now that we are aggressively trying to get text to look great even in the presence of extra layers, we can bump the scroll timeout to a more reasonable value to reduce creation and destruction of layers.
Attachment #468603 - Flags: review?(tnikkel)
The marquee test changes in bug 573933 are no longer needed. The text in marquees always gets subpixel AA again.
Ensure that text looks good when scrolled in an overflow:auto div and when it's over a background-attachment:fixed background.
blocking2.0: ? → beta6+
Whiteboard: [needs review]
Attachment #468599 - Flags: superreview?(vladimir) → superreview+
Comment on attachment 468599 [details] [diff] [review]
Part 6: Add gfxASurface::GetTextQualityInTransparentSurfaces API


>+    /**
>+     * Determine how well text would be rendered in transparent surfaces that
>+     * are similar to this surface.
>+     */
>+    virtual TextQuality GetTextQualityInTransparentSurfaces()
>+    {
>+        return TEXT_QUALITY_BAD;
>     }

I wondered if this should be pure virtual. It's not really an inheritable quality.
Also, is D2D intentionally TEXT_QUALITY_BAD? If so it may be better to override
it with a comment as to why.
Attachment #468599 - Flags: review?(jmuizelaar) → review+
(In reply to comment #24)
> >+    /**
> >+     * Determine how well text would be rendered in transparent surfaces that
> >+     * are similar to this surface.
> >+     */
> >+    virtual TextQuality GetTextQualityInTransparentSurfaces()
> >+    {
> >+        return TEXT_QUALITY_BAD;
> >     }
> 
> I wondered if this should be pure virtual. It's not really an inheritable
> quality.

I think making the default implementation BAD is a conservative and reasonable thing to do.

> Also, is D2D intentionally TEXT_QUALITY_BAD? If so it may be better to override
> it with a comment as to why.

Yes, it's BAD because it always renders grayscale AA to transparent surfaces, even over opaque pixels. I'll do as you suggest.
Comment on attachment 468577 [details] [diff] [review]
Part 1: make layer userdata APIs be key/value based

>diff --git a/gfx/layers/Layers.h b/gfx/layers/Layers.h
>--- a/gfx/layers/Layers.h
>+++ b/gfx/layers/Layers.h
>+  /**
>+   * This setter can be used anytime. The user data for all keys is
>+   * initially null. Ownership pases to the layer manager.
>+   */
>+  void SetUserData(void* aKey, LayerUserData* aData)
>+  {
>+    NS_ASSERTION(!mUserDataKey || mUserDataKey == aKey,
>+                 "Multiple LayerUserData objects not supported");
>+    mUserDataKey = aKey;
>+    mUserData = aData;
>+  }
>+  /**
>+   * This can be used anytime. Ownership passes to the caller!
>+   */
>+  nsAutoPtr<LayerUserData> RemoveUserData(void* aKey)
>+  {
>+    if (mUserDataKey == aKey) {
>+      // the nsAutoPtr constructor steals from mUserData
>+      mUserDataKey = nsnull;
>+      nsAutoPtr<LayerUserData> d(mUserData);
>+      return d;
>+    }
>+    nsAutoPtr<LayerUserData> d;
>+    return d;
>+  }

Since you already have the |HasUserData()| test, why not
|assert(!mUserDataKey || mUserDataKey == aKey)| here and in
GetUserData() too?

>+  /**
>+   * This getter can be used anytime.
>+   */
>+  PRBool HasUserData(void* aKey)
>+  {
>+    return mUserDataKey == aKey;
>+  }
>+  /**
>+   * This getter can be used anytime. Ownership is retained by the layer
>+   * manager.
>+   */
>+  LayerUserData* GetUserData(void* aKey)
>+  {
>+    return mUserDataKey == aKey ? mUserData : static_cast<LayerUserData*>(nsnull);
>+  }
> 

I don't know why you're static_cast'ing null here.

>-  // This setter and getter can be used anytime. The user data is initially
>-  // null.
>-  void SetUserData(void* aData) { mUserData = aData; }
>-  void* GetUserData() { return mUserData; }
>+  /**
>+   * This setter can be used anytime. The user data for all keys is
>+   * initially null. Ownership pases to the layer manager.
>+   */
>+  void SetUserData(void* aKey, LayerUserData* aData)
>+  {
>+    NS_ASSERTION(!mUserDataKey || mUserDataKey == aKey,
>+                 "Multiple LayerUserData objects not supported");
>+    mUserDataKey = aKey;
>+    mUserData = aData;
>+  }
>+  /**
>+   * This can be used anytime. Ownership passes to the caller!
>+   */
>+  nsAutoPtr<LayerUserData> RemoveUserData(void* aKey)
>+  {
>+    if (mUserDataKey == aKey) {
>+      // the nsAutoPtr constructor steals from mUserData
>+      mUserDataKey = nsnull;
>+      nsAutoPtr<LayerUserData> d(mUserData);
>+      return d;
>+    }
>+    nsAutoPtr<LayerUserData> d;
>+    return d;
>+  }
>+  /**
>+   * This getter can be used anytime.
>+   */
>+  PRBool HasUserData(void* aKey)
>+  {
>+    return mUserDataKey == aKey;
>+  }
>+  /**
>+   * This getter can be used anytime. Ownership is retained by the layer
>+   * manager.
>+   */
>+  LayerUserData* GetUserData(void* aKey)
>+  {
>+    return mUserDataKey == aKey ? mUserData : static_cast<LayerUserData*>(nsnull);
>+  }
> 

It sucks to duplicate this code, since odds are we'll end up adding a
multi-key interface.  Instead, why not factor this out into

  class HasLayerUserData {
    void SetUserData(void* aKey, LayerUserData* aData)
    ...
  protected:
    nsAutoPtr<LayerUserData> mUserData;
  }

and then

  class LayerManager : protected HasLayerUserData {
  public:
    using HasLayerUserData::SetUserData;
    ...

  class Layer : protected HasLayerUserData {
  public:
    using HasLayerUserData::SetUserData;
    ...

>diff --git a/layout/base/FrameLayerBuilder.cpp b/layout/base/FrameLayerBuilder.cpp

I only skimmed from here on down and above the Layers.h changes.
Comment on attachment 468580 [details] [diff] [review]
Part 3: Change Set/IsOpaqueContent API to Get/SetContentFlags

>diff --git a/gfx/layers/Layers.h b/gfx/layers/Layers.h
>--- a/gfx/layers/Layers.h
>+++ b/gfx/layers/Layers.h
>+  enum {
>+    /**
>+     * If this is set, the caller is promising that by the end of this
>+     * transaction the entire visible region (as specified by
>+     * SetVisibleRegion) will be filled with opaque content.
>+     */
>+    CONTENT_OPAQUE = 0x01,
>+    /**
>+     * ThebesLayers only!
>+     * If this is set, the caller is promising that the visible region
>+     * contains no text at all. If this is set,
>+     * CONTENT_NO_TEXT_OVER_TRANSPARENT will also be set.
>+     */
>+    CONTENT_NO_TEXT = 0x02,
>+    /**
>+     * ThebesLayers only!
>+     * If this is set, the caller is promising that the visible region
>+     * contains no text over transparent pixels (any text, if present,
>+     * is over fully opaque pixels).
>+     */
>+    CONTENT_NO_TEXT_OVER_TRANSPARENT = 0x04
>+  };

Why not separate flags for ThebesLayers then?  Layer attributes are
easy to remote for shadow layers, no worries about that.

OK by me either way.
Attachment #468580 - Flags: review?(jones.chris.g) → review+
Comment on attachment 468600 [details] [diff] [review]
Part 7: Don't retain transparent layers that would hurt text rendering

>diff --git a/gfx/layers/basic/BasicLayers.cpp b/gfx/layers/basic/BasicLayers.cpp
>--- a/gfx/layers/basic/BasicLayers.cpp
>+++ b/gfx/layers/basic/BasicLayers.cpp
> void
> BasicThebesLayer::Paint(gfxContext* aContext,
>                         LayerManager::DrawThebesLayerCallback aCallback,
>                         void* aCallbackData,
>                         float aOpacity)
> {
>   NS_ASSERTION(BasicManager()->InDrawing(),
>                "Can only draw in drawing phase");
>   gfxContext* target = BasicManager()->GetTarget();
>   NS_ASSERTION(target, "We shouldn't be called if there's no target");
>+  nsRefPtr<gfxASurface> targetSurface = aContext->CurrentSurface();
> 
>-  if (!BasicManager()->IsRetained()) {
>+  PRBool canUseOpaqueSurface = CanUseOpaqueSurface();
>+  if (!BasicManager()->IsRetained() ||
>+      (aOpacity == 1.0 && !canUseOpaqueSurface &&
>+       !ShouldRetainTransparentSurface(mContentFlags, targetSurface))) {
>+    mValidRegion.SetEmpty();
>+    mBuffer.Clear();
>+
>     if (aOpacity != 1.0) {
>       target->Save();
>       ClipToContain(target, mVisibleRegion.GetBounds());
>       target->PushGroup(gfxASurface::CONTENT_COLOR_ALPHA);
>     }
>-    mValidRegion.SetEmpty();
>-    mBuffer.Clear();
>     aCallback(this, target, mVisibleRegion, nsIntRegion(), aCallbackData);
>     if (aOpacity != 1.0) {
>       target->PopGroupToSource();
>       target->Paint(aOpacity);
>       target->Restore();
>     }
>     return;
>   }
> 
>-  nsRefPtr<gfxASurface> targetSurface = aContext->CurrentSurface();
>-  PRBool isOpaqueContent =
>-    (targetSurface->AreSimilarSurfacesSensitiveToContentType() &&
>-     aOpacity == 1.0 &&
>-     CanUseOpaqueSurface());
>   {
>+    PRBool opaqueBuffer = canUseOpaqueSurface &&
>+      targetSurface->AreSimilarSurfacesSensitiveToContentType();
>     Buffer::ContentType contentType =
>-      isOpaqueContent ? gfxASurface::CONTENT_COLOR :
>-                        gfxASurface::CONTENT_COLOR_ALPHA;
>+      opaqueBuffer ? gfxASurface::CONTENT_COLOR :
>+                     gfxASurface::CONTENT_COLOR_ALPHA;

It appears to me that we'll end up creating an opaque surface even if
we're painting with opacity < 1.0, which isn't intended, right?
(In reply to comment #26)
> Comment on attachment 468577 [details] [diff] [review]
> Part 1: make layer userdata APIs be key/value based
> >+   * This can be used anytime. Ownership passes to the caller!
> >+   */
> >+  nsAutoPtr<LayerUserData> RemoveUserData(void* aKey)
> >+  {
> >+    if (mUserDataKey == aKey) {
> >+      // the nsAutoPtr constructor steals from mUserData
> >+      mUserDataKey = nsnull;
> >+      nsAutoPtr<LayerUserData> d(mUserData);
> >+      return d;
> >+    }
> >+    nsAutoPtr<LayerUserData> d;
> >+    return d;
> >+  }
> 
> Since you already have the |HasUserData()| test, why not
> |assert(!mUserDataKey || mUserDataKey == aKey)| here and in
> GetUserData() too?

Because that forces the caller to always call HasUserData(). It seems simpler for callers to get null instead of doing that.

> >+  /**
> >+   * This getter can be used anytime.
> >+   */
> >+  PRBool HasUserData(void* aKey)
> >+  {
> >+    return mUserDataKey == aKey;
> >+  }
> >+  /**
> >+   * This getter can be used anytime. Ownership is retained by the layer
> >+   * manager.
> >+   */
> >+  LayerUserData* GetUserData(void* aKey)
> >+  {
> >+    return mUserDataKey == aKey ? mUserData : static_cast<LayerUserData*>(nsnull);
> >+  }
> > 
> 
> I don't know why you're static_cast'ing null here.

mUserData is not a pointer type so the compiler gets confused. But yeah, I can just use mUserData.get() instead. I'll do that.

> It sucks to duplicate this code, since odds are we'll end up adding a
> multi-key interface.  Instead, why not factor this out into
> 
>   class HasLayerUserData {
>     void SetUserData(void* aKey, LayerUserData* aData)
>     ...
>   protected:
>     nsAutoPtr<LayerUserData> mUserData;
>   }
> 
> and then
> 
>   class LayerManager : protected HasLayerUserData {
>   public:
>     using HasLayerUserData::SetUserData;
>     ...
> 
>   class Layer : protected HasLayerUserData {
>   public:
>     using HasLayerUserData::SetUserData;
>     ...

Instead of protected inheritance, would you be OK with a direct member and forwarding of the Set/Get/Has/Remove calls to the member? And how about LayerUserDataSet instead of HasLayerUserData?
(In reply to comment #27)
> Why not separate flags for ThebesLayers then?  Layer attributes are
> easy to remote for shadow layers, no worries about that.

So we don't need a separate flags word and separate accessor functions. That's all.
(In reply to comment #28)
> It appears to me that we'll end up creating an opaque surface even if
> we're painting with opacity < 1.0, which isn't intended, right?

Sorry. That change was intended, and I should have mentioned it. That change is perfectly OK, cairo can paint an opaque surface with a global opacity factor applied, no problem.
(In reply to comment #29)
> (In reply to comment #26)
> > >+  /**
> > >+   * This getter can be used anytime.
> > >+   */
> > >+  PRBool HasUserData(void* aKey)
> > >+  {
> > >+    return mUserDataKey == aKey;
> > >+  }
> > >+  /**
> > >+   * This getter can be used anytime. Ownership is retained by the layer
> > >+   * manager.
> > >+   */
> > >+  LayerUserData* GetUserData(void* aKey)
> > >+  {
> > >+    return mUserDataKey == aKey ? mUserData : static_cast<LayerUserData*>(nsnull);
> > >+  }
> > > 
> > 
> > I don't know why you're static_cast'ing null here.
> 
> mUserData is not a pointer type so the compiler gets confused. But yeah, I can
> just use mUserData.get() instead. I'll do that.
> 

Duh, right.  OK.

> > It sucks to duplicate this code, since odds are we'll end up adding a
> > multi-key interface.  Instead, why not factor this out into
> > 
> >   class HasLayerUserData {
> >     void SetUserData(void* aKey, LayerUserData* aData)
> >     ...
> >   protected:
> >     nsAutoPtr<LayerUserData> mUserData;
> >   }
> > 
> > and then
> > 
> >   class LayerManager : protected HasLayerUserData {
> >   public:
> >     using HasLayerUserData::SetUserData;
> >     ...
> > 
> >   class Layer : protected HasLayerUserData {
> >   public:
> >     using HasLayerUserData::SetUserData;
> >     ...
> 
> Instead of protected inheritance, would you be OK with a direct member and
> forwarding of the Set/Get/Has/Remove calls to the member? And how about
> LayerUserDataSet instead of HasLayerUserData?

Sure.
Attachment #468577 - Flags: review?(jones.chris.g) → review+
Comment on attachment 468600 [details] [diff] [review]
Part 7: Don't retain transparent layers that would hurt text rendering

(In reply to comment #31)
> (In reply to comment #28)
> > It appears to me that we'll end up creating an opaque surface even if
> > we're painting with opacity < 1.0, which isn't intended, right?
> 
> Sorry. That change was intended, and I should have mentioned it. That change is
> perfectly OK, cairo can paint an opaque surface with a global opacity factor
> applied, no problem.

Interesting.  This might be a slight perf win then, right?
Attachment #468600 - Flags: review?(jones.chris.g) → review+
Yes. It actually fixes a regression that landed with retained layers (which didn't seem to be picked up in any bug).
Comment on attachment 468578 [details] [diff] [review]
Part 2: for a transparent layer over a solid color, hoist the color into the background of the layer

@@ -960,17 +1037,17 @@ ContainerState::ProcessDisplayItems(cons
-      NS_ASSERTION(ownLayer->HasUserData(&gLayerManagerUserData),
+      NS_ASSERTION(!ownLayer->HasUserData(&gLayerManagerUserData),

Seems like this change should just be done correctly in part 1 instead of doing it wrong in part 1 and then fixing it in part 2.
Attachment #468578 - Flags: review?(tnikkel) → review+
Comment on attachment 468581 [details] [diff] [review]
Part 4: Move nsDisplayItem parameter down to ThebesLayerData::Accumulate

@@ -164,20 +164,20 @@ protected:
+    void Accumulate(nsDisplayListBuilder* aBuilder,
+                    nsDisplayItem* aItem,
+                    const nsIntRect& aVisibleRect,
+                    const nsIntRect& aDrawRect);

ContainerState stores an mBuilder in its members, so why bother passing a nsDisplayListBuilder argument to ContainerState::Accumulate?
Attachment #468581 - Flags: review?(tnikkel) → review+
Because Accumulate is a member of ThebesLayerData.
Oh, ok.
Comment on attachment 468582 [details] [diff] [review]
Part 5: Set CONTENT_NO_TEXT and CONTENT_NO_TEXT_OVER_TRANSPARENT flags

+    /**
+     * True if there is any text visible in the layer that's over
+     * transparent pixels.
+     */
+    PRPackedBool mHasTextOverTransparent;

I think the comment would be clearer if you added "in the layer" to the end.

+IsText(nsDisplayItem* aItem) {

Other display item types that might draw something we want to consider text:
TYPE_ALT_FEEDBACK (when drawing alt text for images)
TYPE_BULLET
TYPE_CARET (would the caret ever benefit from this, or is the caret rect always snapped nicely?)
TYPE_HEADER_FOOTER
TYPE_MATHML_CHAR_FOREGROUND
Of those I think bullet might be important.
Attachment #468582 - Flags: review?(tnikkel) → review+
Comment on attachment 468603 [details] [diff] [review]
Part 8: Bump scroll timeout up a lot

Can you restore the "That's 4 generations of xms each." part of the comment? I think it's confusing otherwise.
Attachment #468603 - Flags: review?(tnikkel) → review+
(In reply to comment #39)
> Other display item types that might draw something we want to consider text:
> TYPE_ALT_FEEDBACK (when drawing alt text for images)
> TYPE_BULLET

Good call.

> TYPE_CARET (would the caret ever benefit from this, or is the caret rect always
> snapped nicely?)

No, it always snaps. Anyway it's just a rect, it doesn't get subpixel AA on any platform.

> TYPE_HEADER_FOOTER
> TYPE_MATHML_CHAR_FOREGROUND

Good call.

(In reply to comment #40)
> Comment on attachment 468603 [details] [diff] [review]
> Part 8: Bump scroll timeout up a lot
> 
> Can you restore the "That's 4 generations of xms each." part of the comment? I
> think it's confusing otherwise.

Sure.
(In reply to comment #41)
> (In reply to comment #39)
> > Other display item types that might draw something we want to consider text:
> > TYPE_ALT_FEEDBACK (when drawing alt text for images)
> > TYPE_BULLET
> 
> Good call.

If you're going to do these then you might want to consider that I think both of them can be text, or can just be images.
Attachment #468577 - Flags: superreview?(vladimir) → superreview+
Comment on attachment 468580 [details] [diff] [review]
Part 3: Change Set/IsOpaqueContent API to Get/SetContentFlags

I sorta miss the days when we actually used fewer C++ things, like ref return values that lead to assignment to function calls...
Attachment #468580 - Flags: superreview?(vladimir) → superreview+
(In reply to comment #42)
> If you're going to do these then you might want to consider that I think both
> of them can be text, or can just be images.

Hmm. I'll take ALT_FEEDBACK off the list --- no subpixel AA for it! I think we can live with that and it will avoid changing layer types when an image loads in an image-only layer.

For bullets, I think we don't need to optimize for cases where there are non-text bullets but no other text in the layer. So I'll leave that on the list.
Whiteboard: [needs review] → [needs landing]
I tried pushing this but it bounced. I think I fixed the problem but I'm rerunning through try server to make sure.
Whiteboard: [needs landing]
Trivial move
Attachment #471479 - Flags: review?(vladimir)
This is the real fix for the regression. As documented in Layers.h, DrawThebesLayer callbacks expect the caller to ensure that drawing outside the aRegionToDraw has no effect. We weren't doing that on the nonretained path.
Attachment #471480 - Flags: review?
Attachment #471480 - Flags: review? → review?(jones.chris.g)
As we did for the retained path, we should do for the nonretained path as well. This is unrelated to the regression, just a bonus.
Attachment #471481 - Flags: review?(jones.chris.g)
Performance doing what?
Attachment #471480 - Flags: review?(jones.chris.g) → review+
Attachment #471481 - Flags: review?(jones.chris.g) → review+
Whiteboard: [needs landing]
Attached patch Part 11 v2Splinter Review
Add a ClipToRegionSnapped to support snapping of the region rects.
Attachment #471666 - Flags: review?(vladimir)
Attached patch Part 12 v2Splinter Review
Call ClipToRegionSnapped on the non-retained path. This fixes a reftest failure reported by the tryserver ... a non-retained element transformed by a non-integer pixel offset was getting fuzzy edges because we got an unaligned clip.
Attachment #471479 - Attachment is obsolete: true
Attachment #471480 - Attachment is obsolete: true
Attachment #471669 - Flags: review?(jones.chris.g)
Attachment #471669 - Flags: review?(jones.chris.g) → review+
Blocks: 591734
(In reply to comment #50)
> Performance doing what?

Tab animations, listing in menus (favs in bookmarks showing one by one)...
(In reply to comment #53)
> (In reply to comment #50)
> > Performance doing what?
> 
> Tab animations, listing in menus (favs in bookmarks showing one by one)...

I can confirm all of that.
...
Not sure if related, today's builds are extremely slow w. D2D at scrolling & rendering. ?
All pages, or just some?
(In reply to comment #55)
> All pages, or just some?

All pages. Performance issues w/o D2D as well.
I'll look at this regression in bug 593361.
Depends on: 593700
Depends on: 595400
Depends on: 598575
Depends on: 600737
Whiteboard: [needs landing]
Depends on: 601894
Depends on: 615225
Depends on: 634762
Depends on: 647560
You need to log in before you can comment on or make changes to this bug.