Skip to content

Commit

Permalink
Remove sorting and de-duplication in media queries.
Browse files Browse the repository at this point in the history 
This was removed from the spec per resolution[1] and incompatible with
never media queries. Improves interop with Gecko which have not seen any
issues with the different serialization.

Removed fast/media test which is covered by existing wpt tests.

[1] w3c/csswg-drafts#5627 (comment)

Bug: 1138859
Change-Id: I1483008c81df90f8277dcad7e90c8036c5cc019b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2478992
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819090}
  • Loading branch information
Rune Lillesveen authored and Commit Bot committed Oct 20, 2020
1 parent ba2e367 commit 99589ad
Show file tree
Hide file tree
Showing 7 changed files with 47 additions and 91 deletions.
19 changes: 1 addition & 18 deletions third_party/blink/renderer/core/css/media_query.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,6 @@ String MediaQuery::Serialize() const {
return result.ToString();
}

static bool ExpressionCompare(const MediaQueryExp& a, const MediaQueryExp& b) {
return CodeUnitCompare(a.Serialize(), b.Serialize()) < 0;
}

std::unique_ptr<MediaQuery> MediaQuery::CreateNotAll() {
return std::make_unique<MediaQuery>(MediaQuery::kNot, media_type_names::kAll,
ExpressionHeapVector());
Expand All @@ -83,20 +79,7 @@ MediaQuery::MediaQuery(RestrictorType restrictor,
ExpressionHeapVector expressions)
: restrictor_(restrictor),
media_type_(AttemptStaticStringCreation(media_type.LowerASCII())),
expressions_(std::move(expressions)) {
std::sort(expressions_.begin(), expressions_.end(), ExpressionCompare);

// Remove all duplicated expressions.
MediaQueryExp key = MediaQueryExp::Invalid();
for (int i = expressions_.size() - 1; i >= 0; --i) {
MediaQueryExp exp = expressions_.at(i);
CHECK(exp.IsValid());
if (exp == key)
expressions_.EraseAt(i);
else
key = exp;
}
}
expressions_(std::move(expressions)) {}

MediaQuery::MediaQuery(const MediaQuery& o)
: restrictor_(o.restrictor_),
Expand Down
3 changes: 1 addition & 2 deletions third_party/blink/renderer/core/css/media_query_set_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,7 @@ TEST(MediaQuerySetTest, Basic) {
{"(example, all,), speech", "not all, speech"},
{"&test, screen", "not all, screen"},
{"print and (min-width: 25cm)", nullptr},
{"screen and (min-width: 400px) and (max-width: 700px)",
"screen and (max-width: 700px) and (min-width: 400px)"},
{"screen and (min-width: 400px) and (max-width: 700px)", nullptr},
{"screen and (device-width: 800px)", nullptr},
{"screen and (device-height: 60em)", nullptr},
{"screen and (device-height: 60rem)", nullptr},
Expand Down
7 changes: 4 additions & 3 deletions third_party/blink/renderer/core/css/parser/media_condition_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@ TEST(MediaConditionParserTest, Basic) {
{"(min-width:500px)", "(min-width: 500px)"},
{"(min-width: -100px)", "not all"},
{"(min-width: 100px) and print", "not all"},
{"(min-width: 100px) and (max-width: 900px)",
"(max-width: 900px) and (min-width: 100px)"},
{"(min-width: 100px) and (max-width: 900px)", nullptr},
{"(min-width: [100px) and (max-width: 900px)", "not all"},
{"not (min-width: 900px)", "not all and (min-width: 900px)"},
{"not (blabla)", "not all"},
Expand All @@ -44,7 +43,9 @@ TEST(MediaConditionParserTest, Basic) {
nullptr);
ASSERT_EQ(media_condition_query_set->QueryVector().size(), (unsigned)1);
String query_text = media_condition_query_set->QueryVector()[0]->CssText();
ASSERT_EQ(test_cases[i].output, query_text);
const char* expected_text =
test_cases[i].output ? test_cases[i].output : test_cases[i].input;
ASSERT_EQ(expected_text, query_text);
}
}

Expand Down
6 changes: 2 additions & 4 deletions third_party/blink/web_tests/external/wpt/css/cssom/medialist-interfaces-003.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,19 +40,17 @@
mediaList = styleSheet.media;
}

// First explicit example input (first column) and output (second column) in specification.
test(function() {
setupMedia('not screen and (min-WIDTH:5px) AND (max-width:40px )');

assert_equals(mediaList.mediaText, "not screen and (max-width: 40px) and (min-width: 5px)");
assert_equals(mediaList.mediaText, "not screen and (min-width: 5px) and (max-width: 40px)");

}, "mediatest_mediaquery_serialize_1");

// Second explicit example input (first column) and output (second column) in specification.
test(function() {
setupMedia('all and (color) and (color) ');

assert_equals(mediaList.mediaText, "(color)");
assert_equals(mediaList.mediaText, "(color) and (color)");

}, "mediatest_mediaquery_serialize_2");

Expand Down
4 changes: 2 additions & 2 deletions third_party/blink/web_tests/external/wpt/css/cssom/serialize-media-rule.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,9 @@
);
assert_equals(
sheet.cssRules[1].cssText,
'@media screen and (color) and (max-width: 0px) {\n}'
'@media screen and (max-width: 0px) and (color) {\n}'
);
}, 'features - lexicographical sorting');
}, 'features - no lexicographical sorting');

test(function(t) {
var sheet = makeSheet(t);
Expand Down
25 changes: 0 additions & 25 deletions third_party/blink/web_tests/fast/media/media-query-serialization.html
View file

This file was deleted.

74 changes: 37 additions & 37 deletions third_party/blink/web_tests/inspector-protocol/css/css-get-media-queries-expected.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,22 +33,6 @@ mediaRule #1
unit: em
computed length: 1152
mediaRule #2
text: (max-width: 10px) and (min-width: 20px)
source: mediaRule
range: {"endColumn":46,"endLine":13,"startColumn":7,"startLine":13}
computedText: (min-width: 20px) and (max-width: 10px)
mediaQuery #0 active: false
mediaExpression #0
feature: max-width
value: 10
unit: px
computed length: 10
mediaExpression #1
feature: min-width
value: 20
unit: px
computed length: 20
mediaRule #3
text: (max-width: 200px) and (min-width: 100px)
source: mediaRule
range: {"endColumn":52,"endLine":6,"startColumn":11,"startLine":6}
Expand All @@ -64,7 +48,7 @@ mediaRule #3
value: 100
unit: px
computed length: 100
mediaRule #4
mediaRule #3
text: (min-monochrome: 8)
source: mediaRule
range: {"endColumn":38,"endLine":22,"startColumn":11,"startLine":22}
Expand All @@ -74,7 +58,7 @@ mediaRule #4
feature: min-monochrome
value: 8
unit:
mediaRule #5
mediaRule #4
text: (min-width: 100px)
source: mediaRule
range: {"endColumn":25,"endLine":4,"startColumn":7,"startLine":4}
Expand All @@ -85,7 +69,7 @@ mediaRule #5
value: 100
unit: px
computed length: 100
mediaRule #6
mediaRule #5
text: (min-width: 100px)
source: mediaRule
range: {"endColumn":25,"endLine":4,"startColumn":7,"startLine":4}
Expand All @@ -96,7 +80,7 @@ mediaRule #6
value: 100
unit: px
computed length: 100
mediaRule #7
mediaRule #6
text: (min-width: 100px)
source: mediaRule
range: {"endColumn":25,"endLine":4,"startColumn":7,"startLine":4}
Expand All @@ -107,7 +91,7 @@ mediaRule #7
value: 100
unit: px
computed length: 100
mediaRule #8
mediaRule #7
text: (min-width: 1px), (max-width: 1000em)
source: mediaRule
range: {"endColumn":1,"endLine":9,"startColumn":7,"startLine":1}
Expand All @@ -124,6 +108,22 @@ mediaRule #8
value: 1000
unit: em
computed length: 16000
mediaRule #8
text: (min-width: 20px) and (max-width: 10px)
source: mediaRule
range: {"endColumn":46,"endLine":13,"startColumn":7,"startLine":13}
computedText: (min-width: 20px) and (max-width: 10px)
mediaQuery #0 active: false
mediaExpression #0
feature: min-width
value: 20
unit: px
computed length: 20
mediaExpression #1
feature: max-width
value: 10
unit: px
computed length: 10
mediaRule #9
text: (orientation: landscape), handheld and (max-resolution: 3dppx)
source: importRule
Expand Down Expand Up @@ -186,22 +186,6 @@ mediaRule #15
computedText: screen and (device-aspect-ratio: 16/9), screen and (device-aspect-ratio: 16/10)
mediaList is empty
mediaRule #16
text: screen and (max-height: 4000px) and (min-width: 10px)
source: importRule
range: {"endColumn":42,"endLine":1,"startColumn":37,"startLine":0}
computedText: screen and\n(min-width: 10px) and (max-height: 4000px)
mediaQuery #0
mediaExpression #0
feature: max-height
value: 4000
unit: px
computed length: 4000
mediaExpression #1
feature: min-width
value: 10
unit: px
computed length: 10
mediaRule #17
text: screen and (min-resolution: 2dppx)
source: mediaRule
range: {"endColumn":41,"endLine":7,"startColumn":7,"startLine":7}
Expand All @@ -211,4 +195,20 @@ mediaRule #17
feature: min-resolution
value: 2
unit: dppx
mediaRule #17
text: screen and (min-width: 10px) and (max-height: 4000px)
source: importRule
range: {"endColumn":42,"endLine":1,"startColumn":37,"startLine":0}
computedText: screen and\n(min-width: 10px) and (max-height: 4000px)
mediaQuery #0
mediaExpression #0
feature: min-width
value: 10
unit: px
computed length: 10
mediaExpression #1
feature: max-height
value: 4000
unit: px
computed length: 4000

0 comments on commit 99589ad

Please sign in to comment.