Bug 229776 - Move ENABLE_RESOLUTION_MEDIA_QUERY to a runtime flag, start running tests again
Summary: Move ENABLE_RESOLUTION_MEDIA_QUERY to a runtime flag, start running tests again
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Sneddon [:gsnedders]
URL:
Keywords: InRadar
Depends on:
Blocks: 78087
  Show dependency treegraph
 
Reported: 2021-09-01 16:30 PDT by Sam Sneddon [:gsnedders]
Modified: 2021-09-13 14:14 PDT (History)
16 users (show)

See Also:


Attachments
Patch (55.03 KB, patch)
2021-09-01 16:40 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff
Patch (60.92 KB, patch)
2021-09-03 21:02 PDT, Sam Sneddon [:gsnedders]
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (60.94 KB, patch)
2021-09-03 21:17 PDT, Sam Sneddon [:gsnedders]
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (60.96 KB, patch)
2021-09-03 22:49 PDT, Sam Sneddon [:gsnedders]
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Sneddon [:gsnedders] 2021-09-01 16:30:15 PDT
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.
Comment 1 Sam Sneddon [:gsnedders] 2021-09-01 16:40:47 PDT
Created attachment 437083 [details]
Patch
Comment 2 Sam Weinig 2021-09-03 10:03:34 PDT
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?
Comment 3 Sam Sneddon [:gsnedders] 2021-09-03 19:57:23 PDT
(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.
Comment 4 Sam Sneddon [:gsnedders] 2021-09-03 21:02:00 PDT
Created attachment 437334 [details]
Patch
Comment 5 Sam Sneddon [:gsnedders] 2021-09-03 21:17:30 PDT
Created attachment 437336 [details]
Patch
Comment 6 Sam Sneddon [:gsnedders] 2021-09-03 22:49:31 PDT
Created attachment 437340 [details]
Patch
Comment 7 Radar WebKit Bug Importer 2021-09-08 16:31:21 PDT
<rdar://problem/82897639>
Comment 8 EWS 2021-09-13 14:13:59 PDT
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].