Bug 132895

Summary: Subpixel layout: Change Element.offset* client* scroll* return type to double.
Product: WebKit Reporter: zalan <zalan>
Component: DOMAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cmarcelo, commit-queue, esprehn+autocc, gyuyoung.kim, kangil.han, kondapallykalyan, simon.fraser, zcorpan
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description zalan 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.)
Comment 1 zalan 2014-05-13 20:57:42 PDT
Created attachment 231432 [details]
Patch
Comment 2 zalan 2014-05-14 07:29:31 PDT
Created attachment 231450 [details]
Patch
Comment 3 zalan 2014-05-14 08:03:41 PDT
Not sure why the patch does not apply. trying with smaller -expected.txt.
Comment 4 zalan 2014-05-14 08:04:08 PDT
Created attachment 231454 [details]
Patch
Comment 5 zalan 2014-05-14 09:18:15 PDT
oh, it fails on the widows file format fix.
Comment 6 zalan 2014-05-14 10:55:09 PDT
Created attachment 231458 [details]
Patch
Comment 7 zalan 2014-05-14 10:56:20 PDT
windows line ending fixes are in a separate (already pushed) patch.
Comment 8 Simon Fraser (smfr) 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?
Comment 9 zalan 2014-05-14 14:27:07 PDT
Created attachment 231465 [details]
Patch
Comment 10 zalan 2014-05-14 14:27:34 PDT
Comment on attachment 231465 [details]
Patch

EWSing
Comment 11 zalan 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.
Comment 12 WebKit Commit Bot 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>
Comment 13 WebKit Commit Bot 2014-05-14 16:14:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Simon Pieters (:zcorpan) 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
Comment 15 zalan 2014-06-25 18:40:22 PDT
<rdar://problem/17435776> umbrella for the type/value issue.
Comment 16 zalan 2014-07-05 09:13:21 PDT
Going back to integral return values: bug 134651