Bug 166487 – Subpixel rendering: Make list marker painting subpixel aware.

• Status: ASSIGNED
• Component: Layout and Rendering

[alan]

see https://bug-163461-attachments.webkit.org/attachment.cgi?id=297759 (patch -> bug 163461)

[alan]

Created attachment 297786 [details]
Patch

[Build Bot]

Comment on attachment 297786 [details]
Patch

Attachment 297786 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2795260

New failing tests:
fast/lists/drag-into-marker.html

[Build Bot]

Created attachment 297787 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 297786 [details]
Patch

Attachment 297786 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2795263

New failing tests:
fast/lists/drag-into-marker.html

[Build Bot]

Created attachment 297788 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 297786 [details]
Patch

Attachment 297786 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2795264

New failing tests:
tables/mozilla/marvin/backgr_index.html
tables/mozilla_expected_failures/marvin/backgr_fixed-bg.html
tables/mozilla/marvin/backgr_layers-opacity.html
fast/lists/drag-into-marker.html
editing/unsupported-content/list-type-after.html
tables/mozilla_expected_failures/marvin/backgr_layers-show.html

[Build Bot]

Created attachment 297789 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[alan]

Created attachment 297790 [details]
Patch

[Darin Adler]

Comment on attachment 297790 [details]
Patch

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

I’m not sure why we want all these wider sizes. What’s the underlying logic to why it’s better?

> Source/WebCore/rendering/RenderListMarker.cpp:1198
> +    // Use roundPointToDevicePixels() for both location and size instead of snapRectToDevicePixels so that we always end up with
> +    // a square repaint rect (snapRectToDevicePixels's size snapping is location dependent, so snapped height == width is not guaranteed).
> +    auto markerRectToPaint = LayoutRect(roundPointToDevicePixels(markerRect.location(), deviceScaleFactor),
> +        toFloatSize(roundPointToDevicePixels(toLayoutPoint(markerRect.size()), deviceScaleFactor)));

It is good to round the size rather than snapping it for the reason you give here. But it is unclear why we are rounding the location instead of snapping it. Maybe there is no such thing as “snap” for location?

I think we should write a helper function for this operation rather than writing this expression out; maybe call it roundRectToDevicePixels, or think through what this operation should be called.

It seems strange to call toFloatSize on the size but we don’t need something similar for the location. What’s going on there?

> Source/WebCore/rendering/RenderListMarker.cpp:1208
>              LayoutRect selRect = localSelectionRect();

We should rename this to selectionRect instead of selRect.

> Source/WebCore/rendering/RenderListMarker.cpp:1216
>          LayoutRect selRect = localSelectionRect();

We should rename this to selectionRect instead of selRect.

> Source/WebCore/rendering/RenderListMarker.cpp:1330
> +        markerRect.moveBy(LayoutPoint(box.x(), box.y() - logicalHeight()));

Do we need to write the type LayoutPoint explicitly? Can we just write this instead?

    markerRect.moveBy({ box.x(), box.y() - logicalHeight() });

Maybe not. Unclear to me what the types involved are.

> Source/WebCore/rendering/RenderListMarker.cpp:1560
> +            logicalWidth = font.fontMetrics().floatAscent() / 3 + 2;

I don’t understand what the 2 represents here. Why are we adding two?

> Source/WebCore/rendering/RenderListMarker.cpp:1783
> +            // Position the center of the rect just a bit below the midpoint of the ascent.

Why just a bit below the midpoint of the ascent? Why 3/5 instead of 1/2?

> Source/WebCore/rendering/RenderListMarker.h:83
> -    FloatRect getRelativeMarkerRect();
> +    LayoutRect getRelativeMarkerRect();

Function should not have "get" in its name in WebKit coding style.