Bug 132895

Summary:

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

Product:

WebKit

Reporter:

alan <zalan>

Component:

DOM

Assignee:

alan <zalan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, cmarcelo, commit-queue, esprehn+autocc, gyuyoung.kim, kangil.han, kondapallykalyan, simon.fraser, zcorpan

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

alan

Reported 2014-05-13 19:39:02 PDT

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

Attachments
Patch (428.99 KB, patch) 2014-05-13 20:57 PDT, alan	no flags	Details Formatted Diff Diff
Patch (429.31 KB, patch) 2014-05-14 07:29 PDT, alan	no flags	Details Formatted Diff Diff
Patch (128.45 KB, patch) 2014-05-14 08:04 PDT, alan	no flags	Details Formatted Diff Diff
Patch (104.96 KB, patch) 2014-05-14 10:55 PDT, alan	no flags	Details Formatted Diff Diff
Patch (105.13 KB, patch) 2014-05-14 14:27 PDT, alan	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

alan

Comment 1 2014-05-13 20:57:42 PDT

Created attachment 231432 [details] Patch

alan

Comment 2 2014-05-14 07:29:31 PDT

Created attachment 231450 [details] Patch

alan

Comment 3 2014-05-14 08:03:41 PDT

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

alan

Comment 4 2014-05-14 08:04:08 PDT

Created attachment 231454 [details] Patch

alan

Comment 5 2014-05-14 09:18:15 PDT

oh, it fails on the widows file format fix.

alan

Comment 6 2014-05-14 10:55:09 PDT

Created attachment 231458 [details] Patch

alan

Comment 7 2014-05-14 10:56:20 PDT

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

Simon Fraser (smfr)

Comment 8 2014-05-14 13:39:13 PDT

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?

alan

Comment 9 2014-05-14 14:27:07 PDT

Created attachment 231465 [details] Patch

alan

Comment 10 2014-05-14 14:27:34 PDT

Comment on attachment 231465 [details] Patch EWSing

alan

Comment 11 2014-05-14 14:29:39 PDT

(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 12 2014-05-14 16:14:26 PDT

Comment on attachment 231465 [details] Patch Clearing flags on attachment: 231465 Committed r168868: <http://trac.webkit.org/changeset/168868>

WebKit Commit Bot

Comment 13 2014-05-14 16:14:35 PDT

All reviewed patches have been landed. Closing bug.

Simon Pieters (:zcorpan)

Comment 14 2014-05-19 22:27:32 PDT

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

alan

Comment 15 2014-06-25 18:40:22 PDT

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

alan

Comment 16 2014-07-05 09:13:21 PDT

Going back to integral return values: bug 134651

Note You need to log in before you can comment on or make changes to this bug.