Bug 217300 – Remove support for enabling subpixel CSSOM values, it's off by default everywhere and known to be not-compatible with the web

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Remove support for enabling subpixel CSSOM values, it's off by default everywhere and known to be not-compatible with the web

[Sam Weinig]

Created attachment 410484 [details]
Patch

[Sam Weinig]

Created attachment 410488 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 410488 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:1057
> +    result.adjustedValue = result.zoomFactor == 1 ? value.toDouble() : value.toDouble() / result.zoomFactor;

Surely just dividing by 1 is faster than the test.

> Source/WebCore/dom/Element.cpp:1082
> +    LayoutUnit offsetLeft = LayoutUnit(roundToInt(offset));

auto offsetLeft = LayoutUnit { roundToInt(offset) } ?

> Source/WebCore/dom/Element.cpp:1157
> +        LayoutUnit offsetWidth = LayoutUnit(roundToInt(renderer->offsetWidth()));

Ditto

> Source/WebCore/dom/Element.cpp:1167
> +        LayoutUnit offsetHeight = LayoutUnit(roundToInt(renderer->offsetHeight()));

Ditto

> Source/WebCore/dom/Element.cpp:1200
> +        LayoutUnit clientLeft = LayoutUnit(roundToInt(renderer->clientLeft()));

ditto

> Source/WebCore/dom/Element.cpp:1211
> +        LayoutUnit clientTop = LayoutUnit(roundToInt(renderer->clientTop()));

ditto

> Source/WebCore/dom/Element.cpp:1234
>          // clientWidth/Height is the visual portion of the box content, not including

ditto

> Source/WebCore/dom/Element.cpp:1266
> +        LayoutUnit clientHeight = LayoutUnit(roundToInt(renderer->clientHeight()));

ditto

[Darin Adler]

Comment on attachment 410488 [details]
Patch

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

>> Source/WebCore/dom/Element.cpp:1057
>> +    result.adjustedValue = result.zoomFactor == 1 ? value.toDouble() : value.toDouble() / result.zoomFactor;
> 
> Surely just dividing by 1 is faster than the test.

I’m not sure of that. We may need to benchmark it. I know that floating point division is both costly and hard for the compiler to optimize. And modern CPUs are really good at optimizing branches.

[Sam Weinig]

Created attachment 410518 [details]
Patch

[Sam Weinig]

(In reply to Darin Adler from comment #4)
> Comment on attachment 410488 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410488&action=review
> 
> >> Source/WebCore/dom/Element.cpp:1057
> >> +    result.adjustedValue = result.zoomFactor == 1 ? value.toDouble() : value.toDouble() / result.zoomFactor;
> > 
> > Surely just dividing by 1 is faster than the test.
> 
> I’m not sure of that. We may need to benchmark it. I know that floating
> point division is both costly and hard for the compiler to optimize. And
> modern CPUs are really good at optimizing branches.

I am also not sure, but with some manual inlining of the helper which was only being called by this function, the choice goes away, since we still need to branch on zoomFactor to determine the rounding mode:

static int adjustOffsetForZoomAndSubpixelLayout(RenderBoxModelObject& renderer, const LayoutUnit& offset)
{
    auto offsetLeft = LayoutUnit(roundToInt(offset));
    double zoomFactor = localZoomForRenderer(renderer);
    if (zoomFactor == 1)
        return convertToNonSubpixelValue(offsetLeft, Floor);
    return convertToNonSubpixelValue(offsetLeft / zoomFactor, Round);
}

[Sam Weinig]

Created attachment 410520 [details]
Patch

[EWS]

Committed r267970: <https://trac.webkit.org/changeset/267970>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410520 [details].

[Radar WebKit Bug Importer]

<rdar://problem/69956883>