김민성 (Gerrit)

unread,

Jun 19, 2025, 9:59:46 AMJun 19

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 PMJun 19

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 AMJun 20

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 AMJun 23

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 AMJun 23

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 AMJun 23

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 AMJun 24

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 AMJun 24

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 PMJun 29

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 PMJul 1

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 AM (14 days ago) Jul 2

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 PM (13 days ago) Jul 2

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 PM (13 days ago) Jul 2

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

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