Bug 92327 - -webkit-background-clip:text is blurry in WebKit 1 apps when deviceScaleFactor > 1
Summary: -webkit-background-clip:text is blurry in WebKit 1 apps when deviceScaleFacto...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords: InRadar
: 93298 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-07-25 18:18 PDT by Beth Dakin
Modified: 2012-08-08 21:02 PDT (History)
13 users (show)

See Also:


Attachments
Patch (17.46 KB, patch)
2012-07-25 18:33 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch for style bot (17.47 KB, patch)
2012-07-25 18:39 PDT, Beth Dakin
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch that should build everywhere (29.19 KB, patch)
2012-07-25 19:14 PDT, Beth Dakin
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (30.07 KB, patch)
2012-07-26 14:29 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Much simpler patch (4.43 KB, patch)
2012-07-26 22:33 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch that works for gradients with scaled ancestors (1.31 MB, patch)
2012-07-27 12:09 PDT, Beth Dakin
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (1.31 MB, patch)
2012-07-27 13:16 PDT, Beth Dakin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2012-07-25 18:18:22 PDT
-webkit-background-clip:text is blurry in WebKit 1 when deviceScaleFactor > 1

<rdar://problem/11683788>

Patch forthcoming.
Comment 1 Beth Dakin 2012-07-25 18:33:01 PDT
Created attachment 154509 [details]
Patch
Comment 2 WebKit Review Bot 2012-07-25 18:34:57 PDT
Attachment 154509 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/css/CSSGradientValue.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Beth Dakin 2012-07-25 18:39:09 PDT
Created attachment 154510 [details]
Patch for style bot
Comment 4 Early Warning System Bot 2012-07-25 18:58:00 PDT
Comment on attachment 154510 [details]
Patch for style bot

Attachment 154510 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13340465
Comment 5 Beth Dakin 2012-07-25 19:14:15 PDT
Created attachment 154524 [details]
Patch that should build everywhere
Comment 6 Beth Dakin 2012-07-25 19:20:15 PDT
There may be a simpler way to do this…
Comment 7 Simon Fraser (smfr) 2012-07-25 20:03:49 PDT
Comment on attachment 154524 [details]
Patch that should build everywhere

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

> Source/WebCore/css/CSSGradientValue.cpp:78
> +    if (FrameView* frameView = renderer->view()->frameView()) {
> +        if (HostWindow* hostWindow = frameView->hostWindow()) {
> +            if (!hostWindow->ctmIncludesDeviceScaleFactor()) {
> +                if (Page* page = renderer->document()->page())

Hmm, that's pretty icky. Could we just export a method on Page, perhaps, that would make this cleaner?

> Source/WebCore/page/Chrome.h:90
> +        virtual bool ctmIncludesDeviceScaleFactor();

This feels a bit too general. It doesn't say _which_ ctm. I could imagine that we could easily have bugs where we use code to draw both into the tiles, and into an ImageBuffer, and we need to be able to get both of them right.

I think 'device scale is baked into CTM' is something you have to ask the GraphicsContext.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:836
> +        if (FrameView* frameView = view()->frameView()) {
> +            if (HostWindow* hostWindow = frameView->hostWindow()) {
> +                if (!hostWindow->ctmIncludesDeviceScaleFactor()) {
> +                    if (Page* page = document()->page())
> +                        scaleFactorNotIncludedInCCTM = page->deviceScaleFactor();

Similar ickitude.
Comment 8 Beth Dakin 2012-07-26 13:07:18 PDT
(In reply to comment #7)
> (From update of attachment 154524 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154524&action=review
> 
> > Source/WebCore/css/CSSGradientValue.cpp:78
> > +    if (FrameView* frameView = renderer->view()->frameView()) {
> > +        if (HostWindow* hostWindow = frameView->hostWindow()) {
> > +            if (!hostWindow->ctmIncludesDeviceScaleFactor()) {
> > +                if (Page* page = renderer->document()->page())
> 
> Hmm, that's pretty icky. Could we just export a method on Page, perhaps, that would make this cleaner?
> 

Agreed. A method on Page is a really good idea. I will go in that direction.

> > Source/WebCore/page/Chrome.h:90
> > +        virtual bool ctmIncludesDeviceScaleFactor();
> 
> This feels a bit too general. It doesn't say _which_ ctm. I could imagine that we could easily have bugs where we use code to draw both into the tiles, and into an ImageBuffer, and we need to be able to get both of them right.
> 
> I think 'device scale is baked into CTM' is something you have to ask the GraphicsContext.
> 

I'm a little confused here. Are you saying that you think that this function SHOULD move to GraphicsContext? Or are you just saying that it named poorly because it is named AS IF it's a GraphicsContext function?

I definitely agree that it is poorly named. But if you are saying that you think is SHOULD move to GraphicsContext, well I just don't know how we would be able to answer that question at that level. I tired a number of approaches there since that would simplify this code greatly, but nothing has panned out.
Comment 9 Beth Dakin 2012-07-26 14:29:01 PDT
Created attachment 154751 [details]
Patch

Okay, here's a patch with better names, and with new function on Page to avoid the ickiness of the old patch in RenderBoxModelObject and GeneratorGeneratedImage.
Comment 10 Beth Dakin 2012-07-26 17:24:25 PDT
Comment on attachment 154751 [details]
Patch

Oh, I have a way simpler approach. New patch coming soon.
Comment 11 Beth Dakin 2012-07-26 22:33:47 PDT
Created attachment 154843 [details]
Much simpler patch
Comment 12 Simon Fraser (smfr) 2012-07-27 10:45:55 PDT
Comment on attachment 154843 [details]
Much simpler patch

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

Does this do the right thing with a gradient on a <div> which has an ancestor with a scale transform? That might be a good testcase to have.

> Source/WebCore/platform/graphics/GraphicsContext.h:562
> +        // getCompatibleCTM() is guaranteed to include the deviceScaleFactor

Period at end of sentence.
Comment 13 Darin Adler 2012-07-27 10:55:45 PDT
Comment on attachment 154843 [details]
Much simpler patch

I love this solution, but I do not understand the meaning of the word “compatible” in this function name. The matrix is compatible with what? The createCompatibleBuffer function uses the word compatible to mean “compatible with the device to be drawn on”, but I don’t see how the matrix would be compatible. Maybe deviceSpaceCTM would be a good name?
Comment 14 Darin Adler 2012-07-27 10:56:02 PDT
Comment on attachment 154843 [details]
Much simpler patch

Or deviceTransform?
Comment 15 Beth Dakin 2012-07-27 10:58:33 PDT
(In reply to comment #13)
> (From update of attachment 154843 [details])
> I love this solution, but I do not understand the meaning of the word “compatible” in this function name. The matrix is compatible with what? The createCompatibleBuffer function uses the word compatible to mean “compatible with the device to be drawn on”, but I don’t see how the matrix would be compatible. Maybe deviceSpaceCTM would be a good name?

I agree that the name is weak. I like your suggestions! I need to do a bit more testing before I commit because there may actually be a bug still with the case Simon mentioned (gradients with a scaled ancestor). Anyway, I will consider one of these re-names before I commit.
Comment 16 Beth Dakin 2012-07-27 12:09:54 PDT
Created attachment 155011 [details]
Patch that works for gradients with scaled ancestors
Comment 17 Eric Seidel (no email) 2012-07-27 12:11:47 PDT
Comment on attachment 154843 [details]
Much simpler patch

Cleared Simon Fraser's review+ from obsolete attachment 154843 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 18 Simon Fraser (smfr) 2012-07-27 12:37:39 PDT
Comment on attachment 155011 [details]
Patch that works for gradients with scaled ancestors

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

> Source/WebCore/platform/graphics/GraphicsContext.h:419
> +        // deviceTransform() is similar to getCTM(), but it is guaranteed to include the deviceScaleFactor.
> +        AffineTransform deviceTransform() const;

I don't think deviceTransform is a good name; it implies only 1x or 2x. Maybe ctmIncludingDeviceScale()?
Comment 19 Beth Dakin 2012-07-27 13:16:20 PDT
Created attachment 155036 [details]
Patch
Comment 20 Simon Fraser (smfr) 2012-07-27 13:23:36 PDT
Comment on attachment 155036 [details]
Patch

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

> Source/WebCore/ChangeLog:37
> +        Actually use the new parameter in the CG implementation. Use CG API to get a
> +
> +        matrix that definitely includes the device scale when that is required. 

Blank line here.

> Source/WebCore/platform/graphics/GraphicsContext.h:417
> +        enum IncludeDeviceScale { DefinitelyIncludeDeviceScale, PossiblyIncludeDeviceScale};

Space before the }

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1425
>      return AffineTransform(CGContextGetCTM(platformContext()));

It seems like this explicit construction of a AffineTransform is unnecessary.

> LayoutTests/fast/hidpi/gradient-with-scaled-ancestor.html:2
> +<div style="-webkit-transform:scale(2); -webkit-transform-origin:top left;">
> +<div style="height:300px;width:300px;background-image:-webkit-repeating-linear-gradient(top left, red 10%, blue 30%);border:1px solid">

You should fix this test to not show a scrollbar in the image result.
Comment 21 Beth Dakin 2012-07-27 13:34:47 PDT
Fixed! And committed http://trac.webkit.org/changeset/123912
Comment 22 mitz 2012-08-08 21:02:18 PDT
*** Bug 93298 has been marked as a duplicate of this bug. ***