Bug 229776 – Move ENABLE_RESOLUTION_MEDIA_QUERY to a runtime flag, start running tests again

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

[Sam Sneddon [:gsnedders]]

We haven't compiled with this enabled since Blink forked, so it's likely that it doesn't quite compile and that the tests don't all pass.

Given there's no reason for this to be a compile-time switch, let's make this a run-time switch (which also means we don't regress compiling!) and make sure the tests aren't skipped.

[Sam Sneddon [:gsnedders]]

Created attachment 437083 [details]
Patch

[Sam Weinig]

Comment on attachment 437083 [details]
Patch

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

> Source/WebCore/css/MediaList.cpp:235
> +    String message = String(mediaQueryMessage.get());

This should be writable as `String message = mediaQueryMessage`; No need for the explicit String constructor call.

That said, this whole thing using replace seems very unnecessary. I would just use makeString() for both of them. Something along the lines of:

        ASSERT(value.isDotsPerInch() || value.isDotsPerCentimeter());
        auto replacementUnit = value.isDotsPerInch() ? "dpi" : "dpcm";
        auto lengthUnit = value.isDotsPerInch() ? "inch" : "centimeter";

        auto message = makeString("Consider using 'dppx' units instead of '", replacementUnit, "', as in CSS '", replacementUnit, "' means dots-per-CSS-", lengthUnit, ", not dots-per-physical-", lengthUnit, ", so does not correspond to the actual '", replacementUnit, "' of a screen. In media query expression: ", serializedExpression);

> LayoutTests/ChangeLog:9
> +        Rebaseline.

Why are these failing now?

[Sam Sneddon [:gsnedders]]

(In reply to Sam Weinig from comment #2)
> Comment on attachment 437083 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437083&action=review
> 
> > Source/WebCore/css/MediaList.cpp:235
> > +    String message = String(mediaQueryMessage.get());
> 
> This should be writable as `String message = mediaQueryMessage`; No need for
> the explicit String constructor call.
> 
> That said, this whole thing using replace seems very unnecessary. I would
> just use makeString() for both of them. Something along the lines of:
> 
>         ASSERT(value.isDotsPerInch() || value.isDotsPerCentimeter());
>         auto replacementUnit = value.isDotsPerInch() ? "dpi" : "dpcm";
>         auto lengthUnit = value.isDotsPerInch() ? "inch" : "centimeter";
> 
>         auto message = makeString("Consider using 'dppx' units instead of
> '", replacementUnit, "', as in CSS '", replacementUnit, "' means
> dots-per-CSS-", lengthUnit, ", not dots-per-physical-", lengthUnit, ", so
> does not correspond to the actual '", replacementUnit, "' of a screen. In
> media query expression: ", serializedExpression);

Yeah, I don't disagree; was just trying to keep changes beyond re-enabling the feature minimal. But I guess I can do that.

> 
> > LayoutTests/ChangeLog:9
> > +        Rebaseline.
> 
> Why are these failing now?

I was trying to land this with minimal changes beyond re-enabling the flag. Digging into this now, though got a bit held up by our sheer lack of documentation of any of internals.* or testRunner.* and struggling to find appropriate APIs. My suspicion it only ever passed on the Chromium port.

[Sam Sneddon [:gsnedders]]

Created attachment 437334 [details]
Patch

[Sam Sneddon [:gsnedders]]

Created attachment 437336 [details]
Patch

[Sam Sneddon [:gsnedders]]

Created attachment 437340 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/82897639>

[EWS]

Committed r282357 (241620@main): <https://commits.webkit.org/241620@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437340 [details].

[Sam Sneddon [:gsnedders]]

*** Bug 229778 has been marked as a duplicate of this bug. ***