Bug 132911 - sourceFrameOnScreenForShareItem: can be off by a pixel
Summary: sourceFrameOnScreenForShareItem: can be off by a pixel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-05-14 09:48 PDT by Brady Eidson
Modified: 2014-05-14 12:34 PDT (History)
3 users (show)

See Also:


Attachments
Patch v1 (6.30 KB, patch)
2014-05-14 10:22 PDT, Brady Eidson
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2014-05-14 09:48:26 PDT
sourceFrameOnScreenForShareItem: can be off by a pixel

This is due largely to sub pixel layout and conversions between FloatRects and IntRects.

In radar at <rdar://problem/16878020>
Comment 1 Brady Eidson 2014-05-14 10:22:26 PDT
Created attachment 231456 [details]
Patch v1

This splits off the client rect calculation into a utility method (which will also be used in bug 132912), and that method is guaranteed to keep everything in terms of floats.

Only a single rounding conversion from Float->Int rect takes place.

This fixes the known cases of off-by-one-pixel errors.
Comment 2 Simon Fraser (smfr) 2014-05-14 10:38:07 PDT
Comment on attachment 231456 [details]
Patch v1

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

> Source/WebKit/mac/WebCoreSupport/WebContextMenuClient.mm:404
> +        return NSRect();

Isn't there an NSEmptyRect or similar? I'm not sure this is constructing an empty rect.

> Source/WebKit/mac/WebCoreSupport/WebContextMenuClient.mm:409
> +        return NSRect();

Ditto.

> Source/WebKit/mac/WebCoreSupport/WebContextMenuClient.mm:414
> +    // FIXME: Ideally we'd like to convert the content rect to screen coordinates
> +    // without the lossy float -> int conversion.
> +    // Creating a rounded int rect works well in practice, but might still lead to
> +    // off-by-one-pixel problems in edge cases.

You should file a bugzilla/radar to track this and reference it here.
Comment 3 Brady Eidson 2014-05-14 11:03:30 PDT
http://trac.webkit.org/changeset/168847
Comment 4 Darin Adler 2014-05-14 12:34:02 PDT
Comment on attachment 231456 [details]
Patch v1

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

>> Source/WebKit/mac/WebCoreSupport/WebContextMenuClient.mm:404
>> +        return NSRect();
> 
> Isn't there an NSEmptyRect or similar? I'm not sure this is constructing an empty rect.

NSZeroRect