Chuong Ho-Dac (Gerrit)

unread,

Jan 12, 2026, 2:12:09 AM (8 days ago) Jan 12

to Mirko Bonadei, Jerome Jiang, Code Review Nudger, Sam McNally, Koji Ishii, Arthur Sonzogni, Nico Weber, Philip Rogers, Bao-Duy Tran, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, penghuan...@chromium.org, browser-comp...@chromium.org, fuzzin...@chromium.org, fgal...@chromium.org, mar...@chromium.org, jz...@chromium.org, devtools...@chromium.org, feature-me...@chromium.org, chrome-intell...@chromium.org, cblume...@chromium.org, chrome-intelligence-te...@google.com, droger+w...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, xiaochen...@chromium.org, mac-r...@chromium.org, ipc-securi...@chromium.org

Attention needed from Arthur Sonzogni, Bao-Duy Tran, Nico Weber and Philip Rogers

Chuong Ho-Dac added 8 comments

File components/spellcheck/browser/android/java/src/org/chromium/components/spellcheck/SpellCheckerSessionBridge.java

Line 125, Patchset 17: : SuggestionSpan.FLAG_GRAMMAR_ERROR),

Bao-Duy Tran . resolved

should this be flipped around, so the default (incl. for when there's a 3rd SpellingMarker.Decoration added, but this code isn't updated) remains FLAG_MISSPELLED, which looks more reasonable to be the default? Lines 184-185 also use SPELLING as default.

Chuong Ho-Dac

Thanks, that soudns good

Line 127, Patchset 19: Math.min(marker.end(), text.length() - 1),

Arthur Sonzogni . resolved

In follow-up: I will let you double check. Previously the `gfx::Range` documented that `end` was exclusive. So there is an off-by-one error here.
If your SpellingMarker is using an inclusive range, then this is correct.

Chuong Ho-Dac

Great thanks for spotting that. It should be inclusive-exclusive.

File components/spellcheck/browser/android/java/src/org/chromium/components/spellcheck/SpellingMarker.java

Line 26, Patchset 17: mStart = Math.min(start, end);

Bao-Duy Tran . resolved

nit: also clamp to non-negative?

Chuong Ho-Dac

I've added the assert's.

File components/spellcheck/browser/spell_check_host_impl.cc

Line 19, Patchset 19: if (marker.start >= text_length || marker.end >= text_length) {

Arthur Sonzogni . resolved

In follow-up: I will let you double check. Previously the `gfx::Range` documented that `end` was exclusive. So there is an off-by-one error here.

If your SpellingMarker is using an inclusive range, then this is correct.

Chuong Ho-Dac

fixed. Thanks!

File components/spellcheck/browser/spellchecker_session_bridge_android.h

Line 71, Patchset 19: const std::vector<spellcheck::SpellingMarker> spelling_markers_;

Bao-Duy Tran . resolved

nit: why const now? maybe done separately if really needed.

Chuong Ho-Dac

it should be `const` because the spelling request is meant to have spelling markers fixed but I agree it should be a different CL.

File components/spellcheck/common/spellcheck.mojom

Line 91, Patchset 19:struct SpellingMarker {

Arthur Sonzogni . resolved

Could you add a description. In particular, I would be interested to know if `end` is inclusive or exclusive.
Note that the `gfx::Range` description was:
```
// This class represents either a forward range [min, max) or a reverse range
// (max, min]. |start_| is always the first of these and |end_| the second; as a
// result, the range is forward if (start_ <= end_). The zero-width range
// [val, val) is legal, contains and intersects itself, and is contained by and
// intersects any nonempty range [min, max) where min <= val < max.
```

Chuong Ho-Dac

End should be exclusive. So it should be [start, end)

File components/spellcheck/common/spellcheck_mojom_traits.cc

Line 61, Patchset 19: output->start = std::min(input.start(), input.end());

Bao-Duy Tran . resolved

nit: also clamp to non-negative?

Chuong Ho-Dac

probably no need, since the SpellingMarker in mojom, we define the start and end to be non-negative already?

```
struct SpellingMarker {
  uint32 start;
  uint32 end;
  Decoration marker_type;
};
```

Line 61, Patchset 19:

  output->start = std::min(input.start(), input.end());
  output->end = std::max(input.start(), input.end());

Arthur Sonzogni . resolved

Note that gfx::Range could have end < start. I will let you double check what makes sense in your case.
----
If we disallow negative range, maybe this should be written:
```
if (input.start() > input.end()) {
  return false;
}
output->start = input.start();
output->end = input.end();
```
?
This would avoid potential error to go unnoticed.

Chuong Ho-Dac

Unlike a normal selection that has direction (users can select from beginning -> end of a word or from the end of the word to the beginning), the spelling markers are just a underline and should be non-directional.

https://source.chromium.org/chromium/chromium/src/+/main:components/spellcheck/browser/android/java/src/org/chromium/components/spellcheck/SpellCheckerSessionBridge.java;l=186-187?q=processSpellCheckResults%20-f:out

Spelling marker underlines are constructed from `offset` and `offset + length`, therefore `start` must be <= `end`. The range should also be [inclusive, exclusive)

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni
Bao-Duy Tran
Nico Weber
Philip Rogers

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

Bao-Duy Tran (Gerrit)

unread,

Jan 12, 2026, 3:20:39 AM (8 days ago) Jan 12

to Chuong Ho-Dac, Mirko Bonadei, Jerome Jiang, Code Review Nudger, Sam McNally, Koji Ishii, Arthur Sonzogni, Nico Weber, Philip Rogers, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, penghuan...@chromium.org, browser-comp...@chromium.org, fuzzin...@chromium.org, fgal...@chromium.org, mar...@chromium.org, jz...@chromium.org, devtools...@chromium.org, feature-me...@chromium.org, chrome-intell...@chromium.org, cblume...@chromium.org, chrome-intelligence-te...@google.com, droger+w...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, xiaochen...@chromium.org, mac-r...@chromium.org, ipc-securi...@chromium.org

Attention needed from Arthur Sonzogni, Chuong Ho-Dac, Nico Weber and Philip Rogers

Bao-Duy Tran added 1 comment

Patchset-level comments

File-level comment, Patchset 22 (Latest):

Bao-Duy Tran . unresolved

Patchset 22 (latest) looks similar to my last-reviewed Patchset 19, except for copyright year 2025 -> 2026. Rebase or upload hiccoughs?

Open in Gerrit

Related details

Attention is currently required from:

Arthur Sonzogni
Chuong Ho-Dac
Nico Weber
Philip Rogers

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

satisfied_requirement

unsatisfied_requirement

open

diffy

Arthur Sonzogni (Gerrit)

unread,

Jan 12, 2026, 4:04:31 AM (8 days ago) Jan 12

to Chuong Ho-Dac, Mirko Bonadei, Jerome Jiang, Code Review Nudger, Sam McNally, Koji Ishii, Nico Weber, Philip Rogers, Bao-Duy Tran, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, penghuan...@chromium.org, browser-comp...@chromium.org, fuzzin...@chromium.org, fgal...@chromium.org, mar...@chromium.org, jz...@chromium.org, devtools...@chromium.org, feature-me...@chromium.org, chrome-intell...@chromium.org, cblume...@chromium.org, chrome-intelligence-te...@google.com, droger+w...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, xiaochen...@chromium.org, mac-r...@chromium.org, ipc-securi...@chromium.org

Attention needed from Chuong Ho-Dac, Nico Weber and Philip Rogers

Arthur Sonzogni voted and added 6 comments

Votes added by Arthur Sonzogni

Code-Review

+1

6 comments

Patchset-level comments

File-level comment, Patchset 22 (Latest):

Arthur Sonzogni . resolved

LGTM with nits:

File components/spellcheck/common/spellcheck.mojom

Line 91, Patchset 22 (Latest):struct SpellingMarker {

Arthur Sonzogni . unresolved

Could you please:

Add class level comment.
Document invariants: start<=end?

File components/spellcheck/common/spelling_marker.h

Line 14, Patchset 22 (Latest):

// This class represents spelling markers, i.e. Spelling, Grammar.
struct SpellingMarker {
  SpellingMarker(uint32_t start, uint32_t end, Decoration marker_type);
  SpellingMarker();

  SpellingMarker(const SpellingMarker&);
  SpellingMarker& operator=(const SpellingMarker&);

  ~SpellingMarker();

  bool operator==(const SpellingMarker& other) const;

  uint32_t start;
  uint32_t end;
  Decoration marker_type;
};

Arthur Sonzogni . unresolved

IMO, this is a POD.

I would just define it as:
```
struct SpellingMarker {
  auto operator<=>(const SpellingMarker& other) const = default;

  uint32_t start;
  uint32_t end;
  Decoration marker_type;  
}
```

Then you get nice default, and you don't need to define any function?

Line 24, Patchset 22 (Latest): bool operator==(const SpellingMarker& other) const;

Arthur Sonzogni . unresolved

What about a defaulted `<` `<=` `==` `=>` `>` operators?
```
auto operator<=>(const SpellingMarker& other) const = default.
```

File components/spellcheck/renderer/spellcheck_provider.cc

Line 62, Patchset 22 (Latest):

    default:
      NOTREACHED();
  }

Arthur Sonzogni . unresolved

ditto
```suggestion
  }
  NOTREACHED();
```

File third_party/blink/renderer/core/editing/spellcheck/spell_check_requester.cc

Line 57, Patchset 22 (Latest):

WebTextCheckClient::SpellingMarkerType MapToWebSpellingMarkerType(
    DocumentMarker::MarkerType marker_type) {
  switch (marker_type) {
    case DocumentMarker::MarkerType::kSpelling:
      return WebTextCheckClient::SpellingMarkerType::kSpelling;
    case DocumentMarker::MarkerType::kGrammar:
      return WebTextCheckClient::SpellingMarkerType::kGrammar;
    default:
      NOTREACHED();
  }
}

Arthur Sonzogni . unresolved

Prefer switch statement to be exhaustive. This will make the compiler to emit an error when new values are added, but we don't yet support it.
```suggestion
WebTextCheckClient::SpellingMarkerType MapToWebSpellingMarkerType(
    DocumentMarker::MarkerType marker_type) {
  switch (marker_type) {
    case DocumentMarker::MarkerType::kSpelling:
      return WebTextCheckClient::SpellingMarkerType::kSpelling;
    case DocumentMarker::MarkerType::kGrammar:
      return WebTextCheckClient::SpellingMarkerType::kGrammar;
  }
  NOTREACHED();
}
```

Open in Gerrit

Related details

Attention is currently required from:

Chuong Ho-Dac
Nico Weber
Philip Rogers

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

No-Unresolved-Comments

Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Philip Rogers (Gerrit)

unread,

Jan 12, 2026, 2:08:26 PM (8 days ago) Jan 12

to Chuong Ho-Dac, Arthur Sonzogni, Mirko Bonadei, Jerome Jiang, Code Review Nudger, Sam McNally, Koji Ishii, Nico Weber, Bao-Duy Tran, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, penghuan...@chromium.org, browser-comp...@chromium.org, fuzzin...@chromium.org, fgal...@chromium.org, mar...@chromium.org, jz...@chromium.org, devtools...@chromium.org, feature-me...@chromium.org, chrome-intell...@chromium.org, cblume...@chromium.org, chrome-intelligence-te...@google.com, droger+w...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, xiaochen...@chromium.org, mac-r...@chromium.org, ipc-securi...@chromium.org

Attention needed from Chuong Ho-Dac and Nico Weber

Philip Rogers voted and added 1 comment

Votes added by Philip Rogers

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 23 (Latest):

Philip Rogers . resolved

blink LGTM

Open in Gerrit

Related details

Attention is currently required from:

Chuong Ho-Dac
Nico Weber

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Chuong Ho-Dac (Gerrit)

unread,

Jan 12, 2026, 6:43:24 PM (7 days ago) Jan 12

to Philip Rogers, Arthur Sonzogni, Mirko Bonadei, Jerome Jiang, Code Review Nudger, Sam McNally, Koji Ishii, Nico Weber, Bao-Duy Tran, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, penghuan...@chromium.org, browser-comp...@chromium.org, fuzzin...@chromium.org, fgal...@chromium.org, mar...@chromium.org, jz...@chromium.org, devtools...@chromium.org, feature-me...@chromium.org, chrome-intell...@chromium.org, cblume...@chromium.org, chrome-intelligence-te...@google.com, droger+w...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, xiaochen...@chromium.org, mac-r...@chromium.org, ipc-securi...@chromium.org

Attention needed from Bao-Duy Tran and Nico Weber

Chuong Ho-Dac added 6 comments

Patchset-level comments

File-level comment, Patchset 22:

Bao-Duy Tran . resolved

Patchset 22 (latest) looks similar to my last-reviewed Patchset 19, except for copyright year 2025 -> 2026. Rebase or upload hiccoughs?

Chuong Ho-Dac

Done, I forgot to upload.

File components/spellcheck/common/spellcheck.mojom

Line 91, Patchset 22:struct SpellingMarker {

Arthur Sonzogni . resolved

Could you please:
Add class level comment.
Document invariants: start<=end?

Chuong Ho-Dac

Done

File components/spellcheck/common/spelling_marker.h

Line 14, Patchset 22:// This class represents spelling markers, i.e. Spelling, Grammar.


struct SpellingMarker {
  SpellingMarker(uint32_t start, uint32_t end, Decoration marker_type);
  SpellingMarker();

  SpellingMarker(const SpellingMarker&);
  SpellingMarker& operator=(const SpellingMarker&);

  ~SpellingMarker();

  bool operator==(const SpellingMarker& other) const;

  uint32_t start;
  uint32_t end;
  Decoration marker_type;
};

Arthur Sonzogni . resolved

IMO, this is a POD.
I would just define it as:
```
struct SpellingMarker {
  auto operator<=>(const SpellingMarker& other) const = default;
  uint32_t start;
  uint32_t end;
  Decoration marker_type;  
}
```
Then you get nice default, and you don't need to define any function?

Chuong Ho-Dac

Done

Line 24, Patchset 22: bool operator==(const SpellingMarker& other) const;

Arthur Sonzogni . resolved

What about a defaulted `<` `<=` `==` `=>` `>` operators?
```
auto operator<=>(const SpellingMarker& other) const = default.
```

Chuong Ho-Dac

Done

File components/spellcheck/renderer/spellcheck_provider.cc

Line 62, Patchset 22:

    default:
      NOTREACHED();
  }

Arthur Sonzogni . resolved

ditto
```suggestion
  }
  NOTREACHED();
```

Chuong Ho-Dac

Done

File third_party/blink/renderer/core/editing/spellcheck/spell_check_requester.cc

Line 57, Patchset 22:WebTextCheckClient::SpellingMarkerType MapToWebSpellingMarkerType(


    DocumentMarker::MarkerType marker_type) {
  switch (marker_type) {
    case DocumentMarker::MarkerType::kSpelling:
      return WebTextCheckClient::SpellingMarkerType::kSpelling;
    case DocumentMarker::MarkerType::kGrammar:
      return WebTextCheckClient::SpellingMarkerType::kGrammar;
    default:
      NOTREACHED();
  }
}

Arthur Sonzogni . resolved

Prefer switch statement to be exhaustive. This will make the compiler to emit an error when new values are added, but we don't yet support it.
```suggestion
WebTextCheckClient::SpellingMarkerType MapToWebSpellingMarkerType(
    DocumentMarker::MarkerType marker_type) {
  switch (marker_type) {
    case DocumentMarker::MarkerType::kSpelling:
      return WebTextCheckClient::SpellingMarkerType::kSpelling;
    case DocumentMarker::MarkerType::kGrammar:
      return WebTextCheckClient::SpellingMarkerType::kGrammar;
  }
  NOTREACHED();
}
```

Chuong Ho-Dac

Done

Open in Gerrit

Related details

Attention is currently required from:

Bao-Duy Tran
Nico Weber

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

Bao-Duy Tran (Gerrit)

unread,

Jan 13, 2026, 12:16:07 AM (7 days ago) Jan 13

to Chuong Ho-Dac, Philip Rogers, Arthur Sonzogni, Mirko Bonadei, Jerome Jiang, Code Review Nudger, Sam McNally, Koji Ishii, Nico Weber, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, penghuan...@chromium.org, browser-comp...@chromium.org, fuzzin...@chromium.org, fgal...@chromium.org, mar...@chromium.org, jz...@chromium.org, devtools...@chromium.org, feature-me...@chromium.org, chrome-intell...@chromium.org, cblume...@chromium.org, chrome-intelligence-te...@google.com, droger+w...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, xiaochen...@chromium.org, mac-r...@chromium.org, ipc-securi...@chromium.org

Attention needed from Chuong Ho-Dac and Nico Weber

Bao-Duy Tran added 3 comments

File components/spellcheck/browser/android/java/src/org/chromium/components/spellcheck/SpellCheckerSessionBridge.java

Line 123, Patchset 26 (Latest): marker.type() != SpellingMarker.Decoration.SPELLING

Bao-Duy Tran . unresolved

"type == GRAMMAR ? GRAMMAR : SPELLING)", else it's effectively still "GRAMMAR is default", while I reckon SPELLING had better be the default.

File components/spellcheck/browser/android/java/src/org/chromium/components/spellcheck/SpellingMarker.java

Line 26, Patchset 26 (Latest):

        assert start >= 0;
        assert end >= 0;

Bao-Duy Tran . unresolved

Should "start <= end" condition be similarly checked?
I think generally runtime IllegalArgumentException is preferred.

Refs:

go/java-practices/assertions
http://go/java-practices/preconditions

File components/spellcheck/common/spelling_marker.h

Line 26, Patchset 26 (Latest): auto operator<=>(const SpellingMarker& other) const = default;

Bao-Duy Tran . unresolved

I don't think autogen default operator< and operator> make sense for a SpellingMarker.

Open in Gerrit

Related details

Attention is currently required from:

Chuong Ho-Dac
Nico Weber

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Chuong Ho-Dac (Gerrit)

unread,

Jan 15, 2026, 10:39:42 PM (4 days ago) Jan 15

to Philip Rogers, Arthur Sonzogni, Mirko Bonadei, Jerome Jiang, Code Review Nudger, Sam McNally, Koji Ishii, Nico Weber, Bao-Duy Tran, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, penghuan...@chromium.org, browser-comp...@chromium.org, fuzzin...@chromium.org, fgal...@chromium.org, mar...@chromium.org, jz...@chromium.org, devtools...@chromium.org, feature-me...@chromium.org, chrome-intell...@chromium.org, cblume...@chromium.org, chrome-intelligence-te...@google.com, droger+w...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, xiaochen...@chromium.org, mac-r...@chromium.org, ipc-securi...@chromium.org

Attention needed from Bao-Duy Tran and Nico Weber

Chuong Ho-Dac added 3 comments

File components/spellcheck/browser/android/java/src/org/chromium/components/spellcheck/SpellCheckerSessionBridge.java

Line 123, Patchset 26 (Latest): marker.type() != SpellingMarker.Decoration.SPELLING

Bao-Duy Tran . resolved

"type == GRAMMAR ? GRAMMAR : SPELLING)", else it's effectively still "GRAMMAR is default", while I reckon SPELLING had better be the default.

Chuong Ho-Dac

Done

File components/spellcheck/browser/android/java/src/org/chromium/components/spellcheck/SpellingMarker.java

Line 26, Patchset 26 (Latest):

        assert start >= 0;
        assert end >= 0;

Bao-Duy Tran . resolved

Should "start <= end" condition be similarly checked?
I think generally runtime IllegalArgumentException is preferred.
Refs:
go/java-practices/assertions
http://go/java-practices/preconditions

Chuong Ho-Dac

Done

File components/spellcheck/common/spelling_marker.h

Line 26, Patchset 26 (Latest): auto operator<=>(const SpellingMarker& other) const = default;

Bao-Duy Tran . resolved

I don't think autogen default operator< and operator> make sense for a SpellingMarker.

Chuong Ho-Dac

I agreed, just revert to ==

Open in Gerrit

Related details

Attention is currently required from:

Bao-Duy Tran
Nico Weber

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

Bao-Duy Tran (Gerrit)

unread,

Jan 16, 2026, 12:49:49 AM (4 days ago) Jan 16

to Chuong Ho-Dac, Philip Rogers, Arthur Sonzogni, Mirko Bonadei, Jerome Jiang, Code Review Nudger, Sam McNally, Koji Ishii, Nico Weber, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, penghuan...@chromium.org, browser-comp...@chromium.org, fuzzin...@chromium.org, fgal...@chromium.org, mar...@chromium.org, jz...@chromium.org, devtools...@chromium.org, feature-me...@chromium.org, chrome-intell...@chromium.org, cblume...@chromium.org, chrome-intelligence-te...@google.com, droger+w...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, xiaochen...@chromium.org, mac-r...@chromium.org, ipc-securi...@chromium.org

Attention needed from Chuong Ho-Dac and Nico Weber

Bao-Duy Tran voted and added 3 comments

Votes added by Bao-Duy Tran

Code-Review

+1

3 comments

Patchset-level comments

File-level comment, Patchset 29 (Latest):

Bao-Duy Tran . resolved

LGTM with nits

File components/spellcheck/browser/android/java/src/org/chromium/components/spellcheck/SpellingMarker.java

Line 53, Patchset 29 (Latest): public boolean equals(Object o) {

Bao-Duy Tran . unresolved

[nit, Java] Sorry I missed this earlier. Should also implement hashCode(), even though this hasn't participated in hash-related stuff now. Ref: http://go/java-practices/equals

Line 61, Patchset 29 (Latest): public String toString() {

Bao-Duy Tran . unresolved

[nit, optional, Java] Sorry I missed this earlier. Optionally consider String.format() with placeholders, instead of "+" concats.

Open in Gerrit

Related details

Attention is currently required from:

Chuong Ho-Dac
Nico Weber

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Chuong Ho-Dac (Gerrit)

unread,

Jan 18, 2026, 10:13:36 PM (2 days ago) Jan 18

to Bao-Duy Tran, Philip Rogers, Arthur Sonzogni, Mirko Bonadei, Jerome Jiang, Code Review Nudger, Sam McNally, Koji Ishii, Nico Weber, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, penghuan...@chromium.org, browser-comp...@chromium.org, fuzzin...@chromium.org, fgal...@chromium.org, mar...@chromium.org, jz...@chromium.org, devtools...@chromium.org, feature-me...@chromium.org, chrome-intell...@chromium.org, cblume...@chromium.org, chrome-intelligence-te...@google.com, droger+w...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, xiaochen...@chromium.org, mac-r...@chromium.org, ipc-securi...@chromium.org

Attention needed from Nico Weber

Chuong Ho-Dac added 2 comments

File components/spellcheck/browser/android/java/src/org/chromium/components/spellcheck/SpellingMarker.java

Line 53, Patchset 29: public boolean equals(Object o) {

Bao-Duy Tran . resolved

[nit, Java] Sorry I missed this earlier. Should also implement hashCode(), even though this hasn't participated in hash-related stuff now. Ref: http://go/java-practices/equals

Chuong Ho-Dac

Done

Line 61, Patchset 29: public String toString() {

Bao-Duy Tran . resolved

[nit, optional, Java] Sorry I missed this earlier. Optionally consider String.format() with placeholders, instead of "+" concats.

Chuong Ho-Dac

Done

Open in Gerrit

Related details

Attention is currently required from:

Nico Weber

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

Hidehiko Abe (Gerrit)

unread,

1:11 AM (5 hours ago) 1:11 AM

to Chuong Ho-Dac, Bao-Duy Tran, Philip Rogers, Arthur Sonzogni, Mirko Bonadei, Jerome Jiang, Code Review Nudger, Sam McNally, Koji Ishii, Nico Weber, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, penghuan...@chromium.org, browser-comp...@chromium.org, fuzzin...@chromium.org, fgal...@chromium.org, mar...@chromium.org, jz...@chromium.org, devtools...@chromium.org, feature-me...@chromium.org, chrome-intell...@chromium.org, cblume...@chromium.org, chrome-intelligence-te...@google.com, droger+w...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, xiaochen...@chromium.org, mac-r...@chromium.org, ipc-securi...@chromium.org

Attention needed from Chuong Ho-Dac and Nico Weber

Hidehiko Abe voted and added 1 comment

Votes added by Hidehiko Abe

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 31 (Latest):

Hidehiko Abe . resolved

chrome/browser/site_isolation/spellcheck_per_process_browsertest.cc LGTM.

Open in Gerrit

Related details

Attention is currently required from:

Chuong Ho-Dac
Nico Weber

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

open

diffy

Chuong Ho-Dac (Gerrit)

unread,

1:11 AM (5 hours ago) 1:11 AM

to Hidehiko Abe, Bao-Duy Tran, Philip Rogers, Arthur Sonzogni, Mirko Bonadei, Jerome Jiang, Code Review Nudger, Sam McNally, Koji Ishii, Nico Weber, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, penghuan...@chromium.org, browser-comp...@chromium.org, fuzzin...@chromium.org, fgal...@chromium.org, mar...@chromium.org, jz...@chromium.org, devtools...@chromium.org, feature-me...@chromium.org, chrome-intell...@chromium.org, cblume...@chromium.org, chrome-intelligence-te...@google.com, droger+w...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, xiaochen...@chromium.org, mac-r...@chromium.org, ipc-securi...@chromium.org

Attention needed from Nico Weber

Chuong Ho-Dac voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention is currently required from:

Nico Weber

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

2:18 AM (4 hours ago) 2:18 AM

to Chuong Ho-Dac, Hidehiko Abe, Bao-Duy Tran, Philip Rogers, Arthur Sonzogni, Mirko Bonadei, Jerome Jiang, Code Review Nudger, Sam McNally, Koji Ishii, Nico Weber, AyeAye, chromium...@chromium.org, penghuan...@chromium.org, browser-comp...@chromium.org, fuzzin...@chromium.org, fgal...@chromium.org, mar...@chromium.org, jz...@chromium.org, devtools...@chromium.org, feature-me...@chromium.org, chrome-intell...@chromium.org, cblume...@chromium.org, chrome-intelligence-te...@google.com, droger+w...@chromium.org, blink-revi...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, xiaochen...@chromium.org, mac-r...@chromium.org, ipc-securi...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

spellcheck: supply full spelling marker info, incld. marker type

This CL sends the marker type (misspelling vs grammar) of the spelling
marker. Previously only misspelling markers were sent.

Bug: b:457574784

Change-Id: I4a78a17c362f337325d26f55879df7f8b110e025

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

Reviewed-by: Hidehiko Abe <hide...@chromium.org>

Commit-Queue: Chuong Ho-Dac <hdch...@chromium.org>

Reviewed-by: Arthur Sonzogni <arthurs...@chromium.org>

Reviewed-by: Bao-Duy Tran <tranb...@chromium.org>

Reviewed-by: Philip Rogers <p...@chromium.org>

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

Files:

M chrome/browser/site_isolation/spellcheck_per_process_browsertest.cc
M chrome/browser/spellchecker/spell_check_host_chrome_impl.cc
M chrome/browser/spellchecker/spell_check_host_chrome_impl.h
M components/spellcheck/browser/DEPS
M components/spellcheck/browser/android/BUILD.gn
M components/spellcheck/browser/android/java/src/org/chromium/components/spellcheck/SpellCheckerSessionBridge.java
A components/spellcheck/browser/android/java/src/org/chromium/components/spellcheck/SpellingMarker.java
M components/spellcheck/browser/spell_check_host_impl.cc
M components/spellcheck/browser/spell_check_host_impl.h
M components/spellcheck/browser/spellchecker_session_bridge_android.cc
M components/spellcheck/browser/spellchecker_session_bridge_android.h
M components/spellcheck/common/BUILD.gn
M components/spellcheck/common/spellcheck.mojom
M components/spellcheck/common/spellcheck_decoration.h
M components/spellcheck/common/spellcheck_mojom_traits.cc
M components/spellcheck/common/spellcheck_mojom_traits.h
A components/spellcheck/common/spelling_marker.cc
A components/spellcheck/common/spelling_marker.h
M components/spellcheck/renderer/DEPS
M components/spellcheck/renderer/spellcheck_provider.cc
M components/spellcheck/renderer/spellcheck_provider.h
M components/spellcheck/renderer/spellcheck_provider_mac_unittest.cc
M components/spellcheck/renderer/spellcheck_provider_test.cc
M components/spellcheck/renderer/spellcheck_provider_test.h
M content/web_test/renderer/spell_check_client.cc
M content/web_test/renderer/spell_check_client.h
M third_party/blink/public/web/web_text_check_client.h
M third_party/blink/renderer/core/editing/spellcheck/spell_check_requester.cc
M third_party/blink/renderer/core/editing/spellcheck/spell_check_requester.h
M third_party/blink/renderer/core/frame/local_frame.cc
M third_party/blink/renderer/core/frame/web_frame_test.cc

Change size: L

Delta: 31 files changed, 404 insertions(+), 138 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Hidehiko Abe, +1 by Philip Rogers, +1 by Arthur Sonzogni, +1 by Bao-Duy Tran

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

spellcheck: supply full spelling marker info, incld. marker type [chromium/src : main]

Chuong Ho-Dac (Gerrit)

Chuong Ho-Dac added 8 comments

Related details

Bao-Duy Tran (Gerrit)

Bao-Duy Tran added 1 comment

Related details

Arthur Sonzogni (Gerrit)

Arthur Sonzogni voted and added 6 comments

Votes added by Arthur Sonzogni

6 comments

Related details

Philip Rogers (Gerrit)

Philip Rogers voted and added 1 comment

Votes added by Philip Rogers

1 comment

Related details

Chuong Ho-Dac (Gerrit)

Chuong Ho-Dac added 6 comments

Related details

Bao-Duy Tran (Gerrit)

Bao-Duy Tran added 3 comments

Related details

Chuong Ho-Dac (Gerrit)

Chuong Ho-Dac added 3 comments

Related details

Bao-Duy Tran (Gerrit)

Bao-Duy Tran voted and added 3 comments

Votes added by Bao-Duy Tran

3 comments

Related details

Chuong Ho-Dac (Gerrit)

Chuong Ho-Dac added 2 comments

Related details

Hidehiko Abe (Gerrit)

Hidehiko Abe voted and added 1 comment

Votes added by Hidehiko Abe

1 comment

Related details

Chuong Ho-Dac (Gerrit)

Chuong Ho-Dac voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information