Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2661313005:
Add extra DCHECKing to try to catch subpixel rendering on a non-opaque (Closed)
|
Created:
3 years, 10 months ago by Evan Stade Modified:
3 years, 10 months ago CC:
chromium-reviews, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd extra DCHECKing to try to catch subpixel rendering on a non-opaque
background.
This may not catch all such issues, but it would serve as an early
warning system for some cases. For example, if you remove the
subpixel-disabling from ImeListView, this DCHECK will trigger when you
click on it (which applies a ripple and adds a layer).
BUG=none
Review-Url: https://codereview.chromium.org/2661313005
Cr-Commit-Position: refs/heads/master@{#450074}
Committed: https://chromium.googlesource.com/chromium/src/+/853ae34d50f8322dbc272b7d92382a6a3ce3a0ab
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : checkpoint #
Total comments: 4
Patch Set 4 : just check one line #
Total comments: 6
Patch Set 5 : sadrul review #
Total comments: 4
Patch Set 6 : msw review #
Messages
Total messages: 47 (33 generated)
The CQ bit was checked by estade@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...
Description was changed from ========== Add extra DCHECKing to try to catch subpixel rendering on a non-opaque background. This may not catch all such issues, but it would serve as an early warning system for some cases. For example, if you remove the subpixel-disabling from ImeListView, this DCHECK will trigger when you click on it (thereby giving it a layer). BUG=none ========== to ========== Add extra DCHECKing to try to catch subpixel rendering on a non-opaque background. This may not catch all such issues, but it would serve as an early warning system for some cases. For example, if you remove the subpixel-disabling from ImeListView, this DCHECK will trigger when you click on it (which applies a ripple and adds a layer). BUG=none ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by estade@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by estade@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by estade@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
estade@chromium.org changed reviewers: + derat@chromium.org, sadrul@chromium.org
lgtm with a suggestion https://codereview.chromium.org/2661313005/diff/40001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/2661313005/diff/40001/ui/views/controls/label... ui/views/controls/label.cc:460: if (!lines_[i]->subpixel_rendering_suppressed()) { as a minor optimization, you can skip the stuff inside of this if block after the first non-suppressed line that you find, right? right now, it looks like you can check the hierarchy multiple times.
Some of the tests seem to be flakily crashing? Any idea what's up with that? https://codereview.chromium.org/2661313005/diff/40001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/2661313005/diff/40001/ui/views/controls/label... ui/views/controls/label.cc:463: break; The background could be non-opaque (e.g. solid-color background with alpha < 1), and live in a non-opaque layer too, right? So the existence of a background doesn't necessarily give you what you want.
those aren't flakes, those are actual bugs which I found with this patch and fixed in other CLs before re-sending this one to the try server. https://codereview.chromium.org/2661313005/diff/40001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/2661313005/diff/40001/ui/views/controls/label... ui/views/controls/label.cc:463: break; On 2017/02/07 03:36:39, sadrul wrote: > The background could be non-opaque (e.g. solid-color background with alpha < 1), > and live in a non-opaque layer too, right? So the existence of a background > doesn't necessarily give you what you want. Correct, that's the situation I was referring to in the CL description that wouldn't be caught, i.e. would be a false negative.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
https://codereview.chromium.org/2661313005/diff/40001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/2661313005/diff/40001/ui/views/controls/label... ui/views/controls/label.cc:460: if (!lines_[i]->subpixel_rendering_suppressed()) { On 2017/02/06 22:35:49, Daniel Erat wrote: > as a minor optimization, you can skip the stuff inside of this if block after > the first non-suppressed line that you find, right? right now, it looks like you > can check the hierarchy multiple times. yea, we can just do this for the first line since all the lines should have the same value for this variable.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2661313005/diff/60001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/2661313005/diff/60001/ui/views/controls/label... ui/views/controls/label.cc:458: if (!lines_.empty()) { early return insrtead https://codereview.chromium.org/2661313005/diff/60001/ui/views/controls/label... ui/views/controls/label.cc:462: if (!lines_[0]->subpixel_rendering_suppressed()) { ditto https://codereview.chromium.org/2661313005/diff/60001/ui/views/controls/label... ui/views/controls/label.cc:464: if (view->background()) Maybe you can continue with the loop if SkColorGetA(view->background()->get_color()) != OPAQUE or something.
estade@chromium.org changed reviewers: + msw@chromium.org
+msw for bookmark_bar_view_test.cc https://codereview.chromium.org/2661313005/diff/60001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/2661313005/diff/60001/ui/views/controls/label... ui/views/controls/label.cc:458: if (!lines_.empty()) { On 2017/02/08 02:13:56, sadrul wrote: > early return insrtead Done. https://codereview.chromium.org/2661313005/diff/60001/ui/views/controls/label... ui/views/controls/label.cc:462: if (!lines_[0]->subpixel_rendering_suppressed()) { On 2017/02/08 02:13:56, sadrul wrote: > ditto Done. https://codereview.chromium.org/2661313005/diff/60001/ui/views/controls/label... ui/views/controls/label.cc:464: if (view->background()) On 2017/02/08 02:13:56, sadrul wrote: > Maybe you can continue with the loop if > SkColorGetA(view->background()->get_color()) != OPAQUE or something. Done.
The CQ bit was checked by estade@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...
lgtm with nits https://codereview.chromium.org/2661313005/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc (right): https://codereview.chromium.org/2661313005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc:304: // Real bookmark bars get a BookmarkBarViewBackground. Set a non-null optional nit: s/a non-null/an opaque/? https://codereview.chromium.org/2661313005/diff/80001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/2661313005/diff/80001/ui/views/controls/label... ui/views/controls/label.cc:458: #if DCHECK_IS_ON() optional nit: it might be nice to have this as a separate anon-namespace function, instead of inlined in PaintText; especially if anyone tries to later-on add code below this bloc; and miss the debug-only early return.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2661313005/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc (right): https://codereview.chromium.org/2661313005/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc:304: // Real bookmark bars get a BookmarkBarViewBackground. Set a non-null On 2017/02/11 00:49:37, msw wrote: > optional nit: s/a non-null/an opaque/? Done. https://codereview.chromium.org/2661313005/diff/80001/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/2661313005/diff/80001/ui/views/controls/label... ui/views/controls/label.cc:458: #if DCHECK_IS_ON() On 2017/02/11 00:49:37, msw wrote: > optional nit: it might be nice to have this as a separate anon-namespace > function, instead of inlined in PaintText; especially if anyone tries to > later-on add code below this bloc; and miss the debug-only early return. I share your concern about the unnoticed early return, which is why I didn't have it originally, but moving to an anon namespace is annoying given the access to member variables.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, derat@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2661313005/#ps100001 (title: "msw review")
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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by estade@chromium.org
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": 100001, "attempt_start_ts": 1487014606883970,
"parent_rev": "acd180920519c834e6727161b9f98194e7119554", "commit_rev":
"853ae34d50f8322dbc272b7d92382a6a3ce3a0ab"}
Message was sent while issue was closed.
Description was changed from ========== Add extra DCHECKing to try to catch subpixel rendering on a non-opaque background. This may not catch all such issues, but it would serve as an early warning system for some cases. For example, if you remove the subpixel-disabling from ImeListView, this DCHECK will trigger when you click on it (which applies a ripple and adds a layer). BUG=none ========== to ========== Add extra DCHECKing to try to catch subpixel rendering on a non-opaque background. This may not catch all such issues, but it would serve as an early warning system for some cases. For example, if you remove the subpixel-disabling from ImeListView, this DCHECK will trigger when you click on it (which applies a ripple and adds a layer). BUG=none Review-Url: https://codereview.chromium.org/2661313005 Cr-Commit-Position: refs/heads/master@{#450074} Committed: https://chromium.googlesource.com/chromium/src/+/853ae34d50f8322dbc272b7d9238... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/853ae34d50f8322dbc272b7d9238... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
