Bug 64658 - Add code to attempt to align compositing layers to pixel boundaries when page scale changes
Summary: Add code to attempt to align compositing layers to pixel boundaries when page...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-16 15:30 PDT by Simon Fraser (smfr)
Modified: 2011-07-16 19:34 PDT (History)
2 users (show)

See Also:


Attachments
Patch (37.50 KB, patch)
2011-07-16 15:48 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (37.71 KB, patch)
2011-07-16 17:59 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (38.19 KB, patch)
2011-07-16 18:18 PDT, Simon Fraser (smfr)
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2011-07-16 15:30:24 PDT
Add code to attempt to align compositing layers to pixel boundaries when page scale changes
Comment 1 Simon Fraser (smfr) 2011-07-16 15:48:04 PDT
Created attachment 101102 [details]
Patch
Comment 2 mitz 2011-07-16 17:57:58 PDT
Comment on attachment 101102 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsLayer.h:354
> +    virtual bool keepPixelAligned() const { return m_keepPixelAligned; }

This name makes it sounds as if calling this function keeps the GraphicsLayer pixel-aligned. Note how the name differs from “appliesPageScale” in this regard. I suggest “maintainsPixelAlignment” or “snapsToDevicePixels” and renaming the setter and member variable to match.

> Source/WebCore/platform/graphics/GraphicsLayerClient.h:67
>      // Multiplier for backing store size, related to high DPI.

LOL “related to high DPI”.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:802
> +            if (currLayer->client())
> +                pageScale = currLayer->client()->pageScaleFactor();

Would it make sense to add GraphicsLayer::pageScaleFactor() that does this?

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:819
> +    float pageScaleFactor = 1;

No need to initialize this.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:830
> +        pageScaleFactor = m_client ? m_client->pageScaleFactor() : 1;

Would it make sense to add GraphicsLayer::pageScaleFactor() that does this?

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2439
> +    FloatRect alignedBounds = enclosingIntRect(scaledBounds);

Why always expand? I am afraid that this will turn abutting layers into overlapping layers.
Comment 3 Simon Fraser (smfr) 2011-07-16 17:59:24 PDT
Created attachment 101107 [details]
Patch
Comment 4 Simon Fraser (smfr) 2011-07-16 18:01:13 PDT
(In reply to comment #2)
> (From update of attachment 101102 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101102&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2439
> > +    FloatRect alignedBounds = enclosingIntRect(scaledBounds);
> 
> Why always expand? I am afraid that this will turn abutting layers into overlapping layers.

The added parts of the layers will always be transparent, so new overlap should not matter.

New patch ensures that we only set the 'applies scale' bit for the RenderView in main frame.
Comment 5 Simon Fraser (smfr) 2011-07-16 18:18:16 PDT
Created attachment 101109 [details]
Patch
Comment 6 Simon Fraser (smfr) 2011-07-16 19:34:40 PDT
http://trac.webkit.org/changeset/91161