Bug 158067 – Media queries and platform screen modernization and streamlining

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Darin Adler]

Media queries and platform screen modernization and streamlining

[Darin Adler]

Created attachment 279777 [details]
Patch

[Darin Adler]

Created attachment 279778 [details]
Patch

[Darin Adler]

Created attachment 279860 [details]
Patch

[Darin Adler]

Created attachment 279863 [details]
Patch

[Alex Christensen]

Comment on attachment 279860 [details]
Patch

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

partial review

> Source/WebCore/css/MediaList.cpp:301
> +                if (expression.mediaFeature() == MediaFeatureNames::resolution || expression.mediaFeature() == MediaFeatureNames::maxResolution || expexpression.mediaFeature() == MediaFeatureNames::minResoution) {

expexpression
I guess EFL is the only port to enable RESOLUTION_MEDIA_QUERY

> Source/WebCore/platform/mac/PlatformScreenMac.mm:125
> +    return [window screen] ? : firstScreen();

I've never seen this before.  What does it do if [window screen] is non-nil?

> Source/WebCore/platform/mac/PlatformScreenMac.mm:134
> -    return nil;
> +    return firstScreen();

Is this a subtle change in behavior?

[Darin Adler]

Comment on attachment 279860 [details]
Patch

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

>> Source/WebCore/platform/mac/PlatformScreenMac.mm:125
>> +    return [window screen] ? : firstScreen();
> 
> I've never seen this before.  What does it do if [window screen] is non-nil?

Returns the value of "[window screen]"; it behaves exactly like the || operator in perl or another way to put it is that "a ?: b" is the same as "a ? a : b" except that it only evaluates a once.

We don’t use it in cross platform code, probably because it’s not standard C++, but it has been supported by gcc and clang since before the WebKit project even existed.

I had thought use of this was common in WebKit, but it turns out that I can only find two uses of it, in WebKit2/UIProcess/API/Cocoa/WKWebView.mm and in WebKit2/UIProcess/ios/WKContentViewInteraction.mm; in both cases it’s written "?:" rather than "? :" with a space. I would be happy to just use a local variable instead if you prefer.

>> Source/WebCore/platform/mac/PlatformScreenMac.mm:134
>> +    return firstScreen();
> 
> Is this a subtle change in behavior?

It is a change; here is why it is OK. There were only two call sites for this function:

1) effectiveMousePositionForSelectionAutoscroll in EventHandlerMac.mm called this function and did not check for nil.  There was no real chance that call site would ever get nil — unclear how the screen could disappear before an event from that screen is handled — so the code change doesn’t affect any real case. The code after this point uses the returned NSScreen * and calls the "frame" method; calling that on a nil object will return something unpredictable and so returning an actual object clearly would not do harm, but it’s not really all that helpful to discuss it since it’s really untestable dead code.

2) The function in this file named screenForWidget called this function, and in the old version of that function, if it returned nil, it would call screenForWindow(nil), which was a way to get the pointer to the first screen, as the comment indicated. The new version of screenForWidget, now an overload of screen that takes a Widget*, continues to return the first screen in that case, but now it does so as a result of the fact that all functions named screen, including this one, return that rather than nil.

[Darin Adler]

Created attachment 279889 [details]
Patch

[Darin Adler]

Hoping to get a review and land this soon. While there is more work to be done in that area, this should cut down on the number of memory allocations when dealing with media queries and possibly cut down a bit on total memory used for the queries. Who knows, on pages that use a ton of queries it might even “move the needle” a tiny bit.

[Alex Christensen]

Comment on attachment 279889 [details]
Patch

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

200k patches aren't easy to review

> Source/WebCore/css/CSSGrammar.y.in:655
> -    media_query_exp_list {
> -        $$ = new MediaQuery(MediaQuery::None, "all", std::unique_ptr<Vector<std::unique_ptr<MediaQueryExp>>>($1));
> +    media_query_expression_list {
> +        $$ = new MediaQuery(MediaQuery::None, "all", WTFMove(*$1));
> +        delete $1;

Generated code with this change:
BEFORE:
        (yyval.mediaQuery) = new MediaQuery(MediaQuery::None, "all", std::unique_ptr<Vector<std::unique_ptr<MediaQueryExp>>>((yyvsp[(1) - (1)].mediaQueryExpList)));
AFTER
        (yyval.mediaQuery) = new MediaQuery(MediaQuery::None, "all", WTFMove(*(yyvsp[(1) - (1)].mediaQueryExpressionList)));
        delete (yyvsp[(1) - (1)].mediaQueryExpressionList);
mediaQueryExpressionList is a Vector<MediaQueryExpression>* in the union.  I'm going to need a minute to wrap my head around why these deletes are now necessary when they weren't before, where the corresponding new is, and why they are not double deleting anything.

[Alex Christensen]

Comment on attachment 279889 [details]
Patch

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

> Source/WebCore/css/CSSGrammar.y.in:620
> +        $$ = new Vector<MediaQueryExpression>;

We allocated here with new, so delete is necessary to delete the sizeof(Vector<MediaQueryExpression>) bytes we allocate here.  WTFMoving just sets these bytes to 0, and they still need to be freed.  Before we were adopting this with a std::unique_ptr constructor and storing the std::unique_ptr<ExpressionVector> in MediaQuery, but now MediaQuery just stores the Vector.
Ok, I think I know what's going on here now.

[Alex Christensen]

Comment on attachment 279889 [details]
Patch

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

> Source/WebCore/css/MediaFeatureNames.h:83
> +namespace MediaFeatureNames {

I agree, a namespace is nicer than putting MediaFeature in the name.

> Source/WebCore/css/MediaList.cpp:144
> +        String medium = stripLeadingAndTrailingHTMLSpaces(listString);

Is stripLeadingAndTrailingHTMLSpaces faster than stripWhiteSpace?  Is there a guarantee that there will be no internal whitespace?

> Source/WebCore/css/MediaQueryEvaluator.cpp:203
> +    return compareValue(width * aspectRatio.denominatorValue(), height * aspectRatio.numeratorValue(), op);

This used to do integer comparison and now it does float comparison.  This might cause some things that used to be considered equal to not be considered equal any more.

> Source/WebCore/css/MediaQueryEvaluator.cpp:233
> +    return compareValue(bitsPerComponent, numericValue.value(), op);

Ditto

> Source/WebCore/css/MediaQueryList.cpp:61
> -void MediaQueryList::removeListener(PassRefPtr<MediaQueryListListener> listener)
> +void MediaQueryList::removeListener(RefPtr<MediaQueryListListener>&& listener)

This should take a MediaQueryListListener*

[Darin Adler]

Comment on attachment 279889 [details]
Patch

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

Thank you so much for the review!

>> Source/WebCore/css/MediaList.cpp:144
>> +        String medium = stripLeadingAndTrailingHTMLSpaces(listString);
> 
> Is stripLeadingAndTrailingHTMLSpaces faster than stripWhiteSpace?  Is there a guarantee that there will be no internal whitespace?

Yes, stripLeadingAndTrailingHTMLSpaces is faster, but that is not why I am using it (see below).

Both functions strip only leading and trailing spaces, although the name of the stripWhiteSpace function does not make that as clear. Neither function strips internal whitespace.

The reason I am using this is that stripWhiteSpace strips many characters that it should not strip. Virtually every place in HTML, CSS, DOM, and other specifications that relies on stripping spaces call for stripping only the particular few ASCII characters that HTML calls spaces. Not all the other Unicode whitespace. It’s correctness that has me changing this. Technically it’s a change in behavior for some exotic whitespace characters to stop stripping them. I suppose I could add a couple test cases for this. Maybe after landing the patch (I’ve done that kind of thing before).

I don’t think there are *any* correct uses of stripWhiteSpace in WebKit.

>> Source/WebCore/css/MediaQueryEvaluator.cpp:203
>> +    return compareValue(width * aspectRatio.denominatorValue(), height * aspectRatio.numeratorValue(), op);
> 
> This used to do integer comparison and now it does float comparison.  This might cause some things that used to be considered equal to not be considered equal any more.

True. The old code also used to do integer multiplication that could overflow. That could also change the result of some comparisons.

>> Source/WebCore/css/MediaQueryEvaluator.cpp:233
>> +    return compareValue(bitsPerComponent, numericValue.value(), op);
> 
> Ditto

Yes, absolutely.

But I think it’s even more unlikely than above that the floating point comparison will give a different result here in any important case. I suppose we’ll get different results if someone specifies a floating point value and before relied on integer truncation to make it equal to something. Like asking if this is 8.5 bits per component and getting a yes.

>> Source/WebCore/css/MediaQueryList.cpp:61
>> +void MediaQueryList::removeListener(RefPtr<MediaQueryListListener>&& listener)
> 
> This should take a MediaQueryListListener*

You are right. The issues are similar to the issues with removeEventListener. I would like to return here and improve this further, but I would also like to leave this as-is for now. I don’t want to change how the bindings are generated in this patch.

[WebKit Commit Bot]

Comment on attachment 279889 [details]
Patch

Clearing flags on attachment: 279889

Committed r201441: <http://trac.webkit.org/changeset/201441>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.