Bug 56482 – Make CSSMedia queries pass through Chrome and ChromeClient

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Robert Hogan]

Make CSSMedia queries pass through Chrome and ChromeClient

[Robert Hogan]

Created attachment 85963 [details]
Patch

[Eric Seidel (no email)]

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?

[Robert Hogan]

Created attachment 86050 [details]
Patch

[Robert Hogan]

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.

[Robert Hogan]

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.

[Robert Hogan]

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.

[Eric Seidel (no email)]

Comment on attachment 86050 [details]
Patch

So is this only for fighting fingerprinting?  Shouldn't anti-fingerprinting measures be in WebCore directly?

[Robert Hogan]

(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

[Robert Hogan]

Created attachment 86217 [details]
Patch

[Adam Barth]

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?

[Eric Seidel (no email)]

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?