Bug 229776

Summary:

Move ENABLE_RESOLUTION_MEDIA_QUERY to a runtime flag, start running tests again

Product:

WebKit

Reporter:

Sam Sneddon [:gsnedders] <gsnedders>

Component:

CSS

Assignee:

Sam Sneddon [:gsnedders] <gsnedders>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

annulen, eoconnor, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, macpherson, menard, philipj, ryuan.choi, sam, sergio, simon.fraser, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

78087

Attachments:

Description	Flags
Patch	none
Patch	ews-feeder: commit-queue-
Patch	ews-feeder: commit-queue-
Patch	none

Sam Sneddon [:gsnedders]

Reported 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.

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Sam Sneddon [:gsnedders]

Comment 1 2021-09-01 16:40:47 PDT

Created attachment 437083 [details] Patch

Sam Weinig

Comment 2 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?

Sam Sneddon [:gsnedders]

Comment 3 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.

Sam Sneddon [:gsnedders]

Comment 4 2021-09-03 21:02:00 PDT

Created attachment 437334 [details] Patch

Sam Sneddon [:gsnedders]

Comment 5 2021-09-03 21:17:30 PDT

Created attachment 437336 [details] Patch

Sam Sneddon [:gsnedders]

Comment 6 2021-09-03 22:49:31 PDT

Created attachment 437340 [details] Patch

Radar WebKit Bug Importer

Comment 7 2021-09-08 16:31:21 PDT

<rdar://problem/82897639>