Bug 136319 - Subpixel layout: Rename LayoutRect's device pixel snapping functions.
Summary: Subpixel layout: Rename LayoutRect's device pixel snapping functions.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords:
Depends on:
Blocks: 136275
  Show dependency tree / graph
 
Reported: 2014-08-27 15:47 PDT by zalan
Modified: 2014-08-29 10:00 PDT (History)
3 users (show)

See Also:

Attachments
Patch (142.64 KB, patch)
2014-08-27 15:53 PDT, zalan 		no flags Details | Formatted Diff | Diff
Patch (142.79 KB, patch)
2014-08-27 20:23 PDT, zalan 		no flags Details | Formatted Diff | Diff
Patch (142.85 KB, patch)
2014-08-27 20:41 PDT, zalan 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.