Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1815743002:
[MD] Fix bookmark bar painting bugs. (Closed)
|
Created:
4 years, 9 months ago by Evan Stade Modified:
4 years, 9 months ago CC:
chromium-reviews, tfarina, noyau+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD] Fix bookmark bar painting bugs.
This fixes the background color bug.
This may or may not fix the text rendering bug (but the text rendering bug
is not that important to fix promptly).
BUG=595978, 596003
Committed: https://crrev.com/30b95559fb5be38c99cffe3dc365a43594bc73a8
Cr-Commit-Position: refs/heads/master@{#382079}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 17 (6 generated)
estade@chromium.org changed reviewers: + pkasting@chromium.org, sky@chromium.org
https://codereview.chromium.org/1815743002/diff/1/chrome/browser/ui/views/boo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (left): https://codereview.chromium.org/1815743002/diff/1/chrome/browser/ui/views/boo... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:622: layer()->SetFillsBoundsOpaquely(false); this is the part that I think may fix the text rendering bug (which is only present on windows and looks like subpixel anti-aliasing on a transparent bg). When I added this line I think I copy-pasted without sufficiently considering whether it made sense. The bookmarks bar does fill its bounds opaquely, so it's more correct not to do this.
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/patch-status/1815743002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1815743002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bruthig@chromium.org changed reviewers: + bruthig@chromium.org
https://codereview.chromium.org/1815743002/diff/1/chrome/browser/ui/views/boo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (left): https://codereview.chromium.org/1815743002/diff/1/chrome/browser/ui/views/boo... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:622: layer()->SetFillsBoundsOpaquely(false); On 2016/03/18 19:02:04, Evan Stade wrote: > this is the part that I think may fix the text rendering bug (which is only > present on windows and looks like subpixel anti-aliasing on a transparent bg). > When I added this line I think I copy-pasted without sufficiently considering > whether it made sense. The bookmarks bar does fill its bounds opaquely, so it's > more correct not to do this. I'm skeptical here. If the BookmarkButtonView is painting opaquely then how will the underlying theme images be visible? It would be worth while making sure some utility function isn't updating the subpixel rendering property on those labels. For example I noticed a drag and drop util function (that captured the image for drag and drop) was updating changing the subpixel rendering property without resetting it before it finished. I will continue looking for this example.
On 2016/03/18 19:40:32, bruthig wrote: > https://codereview.chromium.org/1815743002/diff/1/chrome/browser/ui/views/boo... > File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (left): > > https://codereview.chromium.org/1815743002/diff/1/chrome/browser/ui/views/boo... > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:622: > layer()->SetFillsBoundsOpaquely(false); > On 2016/03/18 19:02:04, Evan Stade wrote: > > this is the part that I think may fix the text rendering bug (which is only > > present on windows and looks like subpixel anti-aliasing on a transparent bg). > > When I added this line I think I copy-pasted without sufficiently considering > > whether it made sense. The bookmarks bar does fill its bounds opaquely, so > it's > > more correct not to do this. > > I'm skeptical here. If the BookmarkButtonView is painting opaquely then how > will the underlying theme images be visible? BookmarkBarViewBackground
On 2016/03/18 19:45:24, Evan Stade wrote: > On 2016/03/18 19:40:32, bruthig wrote: > > > https://codereview.chromium.org/1815743002/diff/1/chrome/browser/ui/views/boo... > > File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (left): > > > > > https://codereview.chromium.org/1815743002/diff/1/chrome/browser/ui/views/boo... > > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:622: > > layer()->SetFillsBoundsOpaquely(false); > > On 2016/03/18 19:02:04, Evan Stade wrote: > > > this is the part that I think may fix the text rendering bug (which is only > > > present on windows and looks like subpixel anti-aliasing on a transparent > bg). > > > When I added this line I think I copy-pasted without sufficiently > considering > > > whether it made sense. The bookmarks bar does fill its bounds opaquely, so > > it's > > > more correct not to do this. > > > > I'm skeptical here. If the BookmarkButtonView is painting opaquely then how > > will the underlying theme images be visible? > > BookmarkBarViewBackground Neat, lgtm
LGTM
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1815743002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1815743002/1
The text rendering fix sounds plausible to me, as those text artifacts are when you get when rendering text to a transparent background.
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [MD] Fix bookmark bar painting bugs. This fixes the background color bug. This may or may not fix the text rendering bug (but the text rendering bug is not that important to fix promptly). BUG=595978,596003 ========== to ========== [MD] Fix bookmark bar painting bugs. This fixes the background color bug. This may or may not fix the text rendering bug (but the text rendering bug is not that important to fix promptly). BUG=595978,596003 Committed: https://crrev.com/30b95559fb5be38c99cffe3dc365a43594bc73a8 Cr-Commit-Position: refs/heads/master@{#382079} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/30b95559fb5be38c99cffe3dc365a43594bc73a8 Cr-Commit-Position: refs/heads/master@{#382079} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
