Bug 132895 – Subpixel layout: Change Element.offset* client* scroll* return type to double.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[zalan]

and return subpixel values (except for scroll*, scroll functions still return integral values as the corresponding classes haven't transitioned to subpixel values yet.)

[zalan]

Created attachment 231432 [details]
Patch

[zalan]

Created attachment 231450 [details]
Patch

[zalan]

Not sure why the patch does not apply. trying with smaller -expected.txt.

[zalan]

Created attachment 231454 [details]
Patch

[zalan]

oh, it fails on the widows file format fix.

[zalan]

Created attachment 231458 [details]
Patch

[zalan]

windows line ending fixes are in a separate (already pushed) patch.

[Simon Fraser (smfr)]

Comment on attachment 231458 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        subpixelCSSOMElementMetricsEnabled setting is added to be able to turn this feature on/off
> +        from Safari's debug menu. It toggles the return value from subpixel to floored integral.

Not just from mainFrameView.contentsToRootView, by via WK2 prefs too,

> Source/WebCore/dom/Element.cpp:649
>      float zoomFactor = localZoomForRenderer(renderer);

Use a double to avoid float -> double promotion at all the / zoomFactor sites?

> Source/WebCore/dom/Element.cpp:749
> +        return convertToNonSubpixelValueIfNeeded(adjustLayoutUnitForAbsoluteZoom(renderer->clientTop(), *renderer).toDouble(), renderer->document());
> +#else
> +        return adjustForAbsoluteZoom(renderer->clientTop(), *renderer);

Do we need both kinds of adjust*forAbsoluteZoom?

> Source/WebCore/page/Settings.in:197
> +subpixelCSSOMElementMetricsEnabled initial=true

I wonder if we should expose this through WebKit1 pref too?

[zalan]

Created attachment 231465 [details]
Patch

[zalan]

Comment on attachment 231465 [details]
Patch

EWSing

[zalan]

(In reply to comment #8)
> (From update of attachment 231458 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231458&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        subpixelCSSOMElementMetricsEnabled setting is added to be able to turn this feature on/off
> > +        from Safari's debug menu. It toggles the return value from subpixel to floored integral.
> 
> Not just from mainFrameView.contentsToRootView, by via WK2 prefs too,
Done.

> 
> > Source/WebCore/dom/Element.cpp:649
> >      float zoomFactor = localZoomForRenderer(renderer);
> 
> Use a double to avoid float -> double promotion at all the / zoomFactor sites?
Done.

> 
> > Source/WebCore/dom/Element.cpp:749
> > +        return convertToNonSubpixelValueIfNeeded(adjustLayoutUnitForAbsoluteZoom(renderer->clientTop(), *renderer).toDouble(), renderer->document());
> > +#else
> > +        return adjustForAbsoluteZoom(renderer->clientTop(), *renderer);
> 
> Do we need both kinds of adjust*forAbsoluteZoom?
Part of the cleanup work I'll be doing once the subpixel buildtime flag is removed. 

> 
> > Source/WebCore/page/Settings.in:197
> > +subpixelCSSOMElementMetricsEnabled initial=true
> 
> I wonder if we should expose this through WebKit1 pref too?
Not sure, I'll do that if needed.

[WebKit Commit Bot]

Comment on attachment 231465 [details]
Patch

Clearing flags on attachment: 231465

Committed r168868: <http://trac.webkit.org/changeset/168868>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Pieters (:zcorpan)]

One datapoint about compat impact for MouseEvents: https://www.w3.org/Bugs/Public/show_bug.cgi?id=24159#c2

[zalan]

<rdar://problem/17435776> umbrella for the type/value issue.

[zalan]

Going back to integral return values: bug 134651