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.
Created attachment 437083 [details] Patch
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?
(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.
Created attachment 437334 [details] Patch
Created attachment 437336 [details] Patch
Created attachment 437340 [details] Patch
<rdar://problem/82897639>
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].
*** Bug 229778 has been marked as a duplicate of this bug. ***