Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2932913002:
Move spellcheck underline colors into LayoutTheme (Closed)
|
Created:
3 years, 6 months ago by rlanday Modified:
3 years, 6 months ago Reviewers:
chrishtr CC:
blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, mac-reviews_chromium.org, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove spellcheck underline colors into LayoutTheme
I'm adding support in Chrome for the Android spellcheck menu in
https://codereview.chromium.org/2931443003. When a user taps on a misspelled
word, I need to highlight the word in a color based on the color of the
spellcheck underline (we add some transparency). We're trying to avoid
hard-coding the spellcheck color in editing code. Therefore, we think it makes
sense to move the colors for spelling/grammar markers out of
InlineTextBoxPainter into LayoutTheme, so other code can make use of them.
BUG=715365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2932913002
Cr-Commit-Position: refs/heads/master@{#478755}
Committed: https://chromium.googlesource.com/chromium/src/+/61856d2266ce3b76ac8485a9ad2091d18842472a
Patch Set 1 #Patch Set 2 : Fix Mac build #
Total comments: 3
Patch Set 3 : Remove redundant wrapper methods #Patch Set 4 : Fix Mac buildc #
Messages
Total messages: 32 (19 generated)
Description was changed from ========== Move spellcheck underline colors into LayoutTheme I'm adding support in Chrome for the Android spellcheck menu in https://codereview.chromium.org/2931443003. When a user taps on a misspelled word, I need to highlight the word in a color based on the color of the spellcheck underline (we add some transparency). We're trying to avoid hard-coding the spellcheck color in editing code. Therefore, we think it makes sense to move the colors for spelling/grammar markers out of InlineTextBoxPainter into LayoutTheme, so other code can make use of them. BUG=715365 ========== to ========== Move spellcheck underline colors into LayoutTheme I'm adding support in Chrome for the Android spellcheck menu in https://codereview.chromium.org/2931443003. When a user taps on a misspelled word, I need to highlight the word in a color based on the color of the spellcheck underline (we add some transparency). We're trying to avoid hard-coding the spellcheck color in editing code. Therefore, we think it makes sense to move the colors for spelling/grammar markers out of InlineTextBoxPainter into LayoutTheme, so other code can make use of them. BUG=715365 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rlanday@chromium.org changed reviewers: + chrishtr@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2932913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTheme.h (right): https://codereview.chromium.org/2932913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTheme.h:133: // Underline colors for spelling and grammar markers. I think this comment is redundant, you can just remove it. https://codereview.chromium.org/2932913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTheme.h:135: Color GrammarMarkerUnderlineColor() const; What's the point of having these extra methods?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2932913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTheme.h (right): https://codereview.chromium.org/2932913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTheme.h:135: Color GrammarMarkerUnderlineColor() const; On 2017/06/10 at 00:45:56, chrishtr wrote: > What's the point of having these extra methods? I'm not sure, I was just copying how the existing methods are set up...
https://codereview.chromium.org/2932913002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTheme.h (right): https://codereview.chromium.org/2932913002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTheme.h:135: Color GrammarMarkerUnderlineColor() const; On 2017/06/10 at 00:45:56, chrishtr wrote: > What's the point of having these extra methods? I'm not sure, I was just copying how the existing methods are set up...
(If I upgrade to the new code review tool, will it stop double-publishing my comments?)
On 2017/06/10 at 16:34:03, rlanday wrote: > https://codereview.chromium.org/2932913002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutTheme.h (right): > > https://codereview.chromium.org/2932913002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutTheme.h:135: Color GrammarMarkerUnderlineColor() const; > On 2017/06/10 at 00:45:56, chrishtr wrote: > > What's the point of having these extra methods? > > I'm not sure, I was just copying how the existing methods are set up... Ok, please remove the redundant one then.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/12 at 15:07:10, chrishtr wrote: > On 2017/06/10 at 16:34:03, rlanday wrote: > > https://codereview.chromium.org/2932913002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/layout/LayoutTheme.h (right): > > > > https://codereview.chromium.org/2932913002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/layout/LayoutTheme.h:135: Color GrammarMarkerUnderlineColor() const; > > On 2017/06/10 at 00:45:56, chrishtr wrote: > > > What's the point of having these extra methods? > > > > I'm not sure, I was just copying how the existing methods are set up... > > Ok, please remove the redundant one then. Ok, updated
lgtm
I bet I know why the other methods use those wrappers: some people think you should never expose virtual methods as part of a public API. Unclear what the benefit is in a case like this though.
The CQ bit was unchecked by rlanday@chromium.org
The CQ bit was checked by rlanday@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by rlanday@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/2932913002/#ps60001 (title: "Fix Mac buildc")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497293106658240,
"parent_rev": "d0420a092646209e306e0f97441cd2e81c2eb62f", "commit_rev":
"61856d2266ce3b76ac8485a9ad2091d18842472a"}
Message was sent while issue was closed.
Description was changed from ========== Move spellcheck underline colors into LayoutTheme I'm adding support in Chrome for the Android spellcheck menu in https://codereview.chromium.org/2931443003. When a user taps on a misspelled word, I need to highlight the word in a color based on the color of the spellcheck underline (we add some transparency). We're trying to avoid hard-coding the spellcheck color in editing code. Therefore, we think it makes sense to move the colors for spelling/grammar markers out of InlineTextBoxPainter into LayoutTheme, so other code can make use of them. BUG=715365 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move spellcheck underline colors into LayoutTheme I'm adding support in Chrome for the Android spellcheck menu in https://codereview.chromium.org/2931443003. When a user taps on a misspelled word, I need to highlight the word in a color based on the color of the spellcheck underline (we add some transparency). We're trying to avoid hard-coding the spellcheck color in editing code. Therefore, we think it makes sense to move the colors for spelling/grammar markers out of InlineTextBoxPainter into LayoutTheme, so other code can make use of them. BUG=715365 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2932913002 Cr-Commit-Position: refs/heads/master@{#478755} Committed: https://chromium.googlesource.com/chromium/src/+/61856d2266ce3b76ac8485a9ad20... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/61856d2266ce3b76ac8485a9ad20... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
