Bug 77263 - PlatformScreenMac should not rely on NSWindow for important bits of data
Summary: PlatformScreenMac should not rely on NSWindow for important bits of data
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-01-27 16:44 PST by Beth Dakin
Modified: 2012-01-30 13:45 PST (History)
4 users (show)

See Also:


Attachments
Patch (20.86 KB, patch)
2012-01-27 17:14 PST, Beth Dakin
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2012-01-27 16:44:05 PST
PlatformScreenMac assumes it can reliably get an NSWindow from the platformWidget(), which is not true in WebKit2. It should be updated so that these functions work properly in WebKit2.

<rdar://problem/10741654>
Comment 1 Beth Dakin 2012-01-27 17:14:44 PST
Created attachment 124405 [details]
Patch
Comment 2 Geoffrey Garen 2012-01-30 11:15:31 PST
Comment on attachment 124405 [details]
Patch 

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

r=me

> Source/WebCore/platform/PlatformScreen.h:59
> +    FloatRect toUserSpace(const NSRect&, NSWindow *destination, float deviceScaleFactor);
> +    NSRect toDeviceSpace(const FloatRect&, NSWindow *source, float deviceScaleFactor);

Follow-up bug: It looks like these functions will do the wrong thing in WebKit2 in the case of multiple screens where the window is not on the primary screen, since the NSWindow argument will be NULL.
Comment 3 Beth Dakin 2012-01-30 11:48:53 PST
Thanks! I will definitely file a follow-up bug for the remaining issue. Committed change with http://trac.webkit.org/changeset/106271
Comment 4 Adam Roben (:aroben) 2012-01-30 12:17:22 PST
Comment on attachment 124405 [details]
Patch 

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

> Source/WebCore/platform/PlatformScreen.h:43
> +    class FrameView;

It's a layering violation for anything in Source/WebCore/platform to know about FrameView. I'm not sure what the best fix is though.
Comment 5 Beth Dakin 2012-01-30 12:29:53 PST
(In reply to comment #4)
> (From update of attachment 124405 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124405&action=review
> 
> > Source/WebCore/platform/PlatformScreen.h:43
> > +    class FrameView;
> 
> It's a layering violation for anything in Source/WebCore/platform to know about FrameView. I'm not sure what the best fix is though.


We could add a virtual function on Widget to return the deviceScaleFactor. It would always return 1 from Widget.h, but FrameViews could fetch the real deviceScaleFactor. That would totally fix the layering violation, but it's a shame that it's such a circuitous path.
Comment 6 Beth Dakin 2012-01-30 13:45:36 PST
32-bit build fix: http://trac.webkit.org/changeset/106286