[SVG][Media Fragments] Parse spatial media fragments in SVGImage [chromium/src : main]

0 views

Skip to first unread message

Divyansh Mangal (Gerrit)

unread,

Apr 1, 2026, 1:37:43 AM (yesterday) Apr 1

to AyeAye, Virali Purbey, Vinay Singh, Ragvesh Sharma, Dileep Maurya, Gaurav Kumar, Sejal Anand, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Olga Gerchikov, Stephen Chenney, blink-rev...@chromium.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org

Attention needed from Divyansh Mangal, Vinay Singh and Virali Purbey

Message from Divyansh Mangal

Set Ready For Review

Open in Gerrit

Related details

Attention is currently required from:

Divyansh Mangal
Vinay Singh
Virali Purbey

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Vinay Singh (Gerrit)

unread,

Apr 1, 2026, 4:25:13 PM (14 hours ago) Apr 1

to Divyansh Mangal, AyeAye, Virali Purbey, Ragvesh Sharma, Dileep Maurya, Gaurav Kumar, Sejal Anand, Chromium LUCI CQ, Menard, Alexis, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Olga Gerchikov, Stephen Chenney, blink-rev...@chromium.org, kinuko...@chromium.org, jmedle...@chromium.org, blink-revie...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, fmalit...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org

Attention needed from Divyansh Mangal and Virali Purbey

Vinay Singh added 7 comments

File third_party/blink/renderer/core/html/media/media_fragment_uri_parser.h

Line 86, Patchset 7 (Latest): bool is_valid_ = true;

Vinay Singh . unresolved

Why would `MediaFragmentURIParser` have a field `is_valid_`?
It is sort of giving an impression as if the parser can be valid or invalid.

`is_url_valid_` probably might be a better choice.

Line 50, Patchset 7 (Latest): gfx::Rect GetRect() const { return rect; }

Vinay Singh . unresolved

`rect` is public already. Either make it private or remove this function and access directly.

File third_party/blink/renderer/core/html/media/media_fragment_uri_parser_test.cc

Line 219, Patchset 7 (Latest):

TEST(MediaFragmentURIParserTest, FragmentConstructorParsesTime) {
  MediaFragmentURIParser parser(StringView("t=5,10"));
  EXPECT_DOUBLE_EQ(parser.StartTime(), 5.0);
  EXPECT_DOUBLE_EQ(parser.EndTime(), 10.0);
}

Vinay Singh . unresolved

```suggestion
TEST(MediaFragmentURIParserTest, FragmentConstructorParsesTime) {
  MediaFragmentURIParser parser(StringView("t=5,10"));
  EXPECT_DOUBLE_EQ(parser.StartTime(), 5.0);
  EXPECT_DOUBLE_EQ(parser.EndTime(), 10.0);
}

TEST(MediaFragmentURIParserTest, FragmentConstructorParsesSpatial) {
   MediaFragmentURIParser parser(StringView("xywh=10,20,30,40"));
   SpatialClip clip = parser.SpatialFragment();
   EXPECT_TRUE(clip.IsValid());
   EXPECT_EQ(clip.rect, gfx::Rect(10, 20, 30, 40));
   EXPECT_EQ(clip.unit, SpatialClip::Unit::kPixel);
}
```

We are not testing the primary functionality of this CL. This additional test should be helpful.

File third_party/blink/renderer/core/svg/graphics/svg_image.cc

Line 205, Patchset 7 (Latest): if (RuntimeEnabledFeatures::SvgSupportMediaFragmentsEnabled()) {

Vinay Singh . unresolved

``&& fragment.Contains("xywh")`` or something similar??

I guess this will make for an early return sparing initialization of MediaFragmentURIParser for all the non-`xywh` cases which I'm assuming must be in majority.

Line 215, Patchset 7 (Latest): gfx::SizeF svg_size(SizeWithConfig(SizeConfig()));

Vinay Singh . unresolved

Consider `SizeWithConfigAsFloat`

SizeWithConfig truncates the fraction as it returns back a RoundedSize which can be a bit premature at this point and may result into pixel errors.

File third_party/blink/web_tests/external/wpt/svg/linking/reftests/media-fragment-spatial-overflow-clamped.html

Line 13, Patchset 7 (Latest): background-image: url("reference/green-100x100.svg#xywh=0,0,200,200");

Vinay Singh . unresolved

Same here.

Wouldn't this produce the same 100x100 green image anyway regardless of the fragment?
Please correct me if I'm wrong.

File third_party/blink/web_tests/external/wpt/svg/linking/reftests/media-fragment-spatial-percent-full.html

Line 11, Patchset 7 (Latest): background-image: url("reference/green-100x100.svg#xywh=percent:0,0,100,100");

Vinay Singh . unresolved

Wouldn't this produce the same 100x100 green image regardless of the fragment?
Please correct me if I'm wrong.

Open in Gerrit

Related details

Attention is currently required from:

Divyansh Mangal
Virali Purbey

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages