김민성 (Gerrit)

unread,

Jun 19, 2025, 9:59:46 AM6/19/25

to Jason Leo, AyeAye, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Stephen Chenney, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

김민성 added 1 comment

Patchset-level comments

File-level comment, Patchset 1:

김민성 . resolved

Hi @cg...@chromium.org! Can I ask you to vote cq+1 for dry run, please?

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

unsatisfied_requirement

open

diffy

김민성 (Gerrit)

unread,

Jun 19, 2025, 8:25:52 PM6/19/25

to Olga Gerchikov, Chromium LUCI CQ, Jason Leo, AyeAye, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Stephen Chenney, blink-revie...@chromium.org, mac-r...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

Attention needed from Jason Leo

김민성 added 1 comment

Patchset-level comments

File-level comment, Patchset 6 (Latest):

김민성 . resolved

Hi, @cg...@chromium.org. Could you vote cq+1 with `win-11` again, please? We need to update windows baseline image. Thanks for your help.

Open in Gerrit

Related details

Attention is currently required from:

Jason Leo

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

김민성 (Gerrit)

unread,

Jun 20, 2025, 6:39:05 AM6/20/25

to Anders Hartvoll Ruud, Jason Leo, Olga Gerchikov, Chromium LUCI CQ, AyeAye, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Stephen Chenney, blink-revie...@chromium.org, mac-r...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

Attention needed from Anders Hartvoll Ruud and Jason Leo

김민성 added 1 comment

Patchset-level comments

File-level comment, Patchset 7 (Latest):

김민성 . resolved

Would you review this CL and vote cq+1, please? Thanks.

Open in Gerrit

Related details

Attention is currently required from:

Anders Hartvoll Ruud
Jason Leo

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,

Jun 23, 2025, 4:49:13 AM6/23/25

to 김민성, Koji Ishii, Rune Lillesveen, Olga Gerchikov, Chromium LUCI CQ, AyeAye, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Stephen Chenney, blink-revie...@chromium.org, mac-r...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

Attention needed from Koji Ishii and 김민성

Rune Lillesveen added 3 comments

Patchset-level comments

File-level comment, Patchset 8 (Latest):

Rune Lillesveen . resolved

Adding Koji as he knows more about inline-layout/spacing.

File third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc

Line 12032, Patchset 8 (Latest): const Length& spacing = style.SpecifiedWordSpacing();

Rune Lillesveen . unresolved

What does "Specified" mean in this context? Specified is a term that normally refers to specified styles which could be a relative length, for instance, but the code here seems to assume it's a pixel value that is a zoom-adjusted computed value?

File third_party/blink/renderer/core/css/resolver/style_adjuster.cc

Line 444, Patchset 8 (Latest): builder.SetWordSpacing(Length::Fixed(0.0f));

Rune Lillesveen . unresolved

Why 0? Reading the spec, it says the initial value is 'normal', so this is supposed to be the initial value? If so, add a comment /* 'normal' */ perhaps?

Open in Gerrit

Related details

Attention is currently required from:

Koji Ishii
김민성

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

김민성 (Gerrit)

unread,

Jun 23, 2025, 5:35:01 AM6/23/25

to Koji Ishii, Rune Lillesveen, Olga Gerchikov, Chromium LUCI CQ, AyeAye, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Stephen Chenney, blink-revie...@chromium.org, mac-r...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

Attention needed from Koji Ishii and Rune Lillesveen

김민성 added 3 comments

Patchset-level comments

File-level comment, Patchset 8 (Latest):

김민성 . resolved

Thank you for review this!

File third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc

Line 12032, Patchset 8 (Latest): const Length& spacing = style.SpecifiedWordSpacing();

Rune Lillesveen . unresolved

What does "Specified" mean in this context? Specified is a term that normally refers to specified styles which could be a relative length, for instance, but the code here seems to assume it's a pixel value that is a zoom-adjusted computed value?

김민성

Thanks for your good feedback! I think it's better to handle this as computed value. So, how about changing function name to `
ComputedStyle::ComputedWordSpacing`?

File third_party/blink/renderer/core/css/resolver/style_adjuster.cc

Line 444, Patchset 8 (Latest): builder.SetWordSpacing(Length::Fixed(0.0f));

Rune Lillesveen . unresolved

Why 0? Reading the spec, it says the initial value is 'normal', so this is supposed to be the initial value? If so, add a comment /* 'normal' */ perhaps?

김민성

I think it's initial value for 'normal', but I'm not sure. I was just implementing without behavior changes at the `
StyleAdjuster::AdjustStyleForCombinedText`.
Hi @ko...@chromium.org, can I ask about this, please?

Open in Gerrit

Related details

Attention is currently required from:

Koji Ishii
Rune Lillesveen

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

김민성 (Gerrit)

unread,

Jun 23, 2025, 8:37:04 AM6/23/25

to Koji Ishii, Rune Lillesveen, Olga Gerchikov, Chromium LUCI CQ, AyeAye, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Stephen Chenney, blink-revie...@chromium.org, mac-r...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

Attention needed from Koji Ishii and Rune Lillesveen

김민성 added 1 comment

File third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc

Line 12032, Patchset 8 (Latest): const Length& spacing = style.SpecifiedWordSpacing();

Rune Lillesveen . unresolved

What does "Specified" mean in this context? Specified is a term that normally refers to specified styles which could be a relative length, for instance, but the code here seems to assume it's a pixel value that is a zoom-adjusted computed value?

김민성

Thanks for your good feedback! I think it's better to handle this as computed value. So, how about changing function name to `
ComputedStyle::ComputedWordSpacing`?

김민성

Or the below?
```
float ComputedStyle::ComputedWordSpacing();
Length ComputedStyle::WordSpacing();
```
Change name of the `WordSpaincg` to the `ComputedWordSpacing` that returns computed float value. And change the `SpecifiedWordSpacing` to the `WordSpacing`. It looks more specific.

Open in Gerrit

Related details

Attention is currently required from:

Koji Ishii
Rune Lillesveen

Submit Requirements:

Code-Coverage
Code-Owners

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

Rune Lillesveen (Gerrit)

unread,

Jun 24, 2025, 7:23:35 AM6/24/25

to 김민성, Koji Ishii, Rune Lillesveen, Olga Gerchikov, Chromium LUCI CQ, AyeAye, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Stephen Chenney, blink-revie...@chromium.org, mac-r...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

Attention needed from Koji Ishii and 김민성

Rune Lillesveen added 1 comment

File third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc

Line 12032, Patchset 8 (Latest): const Length& spacing = style.SpecifiedWordSpacing();

Rune Lillesveen . resolved

What does "Specified" mean in this context? Specified is a term that normally refers to specified styles which could be a relative length, for instance, but the code here seems to assume it's a pixel value that is a zoom-adjusted computed value?

김민성

Thanks for your good feedback! I think it's better to handle this as computed value. So, how about changing function name to `
ComputedStyle::ComputedWordSpacing`?

김민성

Or the below?
```
float ComputedStyle::ComputedWordSpacing();
Length ComputedStyle::WordSpacing();
```
Change name of the `WordSpaincg` to the `ComputedWordSpacing` that returns computed float value. And change the `SpecifiedWordSpacing` to the `WordSpacing`. It looks more specific.

Rune Lillesveen

I see that we're using the "specified" term for other properties, and they should be consistent, so I guess we can leave it for now even though the names are confusing.

Open in Gerrit

Related details

Attention is currently required from:

Koji Ishii
김민성

satisfied_requirement

unsatisfied_requirement

open

diffy

김민성 (Gerrit)

unread,

Jun 24, 2025, 8:02:00 AM6/24/25

to Koji Ishii, Rune Lillesveen, Olga Gerchikov, Chromium LUCI CQ, AyeAye, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Stephen Chenney, blink-revie...@chromium.org, mac-r...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

Attention needed from Koji Ishii

김민성 added 1 comment

File third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc

Line 12032, Patchset 8 (Latest): const Length& spacing = style.SpecifiedWordSpacing();

Rune Lillesveen . resolved

What does "Specified" mean in this context? Specified is a term that normally refers to specified styles which could be a relative length, for instance, but the code here seems to assume it's a pixel value that is a zoom-adjusted computed value?

김민성

Thanks for your good feedback! I think it's better to handle this as computed value. So, how about changing function name to `
ComputedStyle::ComputedWordSpacing`?

김민성

Or the below?
```
float ComputedStyle::ComputedWordSpacing();
Length ComputedStyle::WordSpacing();
```
Change name of the `WordSpaincg` to the `ComputedWordSpacing` that returns computed float value. And change the `SpecifiedWordSpacing` to the `WordSpacing`. It looks more specific.

Rune Lillesveen

I see that we're using the "specified" term for other properties, and they should be consistent, so I guess we can leave it for now even though the names are confusing.

김민성

Okay, I'll add a TODO comment here for understanding. Thanks.

Open in Gerrit

Related details

Attention is currently required from:

Koji Ishii

satisfied_requirement

unsatisfied_requirement

open

diffy

김민성 (Gerrit)

unread,

Jun 29, 2025, 8:24:22 PM6/29/25

to Kent Tamura, Koji Ishii, Olga Gerchikov, Chromium LUCI CQ, AyeAye, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Stephen Chenney, blink-revie...@chromium.org, mac-r...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

Attention needed from Kent Tamura and Koji Ishii

김민성 added 1 comment

Patchset-level comments

File-level comment, Patchset 9 (Latest):

김민성 . resolved

Hi! Would you take a look when you have chance, please?

Open in Gerrit

Related details

Attention is currently required from:

Kent Tamura
Koji Ishii

Submit Requirements:

Code-Coverage
Code-Owners
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

Kent Tamura (Gerrit)

unread,

Jul 1, 2025, 9:56:48 PM7/1/25

to 김민성, Kent Tamura, Koji Ishii, Olga Gerchikov, Chromium LUCI CQ, AyeAye, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Stephen Chenney, blink-revie...@chromium.org, mac-r...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

Attention needed from Koji Ishii and 김민성

Kent Tamura added 2 comments

Patchset-level comments

File-level comment, Patchset 9 (Latest):

Kent Tamura . resolved

This is a web-exposed change. I think this needs a chromestatus.com entry, with "No developer-visible change" feature type.

File third_party/blink/renderer/core/style/computed_style.h

Line 902, Patchset 9 (Latest): // TODO(crbug.com/327740939): Rename this because `Specified` is a term that

Kent Tamura . unresolved

Please fix it in this CL.

Open in Gerrit

Related details

Attention is currently required from:

Koji Ishii
김민성

satisfied_requirement

unsatisfied_requirement

open

diffy

김민성 (Gerrit)

unread,

Jul 2, 2025, 6:18:18 AM7/2/25

to Kent Tamura, Koji Ishii, Olga Gerchikov, Chromium LUCI CQ, AyeAye, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Stephen Chenney, blink-revie...@chromium.org, mac-r...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

Attention needed from Kent Tamura and Koji Ishii

김민성 added 2 comments

Patchset-level comments

File-level comment, Patchset 9:

Kent Tamura . resolved

This is a web-exposed change. I think this needs a chromestatus.com entry, with "No developer-visible change" feature type.

김민성

Thanks! I've requested access to create a new feature. I'll create a new entry and follow the below steps.
https://www.chromium.org/blink/launching-features/#process-existing-standard

File third_party/blink/renderer/core/style/computed_style.h

Line 902, Patchset 9: // TODO(crbug.com/327740939): Rename this because `Specified` is a term that

Kent Tamura . resolved

Please fix it in this CL.

김민성

Done

Open in Gerrit

Related details

Attention is currently required from:

Kent Tamura
Koji Ishii

Submit Requirements:

Code-Coverage
Code-Owners
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

Kent Tamura (Gerrit)

unread,

Jul 2, 2025, 9:09:27 PM7/2/25

to 김민성, Kent Tamura, Koji Ishii, Olga Gerchikov, Chromium LUCI CQ, AyeAye, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Stephen Chenney, blink-revie...@chromium.org, mac-r...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

Attention needed from Koji Ishii and 김민성

Kent Tamura added 1 comment

Patchset-level comments

File-level comment, Patchset 10 (Latest):

Kent Tamura . resolved

Looks ok to me, but I think this should be reviewed by core/css/OWNERS.

Open in Gerrit

Related details

Attention is currently required from:

Koji Ishii
김민성

satisfied_requirement

unsatisfied_requirement

open

diffy

김민성 (Gerrit)

unread,

Jul 2, 2025, 9:20:44 PM7/2/25

to Anders Hartvoll Ruud, Kent Tamura, Koji Ishii, Olga Gerchikov, Chromium LUCI CQ, AyeAye, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Stephen Chenney, blink-revie...@chromium.org, mac-r...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

Attention needed from Anders Hartvoll Ruud and Koji Ishii

김민성 added 1 comment

Patchset-level comments

File-level comment, Patchset 10 (Latest):

김민성 . resolved

Hi @and...@chromium.org, would you review this CL, please?

Open in Gerrit

Related details

Attention is currently required from:

Anders Hartvoll Ruud
Koji Ishii

Submit Requirements:

Code-Coverage
Code-Owners
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

김민성 (Gerrit)

unread,

Jul 24, 2025, 12:01:28 AM7/24/25

to Rune Lillesveen, Kent Tamura, Koji Ishii, Olga Gerchikov, Chromium LUCI CQ, AyeAye, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Stephen Chenney, blink-revie...@chromium.org, mac-r...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

Attention needed from Koji Ishii and Rune Lillesveen

김민성 added 1 comment

File third_party/blink/renderer/core/css/resolver/style_adjuster.cc

Line 444, Patchset 8: builder.SetWordSpacing(Length::Fixed(0.0f));

Rune Lillesveen . unresolved

Why 0? Reading the spec, it says the initial value is 'normal', so this is supposed to be the initial value? If so, add a comment /* 'normal' */ perhaps?

김민성

I think it's initial value for 'normal', but I'm not sure. I was just implementing without behavior changes at the `
StyleAdjuster::AdjustStyleForCombinedText`.
Hi @ko...@chromium.org, can I ask about this, please?

김민성

I've added comment for now. Would you please review this CL?

Open in Gerrit

Related details

Attention is currently required from:

Koji Ishii
Rune Lillesveen

Submit Requirements:

Code-Coverage
Code-Owners
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

Rune Lillesveen (Gerrit)

unread,

Jul 24, 2025, 4:38:46 AM7/24/25

to 김민성, Rune Lillesveen, Kent Tamura, Koji Ishii, Olga Gerchikov, Chromium LUCI CQ, AyeAye, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Stephen Chenney, blink-revie...@chromium.org, mac-r...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

Attention needed from Koji Ishii and 김민성

Rune Lillesveen voted and added 2 comments

Votes added by Rune Lillesveen

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 16 (Latest):

Rune Lillesveen . resolved

lgtm

File third_party/blink/renderer/core/css/resolver/style_adjuster.cc

Line 444, Patchset 8: builder.SetWordSpacing(Length::Fixed(0.0f));

Rune Lillesveen . resolved

Why 0? Reading the spec, it says the initial value is 'normal', so this is supposed to be the initial value? If so, add a comment /* 'normal' */ perhaps?

김민성

I think it's initial value for 'normal', but I'm not sure. I was just implementing without behavior changes at the `
StyleAdjuster::AdjustStyleForCombinedText`.
Hi @ko...@chromium.org, can I ask about this, please?

김민성

I've added comment for now. Would you please review this CL?

Rune Lillesveen

Acknowledged

Open in Gerrit

Related details

Attention is currently required from:

Koji Ishii
김민성

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Koji Ishii (Gerrit)

unread,

Jul 25, 2025, 1:17:01 AM7/25/25

to 김민성, Rune Lillesveen, Kent Tamura, Olga Gerchikov, Chromium LUCI CQ, AyeAye, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Stephen Chenney, blink-revie...@chromium.org, mac-r...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

Attention needed from 김민성

Koji Ishii voted and added 1 comment

Votes added by Koji Ishii

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 16 (Latest):

Koji Ishii . resolved

lgtm

Open in Gerrit

Related details

Attention is currently required from:

김민성

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

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

Gerrit

satisfied_requirement

open

diffy

김민성 (Gerrit)

unread,

Jul 25, 2025, 2:13:24 AM7/25/25

to Koji Ishii, Rune Lillesveen, Kent Tamura, Olga Gerchikov, Chromium LUCI CQ, AyeAye, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Stephen Chenney, blink-revie...@chromium.org, mac-r...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

김민성 voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Jul 25, 2025, 3:09:32 AM7/25/25

to 김민성, Koji Ishii, Rune Lillesveen, Kent Tamura, Olga Gerchikov, AyeAye, Alexis Menard, chromium...@chromium.org, Dirk Schulze, Fredrik Söderquist, Stephen Chenney, blink-revie...@chromium.org, mac-r...@chromium.org, apavlo...@chromium.org, blink-re...@chromium.org, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, jmedle...@chromium.org, kinuko...@chromium.org, kouhe...@chromium.org, pdr+svgw...@chromium.org, zol...@webkit.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

Support percentage values for word-spacing

This CL implements support for percentage values in the 'word-spacing'
property, as defined in the CSS Text Module Level 4 spec.

To achieve this, the type of 'word-spacing' in FontDescription is
changed from float to Length, allowing proper inheritance and improved
compatibility with computed style resolution.

And also merges the implementation from crrev.com/c/6651574

Spec: https://www.w3.org/TR/css-text-4/#word-spacing-property

Bug: 327740939

Change-Id: If9802e0cbe9fa2fbfaf77892e314e2147beac026

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

Commit-Queue: 김민성 <jja0...@gmail.com>

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

Reviewed-by: Koji Ishii <ko...@chromium.org>

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

Files:

M third_party/blink/renderer/build/scripts/core/style/templates/computed_style_initial_values.h.tmpl
M third_party/blink/renderer/core/animation/length_property_functions.cc
M third_party/blink/renderer/core/css/css_properties.json5
M third_party/blink/renderer/core/css/css_property_equality.cc
M third_party/blink/renderer/core/css/properties/css_parsing_utils.cc
M third_party/blink/renderer/core/css/properties/css_parsing_utils.h
M third_party/blink/renderer/core/css/properties/longhands/longhands_custom.cc
M third_party/blink/renderer/core/css/resolver/style_adjuster.cc
M third_party/blink/renderer/core/css/resolver/style_builder_converter.cc
M third_party/blink/renderer/core/css/resolver/style_builder_converter.h
M third_party/blink/renderer/core/html/html_permission_element.cc
M third_party/blink/renderer/core/layout/svg/layout_svg_inline_text.cc
M third_party/blink/renderer/core/style/computed_style.h
M third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d_state.cc
M third_party/blink/renderer/platform/exported/web_font_description.cc
M third_party/blink/renderer/platform/fonts/fallback_list_composite_key_test.cc
M third_party/blink/renderer/platform/fonts/font_description.cc
M third_party/blink/renderer/platform/fonts/font_description.h
M third_party/blink/renderer/platform/fonts/font_description_test.cc
M third_party/blink/renderer/platform/fonts/font_test.cc
M third_party/blink/renderer/platform/fonts/shaping/shape_result_test.cc
M third_party/blink/renderer/platform/runtime_enabled_features.json5
M third_party/blink/web_tests/TestExpectations
M third_party/blink/web_tests/css2.1/20110323/c541-word-sp-001.htm
M third_party/blink/web_tests/css3/calc/word-spacing.html
D third_party/blink/web_tests/external/wpt/css/css-text/parsing/word-spacing-computed-expected.txt
D third_party/blink/web_tests/external/wpt/css/css-text/parsing/word-spacing-valid-expected.txt
D third_party/blink/web_tests/external/wpt/css/css-transitions/properties-value-001-expected.txt
D third_party/blink/web_tests/external/wpt/css/css-transitions/properties-value-inherit-002-expected.txt
M third_party/blink/web_tests/external/wpt/css/css-typed-om/the-stylepropertymap/properties/word-spacing-expected.txt
M third_party/blink/web_tests/external/wpt/web-animations/animation-model/animation-types/accumulation-per-property-002-expected.txt
M third_party/blink/web_tests/external/wpt/web-animations/animation-model/animation-types/addition-per-property-002-expected.txt
M third_party/blink/web_tests/external/wpt/web-animations/animation-model/animation-types/interpolation-per-property-002-expected.txt
M third_party/blink/web_tests/platform/linux/css1/text_properties/word_spacing-expected.png
M third_party/blink/web_tests/platform/mac/css1/text_properties/word_spacing-expected.png
M third_party/blink/web_tests/platform/win/css1/text_properties/word_spacing-expected.png
M third_party/blink/web_tests/platform/win10/css1/text_properties/word_spacing-expected.png

Change size: L

Delta: 37 files changed, 107 insertions(+), 165 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Koji Ishii, +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

Reply all

Reply to author

Forward

Support percentage values for word-spacing [chromium/src : main]

김민성 (Gerrit)

김민성 added 1 comment

Related details

김민성 (Gerrit)

김민성 added 1 comment

Related details

김민성 (Gerrit)

김민성 added 1 comment

Related details

Rune Lillesveen (Gerrit)

Rune Lillesveen added 3 comments

Related details

김민성 (Gerrit)

김민성 added 3 comments

Related details

김민성 (Gerrit)

김민성 added 1 comment

Related details

Rune Lillesveen (Gerrit)

Rune Lillesveen added 1 comment

Related details

김민성 (Gerrit)

김민성 added 1 comment

Related details

김민성 (Gerrit)

김민성 added 1 comment

Related details

Kent Tamura (Gerrit)

Kent Tamura added 2 comments

Related details

김민성 (Gerrit)

김민성 added 2 comments

Related details

Kent Tamura (Gerrit)

Kent Tamura added 1 comment

Related details

김민성 (Gerrit)

김민성 added 1 comment

Related details

김민성 (Gerrit)

김민성 added 1 comment

Related details

Rune Lillesveen (Gerrit)

Rune Lillesveen voted and added 2 comments

Votes added by Rune Lillesveen

2 comments

Related details

Koji Ishii (Gerrit)

Koji Ishii voted and added 1 comment

Votes added by Koji Ishii

1 comment

Related details

김민성 (Gerrit)

김민성 voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information