Alison Maher (Gerrit)

unread,

Apr 16, 2024, 8:59:04 PMApr 16

to Kevin Babbitt, Rune Lillesveen, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Nate Chapin, Kenneth Rohde Christiansen, Yoav Weiss (@Shopify), apavlo...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, dmurph+watch...@chromium.org, dominickn+wa...@chromium.org, feature-me...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, webap...@microsoft.com

Attention needed from Kevin Babbitt and Rune Lillesveen

Alison Maher added 1 comment

File third_party/blink/renderer/core/css/parser/media_query_parser.cc

Line 166, Patchset 9: const Document* document,

Rune Lillesveen . resolved

If document is non-null here, isn't execution_context typically a LocalDOMWindow, and you can get the Document from that via DynamicTo<LocalDOMWindow>(execution_context) when creating the fake_context_ instead of plumbing it down here?

Alison Maher

Document isn't always guaranteed (although there were several call sites, so it could be that this one is guarenteed and others weren't). I tried to pass it in wherever it was available, though. From what I found, the execution context is also sometimes fake. As a result, I decided to pass this in separately, but I do think this could likely use some further cleanup at some point since it would be nice to guarantee both and then avoid passing one of them in.

Rune Lillesveen

Where do we create fake ExecutionContexts? And in those cases, would we have a valid Document instead? It seems to me if we can pass in a valid Document, it would be trivial to pass a real ExecutionContext too?
If the ExecutionContext is an instance of LocalDOMWindow, and the passed in document is non-null, then getting the document from the LocalDOMWindow should match the document parameter, right?

Alison Maher

Just walked back all of the call sites and I think you are right. There are some cases where we pass in nullptr for the context, but we don't have access to the Document in those cases either. There was at least a few cases, example `ManifestManager::ParseManifestFromString()`, where we had a context, but it seemed to not be created via a Document [1]. But this wasn't a case of concern for what I was looking into.
I tried updating to what you had suggested in code, and we are able to still access the Document where we were hoping to, so this will simplify things drastically. Thanks for the super useful feedback. I'll get this cleaned up and re-uploaded.
[1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/manifest/manifest_manager.cc;l=96;drc=503773789a3b4754afd75acf1d1a48658082846b;bpv=1;bpt=1

Alison Maher

Done

Open in Gerrit

Related details

Attention is currently required from:

Kevin Babbitt
Rune Lillesveen

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Rune Lillesveen (Gerrit)

unread,

Apr 17, 2024, 4:47:29 AMApr 17

to Alison Maher, Rune Lillesveen, Kevin Babbitt, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Nate Chapin, Kenneth Rohde Christiansen, Yoav Weiss (@Shopify), apavlo...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, dmurph+watch...@chromium.org, dominickn+wa...@chromium.org, feature-me...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, webap...@microsoft.com

Attention needed from Alison Maher and Kevin Babbitt

Rune Lillesveen voted and added 2 comments

Votes added by Rune Lillesveen

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 12 (Latest):

Rune Lillesveen . resolved

lgtm

File third_party/blink/renderer/core/css/media_feature_overrides.cc

Line 103, Patchset 12 (Parent):

Rune Lillesveen . unresolved

This is optional, but I think API layering-wise, it would be slightly better to keep the construction of the parser context here and pass in Document* instead, unless there is plan for upcoming CLs where you already have parser context coming in from other call sites.

Open in Gerrit

Related details

Attention is currently required from:

Alison Maher
Kevin Babbitt

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Alison Maher (Gerrit)

unread,

Apr 17, 2024, 12:25:04 PMApr 17

to Rune Lillesveen, Kevin Babbitt, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Nate Chapin, Kenneth Rohde Christiansen, Yoav Weiss (@Shopify), apavlo...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, dmurph+watch...@chromium.org, dominickn+wa...@chromium.org, feature-me...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, webap...@microsoft.com

Attention needed from Kevin Babbitt

Alison Maher voted and added 1 comment

Votes added by Alison Maher

Commit-Queue

+1

1 comment

File third_party/blink/renderer/core/css/media_feature_overrides.cc

Line 103, Patchset 12 (Parent):

Rune Lillesveen . resolved

This is optional, but I think API layering-wise, it would be slightly better to keep the construction of the parser context here and pass in Document* instead, unless there is plan for upcoming CLs where you already have parser context coming in from other call sites.

Alison Maher

Good point, yeah no immediate plans to update the parser contexts being passed in, so I've updated to pass in the Document instead.

Open in Gerrit

Related details

Attention is currently required from:

Kevin Babbitt

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Alison Maher (Gerrit)

unread,

Apr 17, 2024, 1:08:39 PMApr 17

to Rune Lillesveen, Kevin Babbitt, Chromium LUCI CQ, Alexis Menard, chromium...@chromium.org, Nate Chapin, Kenneth Rohde Christiansen, Yoav Weiss (@Shopify), apavlo...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, dmurph+watch...@chromium.org, dominickn+wa...@chromium.org, feature-me...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, webap...@microsoft.com

Attention needed from Kevin Babbitt

Alison Maher voted Commit-Queue+2

Commit-Queue

+2

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Apr 17, 2024, 1:31:12 PMApr 17

to Alison Maher, Rune Lillesveen, Kevin Babbitt, Alexis Menard, chromium...@chromium.org, Nate Chapin, Kenneth Rohde Christiansen, Yoav Weiss (@Shopify), apavlo...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, dmurph+watch...@chromium.org, dominickn+wa...@chromium.org, feature-me...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, webap...@microsoft.com

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

12 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: third_party/blink/renderer/core/preferences/preference_overrides.h
Insertions: 2, Deletions: 2.

@@ -14,7 +14,7 @@
 
 namespace blink {
 
-class CSSParserContext;
+class Document;
 
 // PreferenceOverrides represents the Web Preferences API overrides.
 // Spec: https://wicg.github.io/web-preferences-api/
@@ -25,7 +25,7 @@
   // When value_string is empty, or otherwise invalid, it clears the override.
   void SetOverride(const AtomicString& feature,
                    const String& value_string,
-                   const CSSParserContext&);
+                   const Document*);
 
   std::optional<mojom::blink::PreferredColorScheme> GetPreferredColorScheme()
       const {
```
```
The name of the file: third_party/blink/renderer/core/css/media_feature_overrides_test.cc
Insertions: 15, Deletions: 24.

@@ -7,9 +7,6 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/mojom/css/preferred_color_scheme.mojom-blink.h"
 #include "third_party/blink/renderer/core/css/media_feature_names.h"
-#include "third_party/blink/renderer/core/css/parser/css_parser_context.h"
-#include "third_party/blink/renderer/core/css/parser/css_parser_mode.h"
-#include "third_party/blink/renderer/core/execution_context/security_context.h"
 
 namespace blink {
 
@@ -23,84 +20,78 @@
 
 TEST(MediaFeatureOverrides, SetOverrideInvalid) {
   MediaFeatureOverrides overrides;
-  const CSSParserContext* fake_context = MakeGarbageCollected<CSSParserContext>(
-      kHTMLStandardMode, SecureContextMode::kInsecureContext);
 
   overrides.SetOverride(media_feature_names::kPrefersColorSchemeMediaFeature,
-                        "1px", *fake_context);
+                        "1px", /*document=*/nullptr);
   EXPECT_FALSE(overrides.GetPreferredColorScheme().has_value());
 
   overrides.SetOverride(media_feature_names::kPrefersColorSchemeMediaFeature,
-                        "orange", *fake_context);
+                        "orange", /*document=*/nullptr);
   EXPECT_FALSE(overrides.GetPreferredColorScheme().has_value());
 
   overrides.SetOverride(
       media_feature_names::kPrefersReducedTransparencyMediaFeature, "orange",
-      *fake_context);
+      /*document=*/nullptr);
   EXPECT_FALSE(overrides.GetPreferredColorScheme().has_value());
 }
 
 TEST(MediaFeatureOverrides, SetOverrideValid) {
   MediaFeatureOverrides overrides;
-  const CSSParserContext* fake_context = MakeGarbageCollected<CSSParserContext>(
-      kHTMLStandardMode, SecureContextMode::kInsecureContext);
 
   overrides.SetOverride(media_feature_names::kPrefersColorSchemeMediaFeature,
-                        "light", *fake_context);
+                        "light", /*document=*/nullptr);
   EXPECT_EQ(mojom::blink::PreferredColorScheme::kLight,
             overrides.GetPreferredColorScheme());
 
   overrides.SetOverride(media_feature_names::kPrefersColorSchemeMediaFeature,
-                        "dark", *fake_context);
+                        "dark", /*document=*/nullptr);
   EXPECT_EQ(mojom::blink::PreferredColorScheme::kDark,
             overrides.GetPreferredColorScheme());
 
   overrides.SetOverride(
       media_feature_names::kPrefersReducedTransparencyMediaFeature, "reduce",
-      *fake_context);
+      /*document=*/nullptr);
   EXPECT_TRUE(overrides.GetPrefersReducedTransparency().value());
 
   overrides.SetOverride(
       media_feature_names::kPrefersReducedTransparencyMediaFeature,
-      "no-preference", *fake_context);
+      "no-preference", /*document=*/nullptr);
   EXPECT_FALSE(overrides.GetPrefersReducedTransparency().value());
 }
 
 TEST(MediaFeatureOverrides, ResetOverride) {
   MediaFeatureOverrides overrides;
-  const CSSParserContext* fake_context = MakeGarbageCollected<CSSParserContext>(
-      kHTMLStandardMode, SecureContextMode::kInsecureContext);
 
   overrides.SetOverride(media_feature_names::kPrefersColorSchemeMediaFeature,
-                        "light", *fake_context);
+                        "light", /*document=*/nullptr);
   EXPECT_TRUE(overrides.GetPreferredColorScheme().has_value());
   overrides.SetOverride(media_feature_names::kPrefersColorSchemeMediaFeature,
-                        "", *fake_context);
+                        "", /*document=*/nullptr);
   EXPECT_FALSE(overrides.GetPreferredColorScheme().has_value());
 
   overrides.SetOverride(media_feature_names::kPrefersColorSchemeMediaFeature,
-                        "light", *fake_context);
+                        "light", /*document=*/nullptr);
   EXPECT_TRUE(overrides.GetPreferredColorScheme().has_value());
   overrides.SetOverride(media_feature_names::kPrefersColorSchemeMediaFeature,
-                        "invalid", *fake_context);
+                        "invalid", /*document=*/nullptr);
   EXPECT_FALSE(overrides.GetPreferredColorScheme().has_value());
 
   overrides.SetOverride(
       media_feature_names::kPrefersReducedTransparencyMediaFeature, "reduce",
-      *fake_context);
+      /*document=*/nullptr);
   EXPECT_TRUE(overrides.GetPrefersReducedTransparency().has_value());
   overrides.SetOverride(
       media_feature_names::kPrefersReducedTransparencyMediaFeature, "",
-      *fake_context);
+      /*document=*/nullptr);
   EXPECT_FALSE(overrides.GetPrefersReducedTransparency().has_value());
 
   overrides.SetOverride(
       media_feature_names::kPrefersReducedTransparencyMediaFeature, "reduce",
-      *fake_context);
+      /*document=*/nullptr);
   EXPECT_TRUE(overrides.GetPrefersReducedTransparency().has_value());
   overrides.SetOverride(
       media_feature_names::kPrefersReducedTransparencyMediaFeature, "invalid",
-      *fake_context);
+      /*document=*/nullptr);
   EXPECT_FALSE(overrides.GetPrefersReducedTransparency().has_value());
 }
 
```
```
The name of the file: third_party/blink/renderer/core/preferences/preference_overrides.cc
Insertions: 2, Deletions: 2.

@@ -12,9 +12,9 @@
 
 void PreferenceOverrides::SetOverride(const AtomicString& feature,
                                       const String& value_string,
-                                      const CSSParserContext& context) {
+                                      const Document* document) {
   MediaQueryExpValue value = MediaFeatureOverrides::ParseMediaQueryValue(
-      feature, value_string, context);
+      feature, value_string, document);
 
   if (feature == media_feature_names::kPrefersColorSchemeMediaFeature) {
     preferred_color_scheme_ =
```
```
The name of the file: third_party/blink/renderer/core/css/media_feature_overrides.h
Insertions: 3, Deletions: 3.

@@ -15,7 +15,7 @@
 namespace blink {
 
 enum class ColorSpaceGamut;
-class CSSParserContext;
+class Document;
 enum class ForcedColors;
 class MediaQueryExpValue;
 
@@ -25,7 +25,7 @@
  public:
   void SetOverride(const AtomicString& feature,
                    const String& value_string,
-                   const CSSParserContext&);
+                   const Document*);
 
   std::optional<ColorSpaceGamut> GetColorGamut() const { return color_gamut_; }
   std::optional<mojom::blink::PreferredColorScheme> GetPreferredColorScheme()
@@ -59,7 +59,7 @@
 
   static MediaQueryExpValue ParseMediaQueryValue(const AtomicString&,
                                                  const String&,
-                                                 const CSSParserContext&);
+                                                 const Document*);
 
  private:
   std::optional<ColorSpaceGamut> color_gamut_;
```
```
The name of the file: third_party/blink/renderer/core/page/page.cc
Insertions: 2, Deletions: 6.

@@ -1292,10 +1292,8 @@
   if (auto* local_frame = DynamicTo<LocalFrame>(MainFrame())) {
     document = local_frame->GetDocument();
   }
-  const CSSParserContext* fake_context = MakeGarbageCollected<CSSParserContext>(
-      kHTMLStandardMode, SecureContextMode::kInsecureContext, document);
 
-  media_feature_overrides_->SetOverride(media_feature, value, *fake_context);
+  media_feature_overrides_->SetOverride(media_feature, value, document);
   if (media_feature == "prefers-color-scheme" ||
       media_feature == "forced-colors")
     SettingsChanged(ChangeType::kColorScheme);
@@ -1322,10 +1320,8 @@
   if (auto* local_frame = DynamicTo<LocalFrame>(MainFrame())) {
     document = local_frame->GetDocument();
   }
-  const CSSParserContext* fake_context = MakeGarbageCollected<CSSParserContext>(
-      kHTMLStandardMode, SecureContextMode::kInsecureContext, document);
 
-  preference_overrides_->SetOverride(media_feature, value, *fake_context);
+  preference_overrides_->SetOverride(media_feature, value, document);
   if (media_feature == "prefers-color-scheme") {
     SettingsChanged(ChangeType::kColorScheme);
   } else {
```
```
The name of the file: third_party/blink/renderer/core/css/media_feature_overrides.cc
Insertions: 10, Deletions: 4.

@@ -90,12 +90,18 @@
 MediaQueryExpValue MediaFeatureOverrides::ParseMediaQueryValue(
     const AtomicString& feature,
     const String& value_string,
-    const CSSParserContext& context) {
+    const Document* document) {
   CSSTokenizer tokenizer(value_string);
   auto [tokens, raw_offsets] = tokenizer.TokenizeToEOFWithOffsets();
   CSSParserTokenRange range(tokens);
   CSSParserTokenOffsets offsets(tokens, std::move(raw_offsets), value_string);
 
+  // TODO(xiaochengh): This is a fake CSSParserContext that only passes
+  // down the CSSParserMode. Plumb the real CSSParserContext through, so that
+  // web features can be counted correctly.
+  const CSSParserContext* fake_context = MakeGarbageCollected<CSSParserContext>(
+      kHTMLStandardMode, SecureContextMode::kInsecureContext, document);
+
   // MediaFeatureOverrides are used to emulate various media feature values.
   // These don't need to pass an ExecutionContext, since the parsing of
   // the actual CSS will determine whether or not the emulated values will come
@@ -105,16 +111,16 @@
   // Document to get the ExecutionContext so the extra parameter should be
   // removed.
   MediaQueryExpBounds bounds =
-      MediaQueryExp::Create(feature, range, offsets, context).Bounds();
+      MediaQueryExp::Create(feature, range, offsets, *fake_context).Bounds();
   DCHECK(!bounds.left.IsValid());
   return bounds.right.value;
 }
 
 void MediaFeatureOverrides::SetOverride(const AtomicString& feature,
                                         const String& value_string,
-                                        const CSSParserContext& context) {
+                                        const Document* document) {
   MediaQueryExpValue value =
-      ParseMediaQueryValue(feature, value_string, context);
+      ParseMediaQueryValue(feature, value_string, document);
 
   if (feature == media_feature_names::kColorGamutMediaFeature) {
     color_gamut_ = ConvertColorGamut(value);
```

Change information

Commit message:

Ensure CSSParserContext has Document in MediaQueryExp

The CSSParserContext passed into MediaQueryExp is fake and doesn't
currently have access to the Document. Having access to
the Document will allow one to log console messages, etc for media
queries. Thus, ensure that the CSSParserContext passed into
MediaQueryExp has access to the Document where possible.

No behavior changes intended.

Change-Id: Id4ba07f79b202bb81351ed4f8a6fab9d55cc03da

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5451504

Reviewed-by: Rune Lillesveen <fut...@chromium.org>

Commit-Queue: Alison Maher <alm...@microsoft.com>

Cr-Commit-Position: refs/heads/main@{#1288809}

Files:

M third_party/blink/renderer/core/css/media_feature_overrides.cc
M third_party/blink/renderer/core/css/media_feature_overrides.h
M third_party/blink/renderer/core/css/media_feature_overrides_test.cc
M third_party/blink/renderer/core/css/parser/media_query_parser.cc
M third_party/blink/renderer/core/page/page.cc
M third_party/blink/renderer/core/preferences/preference_overrides.cc
M third_party/blink/renderer/core/preferences/preference_overrides.h

Change size: M

Delta: 7 files changed, 63 insertions(+), 28 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Rune Lillesveen

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Alison Maher (Gerrit)

unread,

Apr 17, 2024, 4:25:10 PMApr 17

to Chromium LUCI CQ, Rune Lillesveen, Kevin Babbitt, Alexis Menard, chromium...@chromium.org, Nate Chapin, Kenneth Rohde Christiansen, Yoav Weiss (@Shopify), apavlo...@chromium.org, blink-re...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org, dmurph+watch...@chromium.org, dominickn+wa...@chromium.org, feature-me...@chromium.org, gavinp...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, loading...@chromium.org, webap...@microsoft.com

Alison Maher added 1 comment

File third_party/blink/renderer/core/page/page.cc

Line 34, Patchset 14 (Latest):#include "third_party/blink/renderer/core/css/parser/css_parser_context.h"

Alison Maher . resolved

Just realized I left this behind. Will clean this up in a follow up.

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Reply all

Reply to author

Forward

Ensure CSSParserContext has Document in MediaQueryExp [chromium/src : main]

Alison Maher (Gerrit)

Alison Maher added 1 comment

Related details

Rune Lillesveen (Gerrit)

Rune Lillesveen voted and added 2 comments

Votes added by Rune Lillesveen

2 comments

Related details

Alison Maher (Gerrit)

Alison Maher voted and added 1 comment

Votes added by Alison Maher

1 comment

Related details

Alison Maher (Gerrit)

Alison Maher voted Commit-Queue+2

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information

Alison Maher (Gerrit)

Alison Maher added 1 comment

Related details