Bug 136319

Summary: Subpixel layout: Rename LayoutRect's device pixel snapping functions.
Product: WebKit Reporter: zalan <zalan>
Component: Layout and RenderingAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 136275    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description zalan 2014-08-27 15:47:53 PDT
to the new naming style of snapRectToDevicePixels, enclosingRectToDevicePixels, snappedIntRect, enclosingIntRect
Comment 1 zalan 2014-08-27 15:53:15 PDT
Created attachment 237267 [details]
Patch
Comment 2 Simon Fraser (smfr) 2014-08-27 16:13:26 PDT
Comment on attachment 237267 [details]
Patch

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

> Source/WebCore/platform/graphics/LayoutRect.h:205
> +WEBCORE_EXPORT IntRect enclosingIntRect(const LayoutRect&);
> +WEBCORE_EXPORT LayoutRect enclosingLayoutRect(const FloatRect&);

Do we want these to be inlined? if so, we should make an exported function that wraps them.

> Source/WebCore/platform/graphics/LayoutRect.h:227
> +FloatRect enclosingRectToDevicePixels(const LayoutRect&, float pixelSnappingFactor);

encloseRectToDevicePixels?
Comment 3 zalan 2014-08-27 20:23:57 PDT
Created attachment 237282 [details]
Patch
Comment 4 zalan 2014-08-27 20:25:34 PDT
wait for bug #136314
Comment 5 zalan 2014-08-27 20:41:37 PDT
Created attachment 237284 [details]
Patch
Comment 6 WebKit Commit Bot 2014-08-27 21:24:45 PDT
Comment on attachment 237284 [details]
Patch

Clearing flags on attachment: 237284

Committed r173047: <http://trac.webkit.org/changeset/173047>
Comment 7 WebKit Commit Bot 2014-08-27 21:24:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Darin Adler 2014-08-29 10:00:23 PDT
(In reply to comment #2)
> encloseRectToDevicePixels?

Sorry I missed the discussion of the new names for all these functions.

I am not in favor of verb phrases to name functions that return values and have no side effects. To me it makes total sense to have a function names like these:

    enclosingRectOnDevicePixelBoundaries
    enclosedRectOnDevicePixelBoundaries

But “enclose rect” doesn’t seem right as a function name to me if what the function does is return a rect that encloses another rect that’s passed in. But this one comment by me won’t substitute for a bit of discussion of terminology. I won’t stand in your way in rationalizing these and removing some of the confusing and duplicate function, but I am not sure I approve of the new naming scheme.