Bug 149144 - [Win] Tiled drawing is rendering more times than it should
Summary: [Win] Tiled drawing is rendering more times than it should
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: PC All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-09-14 17:44 PDT by Brent Fulgham
Modified: 2015-09-15 12:56 PDT (History)
3 users (show)

See Also:


Attachments
WIP Patch (Not for review) (51.57 KB, patch)
2015-09-14 20:58 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch v2 (53.21 KB, patch)
2015-09-14 21:54 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (54.19 KB, patch)
2015-09-15 10:17 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (Correct deprecation warning) (54.41 KB, patch)
2015-09-15 10:39 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch v3 (revised for EWS and smfr's comments) (55.49 KB, patch)
2015-09-15 11:29 PDT, Brent Fulgham
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 Brent Fulgham 2015-09-14 17:44:39 PDT
Errors in the Windows tiled drawing implementation cause WebKit to draw the same content multiple times. Furthermore, the nested CALayers used in the tiled drawing system caused the coordinate system to be flipped twice, causing some content to render upside down.

This patch corrects both of these problems. It also has the benefit of causing the debug border drawing for accelerated compositing to work properly on Windows.
Comment 1 Brent Fulgham 2015-09-14 17:45:20 PDT
<rdar://problem/22313905>
Comment 2 Brent Fulgham 2015-09-14 20:58:03 PDT
Created attachment 261166 [details]
WIP Patch (Not for review)
Comment 3 Brent Fulgham 2015-09-14 21:54:36 PDT
Created attachment 261171 [details]
Patch v2
Comment 4 Brent Fulgham 2015-09-15 10:17:55 PDT
Created attachment 261201 [details]
Patch
Comment 5 Brent Fulgham 2015-09-15 10:39:34 PDT
Created attachment 261207 [details]
Patch (Correct deprecation warning)
Comment 6 Simon Fraser (smfr) 2015-09-15 11:00:04 PDT
Comment on attachment 261207 [details]
Patch (Correct deprecation warning)

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

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters:-3038
> -    <ClCompile Include="..\dom\ClassNodeList.cpp">
> -      <Filter>dom</Filter>
> -    </ClCompile>

Why did this change?

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters:-3239
> -    <ClCompile Include="..\dom\NodeFilter.cpp">
> -      <Filter>dom</Filter>
> -    </ClCompile>

Ditto.

> Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp:74
> +#if OS(WINDOWS)
> +    if (platformCALayer->layerType() == PlatformCALayer::LayerTypeTiledBackingTileLayer) {
> +        // Tiled layers in Windows have flipped coordinates
> +        CGContextScaleCTM(context, 1, -1);
> +        CGContextTranslateCTM(context, 0, -indicatorBox.size.height);
> +    }
> +#endif

This doesn't seem like the right place for this code.

> Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp:95
>      CGContextSetTextMatrix(context, CGAffineTransformMakeScale(1, -1));
>      CGContextSelectFont(context, "Helvetica", 22, kCGEncodingMacRoman);

I think it would be better to move these lines into PlatformCALayer::drawTextAtPoint().
Comment 7 Brent Fulgham 2015-09-15 11:13:30 PDT
Comment on attachment 261207 [details]
Patch (Correct deprecation warning)

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

>> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters:-3038
>> -    </ClCompile>
> 
> Why did this change?

People update the WebCore.vcxproj file, but usually forget to update WebCore.vcxproj.filters. In this case, "ClassNodeList.cpp" was renamed to "dom\ClassCollection.cpp" in <http://trac.webkit.org/changeset/188735>, but the fact that this file was removed did not get registered until I updated the project file for this change.

>> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters:-3239
>> -    </ClCompile>
> 
> Ditto.

Ditto. The "NodeFilter.cpp" file was deleted in <http://trac.webkit.org/changeset/189230>.

>> Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp:74
>> +#endif
> 
> This doesn't seem like the right place for this code.

Maybe, but this is called directly from TileGrid::platformCALayerPaintContents; I guess I could add the check there, and use the new "flipCoordinates" method at that point.

>> Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp:95
>>      CGContextSelectFont(context, "Helvetica", 22, kCGEncodingMacRoman);
> 
> I think it would be better to move these lines into PlatformCALayer::drawTextAtPoint().

OK!
Comment 8 Brent Fulgham 2015-09-15 11:28:12 PDT
Comment on attachment 261207 [details]
Patch (Correct deprecation warning)

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

>>> Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp:95
>>>      CGContextSelectFont(context, "Helvetica", 22, kCGEncodingMacRoman);
>> 
>> I think it would be better to move these lines into PlatformCALayer::drawTextAtPoint().
> 
> OK!

Haha -- I have to! They are deprecated as well!
Comment 9 Brent Fulgham 2015-09-15 11:29:33 PDT
Created attachment 261218 [details]
Patch v3 (revised for EWS and smfr's comments)
Comment 10 Simon Fraser (smfr) 2015-09-15 11:39:29 PDT
Comment on attachment 261218 [details]
Patch v3 (revised for EWS and smfr's comments)

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

> Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp:-67
> +    CGContextSaveGState(context);
> +
>      indicatorBox.size.width = 12 + 10 * strlen(text);
>      indicatorBox.size.height = 27;
> -    CGContextSaveGState(context);

This change is a no-op.
Comment 11 Brent Fulgham 2015-09-15 11:40:55 PDT
Comment on attachment 261218 [details]
Patch v3 (revised for EWS and smfr's comments)

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

>> Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp:-67
>> -    CGContextSaveGState(context);
> 
> This change is a no-op.

Whoops. I'll revert that before landing.
Comment 12 Brent Fulgham 2015-09-15 12:56:45 PDT
Committed r189821: <http://trac.webkit.org/changeset/189821>