Bug 56482

Summary: Make CSSMedia queries pass through Chrome and ChromeClient
Product: WebKit Reporter: Robert Hogan <robert>
Component: New BugsAssignee: Robert Hogan <robert>
Status: RESOLVED WONTFIX    
Severity: Normal CC: abarth, cmarcelo, hyatt, mikeperry-webkit, robert
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 50895, 56678    
Attachments:
Description Flags
Patch
none
Patch
none
Patch eric: review-

Description Robert Hogan 2011-03-16 13:22:44 PDT
Make CSSMedia queries pass through Chrome and ChromeClient
Comment 1 Robert Hogan 2011-03-16 13:28:36 PDT
Created attachment 85963 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-03-17 04:28:47 PDT
Comment on attachment 85963 [details]
Patch

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

Why?  Please explain in your ChangeLog.

> Source/WebCore/css/MediaQueryEvaluator.cpp:-232
> -    if (!screenIsMonochrome(frame->page()->mainFrame()->view())) {

what happens to the screenIsMonochrome function?  Is it being removed?
Comment 3 Robert Hogan 2011-03-17 05:54:48 PDT
Created attachment 86050 [details]
Patch
Comment 4 Robert Hogan 2011-03-17 07:35:14 PDT
Comment on attachment 86050 [details]
Patch

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

> Source/WebCore/css/MediaQueryEvaluator.cpp:232
> +    if (!frame->page()->chrome()->screenIsMonochrome()) {

No, it's not removed. It's called from Chrome instead.
Comment 5 Robert Hogan 2011-03-18 11:59:53 PDT
Comment on attachment 86050 [details]
Patch

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

> Source/WebCore/css/MediaQueryEvaluator.cpp:257
> +        FloatRect sg = frame->page()->chrome()->pageRect();

Note to self: this should be windowRect() I think.

> Source/WebCore/css/MediaQueryEvaluator.cpp:332
> +    FloatRect sg = frame->page()->chrome()->pageRect();

Note to self: this should be windowRect() I think.
Comment 6 Robert Hogan 2011-03-18 12:14:31 PDT
Comment on attachment 86050 [details]
Patch

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

> Source/WebCore/page/ChromeClient.h:86
> +        int screenDepthPerComponent(int bitsPerComponent) { return bitsPerComponent; }
> +        bool screenIsMonochrome(bool isMonochrome) { return isMonochrome; }
> +        FloatRect screenRect(FloatRect rect) { return rect; }
> +        FloatRect screenAvailableRect(FloatRect rect) { return rect; }

Oh, these should be virtual.
Comment 7 Eric Seidel (no email) 2011-03-18 14:33:00 PDT
Comment on attachment 86050 [details]
Patch

So is this only for fighting fingerprinting?  Shouldn't anti-fingerprinting measures be in WebCore directly?
Comment 8 Robert Hogan 2011-03-18 14:43:28 PDT
(In reply to comment #7)
> (From update of attachment 86050 [details])
> So is this only for fighting fingerprinting?  Shouldn't anti-fingerprinting measures be in WebCore directly?

It isn't always the right place for them. 

See

https://trac.webkit.org/wiki/Fingerprinting#CreatingaCommonFingerprint

A browser needs the flexibility to create a common fingerprint for it's users - WebCore isn't always best placed to decide what this fingerprint should be.

I think the 'bare metal' properties of the screen and viewport inspected by CSS media queries in PlatformScreen are a good example. A browser will often have to make a trade-off between user experience and a relatively common set of values for screen/chrome/window dimensions - it ensures these are respected by the Screen and Window DOM objects, and it needs to do the same with CSS media queries.

A good example of this is in the Torbutton design document referenced here:

https://trac.webkit.org/wiki/Fingerprinting#WindowObject
Comment 9 Robert Hogan 2011-03-18 14:56:03 PDT
Created attachment 86217 [details]
Patch
Comment 10 Adam Barth 2011-03-30 11:50:23 PDT
Comment on attachment 86217 [details]
Patch

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

> Source/WebCore/css/MediaQueryEvaluator.cpp:222
> -    int bitsPerComponent = screenDepthPerComponent(frame->page()->mainFrame()->view());
> +    int bitsPerComponent = frame->page()->chrome()->screenDepthPerComponent();

What if page is null?
Comment 11 Eric Seidel (no email) 2011-05-23 14:49:14 PDT
Comment on attachment 86217 [details]
Patch

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

I disagree with this approach.  If we are doing this for fingerprinting avoidance, we should write the anti-fingerprinting code in WebCore instead of piping everything through WebKit.

>> Source/WebCore/css/MediaQueryEvaluator.cpp:222
>> +    int bitsPerComponent = frame->page()->chrome()->screenDepthPerComponent();
> 
> What if page is null?

frame->page() can only be null during teardown, or?