Koji Ishii (Gerrit)

unread,

Jul 26, 2024, 2:25:42 AM (yesterday) Jul 26

to Kent Tamura, Ian Kilpatrick, Tricium, chrom...@appspot.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kinuko...@chromium.org, zol...@webkit.org

Attention needed from Ian Kilpatrick and Kent Tamura

Koji Ishii voted and added 1 comment

Votes added by Koji Ishii

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 24 (Latest):

Koji Ishii . resolved

PTAL.

Open in Gerrit

Related details

Attention is currently required from:

Ian Kilpatrick
Kent Tamura

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

Kent Tamura (Gerrit)

unread,

Jul 26, 2024, 2:49:26 AM (yesterday) Jul 26

to Koji Ishii, Kent Tamura, Ian Kilpatrick, Tricium, chrom...@appspot.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kinuko...@chromium.org, zol...@webkit.org

Attention needed from Ian Kilpatrick and Koji Ishii

Kent Tamura added 9 comments

File third_party/blink/renderer/platform/geometry/layout_unit.h

Line 522, Patchset 24 (Latest):// LayoutUnit::max() and ::min()

Kent Tamura . unresolved

LayoutUnit -> FixedPoint

Line 451, Patchset 24 (Latest): // value_ = value << kFractionalBits;

Kent Tamura . unresolved

Remove this line.

Line 150, Patchset 24 (Latest):

  // The specified `value` is rounded to a multiple of 1/64, and
  // is clamped by Min() and Max().
  // A NaN `value` produces LayoutUnit(0).

Kent Tamura . unresolved

Ditto.

Line 143, Patchset 24 (Latest):

  // The specified `value` is truncated to a multiple of 1/64, and is clamped
  // by Min() and Max().
  // A NaN `value` produces LayoutUnit(0).

Kent Tamura . unresolved

Ditto.

Line 136, Patchset 24 (Latest):

  // The specified `value` is rounded up to a multiple of 1/64, and
  // is clamped by Min() and Max().
  // A NaN `value` produces LayoutUnit(0).

Kent Tamura . unresolved

Ditto.

Line 130, Patchset 24 (Latest): // A NaN `value` produces LayoutUnit(0).

Kent Tamura . unresolved

Should be `FixedPoint(0)`.

Line 128, Patchset 24 (Latest): // The specified `value` is truncated to a multiple of 1/64 near 0, and

Kent Tamura . unresolved

should be `Epsilon()`

Line 107, Patchset 24 (Latest):

  // Creates a LayoutUnit with the specified integer value.
  // If the specified value is smaller than LayoutUnit::Min(), the new
  // LayoutUnit is equivalent to LayoutUnit::Min().
  // If the specified value is greater than the maximum integer value which
  // LayoutUnit can represent, the new LayoutUnit is equivalent to
  // LayoutUnit(LayoutUnit::kIntMax) in 32-bit Arm, or is equivalent to
  // LayoutUnit::Max() otherwise.

Kent Tamura . unresolved

This comment needs to be updated for `FixedPoint`.

File third_party/blink/renderer/platform/geometry/layout_unit.cc

Line 55, Patchset 24 (Latest):#if 1

Kent Tamura . unresolved

Remove this `#if 1`, and the `else` part.

Open in Gerrit

Related details

Attention is currently required from:

Ian Kilpatrick
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

Koji Ishii (Gerrit)

unread,

Jul 26, 2024, 3:09:33 AM (yesterday) Jul 26

to Kent Tamura, Ian Kilpatrick, Tricium, chrom...@appspot.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kinuko...@chromium.org, zol...@webkit.org

Attention needed from Ian Kilpatrick and Kent Tamura

Koji Ishii added 9 comments

File third_party/blink/renderer/platform/geometry/layout_unit.h

Line 522, Patchset 24:// LayoutUnit::max() and ::min()

Kent Tamura . resolved

LayoutUnit -> FixedPoint

Koji Ishii

Done

Line 451, Patchset 24: // value_ = value << kFractionalBits;

Kent Tamura . resolved

Remove this line.

Koji Ishii

Done

Line 150, Patchset 24: // The specified `value` is rounded to a multiple of 1/64, and


  // is clamped by Min() and Max().
  // A NaN `value` produces LayoutUnit(0).

Kent Tamura . resolved

Ditto.

Koji Ishii

Done

Line 143, Patchset 24: // The specified `value` is truncated to a multiple of 1/64, and is clamped


  // by Min() and Max().
  // A NaN `value` produces LayoutUnit(0).

Kent Tamura . resolved

Ditto.

Koji Ishii

Done

Line 136, Patchset 24: // The specified `value` is rounded up to a multiple of 1/64, and


  // is clamped by Min() and Max().
  // A NaN `value` produces LayoutUnit(0).

Kent Tamura . resolved

Ditto.

Koji Ishii

Done

Line 130, Patchset 24: // A NaN `value` produces LayoutUnit(0).

Kent Tamura . resolved

Should be `FixedPoint(0)`.

Koji Ishii

Done

Line 128, Patchset 24: // The specified `value` is truncated to a multiple of 1/64 near 0, and

Kent Tamura . resolved

should be `Epsilon()`

Koji Ishii

Done

Line 107, Patchset 24: // Creates a LayoutUnit with the specified integer value.


  // If the specified value is smaller than LayoutUnit::Min(), the new
  // LayoutUnit is equivalent to LayoutUnit::Min().
  // If the specified value is greater than the maximum integer value which
  // LayoutUnit can represent, the new LayoutUnit is equivalent to
  // LayoutUnit(LayoutUnit::kIntMax) in 32-bit Arm, or is equivalent to
  // LayoutUnit::Max() otherwise.

Kent Tamura . resolved

This comment needs to be updated for `FixedPoint`.

Koji Ishii

Done

File third_party/blink/renderer/platform/geometry/layout_unit.cc

Line 55, Patchset 24:#if 1

Kent Tamura . resolved

Remove this `#if 1`, and the `else` part.

Koji Ishii

Done, oops, thanks for catching this.

Open in Gerrit

Related details

Attention is currently required from:

Ian Kilpatrick
Kent Tamura

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

Kent Tamura (Gerrit)

unread,

Jul 26, 2024, 4:02:07 AM (yesterday) Jul 26

to Koji Ishii, Kent Tamura, Ian Kilpatrick, Tricium, chrom...@appspot.gserviceaccount.com, Chromium LUCI CQ, chromium...@chromium.org, Dirk Schulze, Stephen Chenney, blink-rev...@chromium.org, blink-revi...@chromium.org, blink-rev...@chromium.org, blink-reviews-p...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, fserb...@chromium.org, kinuko...@chromium.org, zol...@webkit.org

Attention needed from Ian Kilpatrick and Koji Ishii

Kent Tamura added 4 comments

File third_party/blink/renderer/platform/geometry/layout_unit.h

Line 536, Patchset 26 (Latest): static_cast<int>(result));

Kent Tamura . unresolved

nit: Should be `int32_t` or `RawValue`.

Line 529, Patchset 26 (Latest): (static_cast<uint32_t>(a.RawValue() ^ b.RawValue()) >> 31) + kRawValueMax;

Kent Tamura . unresolved

should be `FixedPoint<fractional_bits, int32_t>::kRawValueMax` or `FixedPoint<fractional_bits, RawValue>::kRawValueMax`.

Line 525, Patchset 26 (Latest): FixedPoint<fractional_bits, int>::kFixedPointDenominator;

Kent Tamura . unresolved

nit: `RawValue` or `int32_t` rather than `int` for consistency with the `requires` above.

Line 340, Patchset 26 (Latest):

  static bool IsInBounds(int value) {
    return ::abs(value) <= kRawValueMax / kFixedPointDenominator;
  }
  static bool IsInBounds(unsigned value) {
    return value <=

Kent Tamura . unresolved

Do we need to update them for `StorageType` and `UnsignedStorageType`?

Open in Gerrit

Related details

Attention is currently required from:

Ian Kilpatrick
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

Reply all

Reply to author

Forward

[layout-unit] Templatize `LayoutUnit` [chromium/src : main]

Koji Ishii (Gerrit)

Koji Ishii voted and added 1 comment

Votes added by Koji Ishii

1 comment

Related details

Kent Tamura (Gerrit)

Kent Tamura added 9 comments

Related details

Koji Ishii (Gerrit)

Koji Ishii added 9 comments

Related details

Kent Tamura (Gerrit)

Kent Tamura added 4 comments

Related details