Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1857353002:
Draw background opaquely on MD download items. (Closed)
|
Created:
4 years, 8 months ago by Evan Stade Modified:
4 years, 8 months ago Reviewers:
sky CC:
chromium-reviews, asanka, tfarina, dbeam+watch-downloads_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDraw background opaquely on MD download items.
Fixes the text rendering bug, as well as a separate rendering bug
evident on custom themes with a non-opaque COLOR_TOOLBAR (similar to
bug 596370)
BUG=600637
Committed: https://crrev.com/409085a57835af711f9e917b14e19dc7cf15ef37
Cr-Commit-Position: refs/heads/master@{#385877}
Patch Set 1 #
Total comments: 2
Patch Set 2 : more commentary #Messages
Total messages: 18 (6 generated)
estade@chromium.org changed reviewers: + sky@chromium.org
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/1857353002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1857353002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1857353002/diff/1/chrome/browser/ui/views/dow... File chrome/browser/ui/views/download/download_item_view_md.cc (right): https://codereview.chromium.org/1857353002/diff/1/chrome/browser/ui/views/dow... chrome/browser/ui/views/download/download_item_view_md.cc:594: canvas->DrawColor(SK_ColorBLACK); Why is black the right color here? Sorry for a similar question as the last, the choice of color seems arbitrary and I'm trying to understand why we go with black here vs white in the bookmarkbar vs anything? Actually, more generally why is it important to paint any color? If the theme wants a transparent color shouldn't that be ok? By which I mean ultimately some ancestor of the downloaditem should paint a color, right?
https://codereview.chromium.org/1857353002/diff/1/chrome/browser/ui/views/dow... File chrome/browser/ui/views/download/download_item_view_md.cc (right): https://codereview.chromium.org/1857353002/diff/1/chrome/browser/ui/views/dow... chrome/browser/ui/views/download/download_item_view_md.cc:594: canvas->DrawColor(SK_ColorBLACK); On 2016/04/05 20:26:55, sky wrote: > Why is black the right color here? Sorry for a similar question as the last, the > choice of color seems arbitrary and I'm trying to understand why we go with > black here vs white in the bookmarkbar vs anything? Black seems more right to me than white because an opaque canvas starts off as black as I understand it. The layer that we add for the ink drop hosting gives us a non-opaque canvas, so we have to paint opaquely in order to draw text on top of it correctly. > > Actually, more generally why is it important to paint any color? If the theme > wants a transparent color shouldn't that be ok? By which I mean ultimately some > ancestor of the downloaditem should paint a color, right? From the theme author's perspective, what do you think a transparent color should mean? What is it painted on top of? Honestly it would make more sense to me not to support transparency at all, but apparently we've handled it gracefully in the past. I suspect this changed because the canvas we're getting is no longer created as opaque: https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/canvas.cc&l=37 https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...
On 2016/04/05 20:46:54, Evan Stade wrote: > https://codereview.chromium.org/1857353002/diff/1/chrome/browser/ui/views/dow... > File chrome/browser/ui/views/download/download_item_view_md.cc (right): > > https://codereview.chromium.org/1857353002/diff/1/chrome/browser/ui/views/dow... > chrome/browser/ui/views/download/download_item_view_md.cc:594: > canvas->DrawColor(SK_ColorBLACK); > On 2016/04/05 20:26:55, sky wrote: > > Why is black the right color here? Sorry for a similar question as the last, > the > > choice of color seems arbitrary and I'm trying to understand why we go with > > black here vs white in the bookmarkbar vs anything? > > Black seems more right to me than white because an opaque canvas starts off as > black as I understand it. The layer that we add for the ink drop hosting gives > us a non-opaque canvas, so we have to paint opaquely in order to draw text on > top of it correctly. > > > > > Actually, more generally why is it important to paint any color? If the theme > > wants a transparent color shouldn't that be ok? By which I mean ultimately > some > > ancestor of the downloaditem should paint a color, right? > > From the theme author's perspective, what do you think a transparent color > should mean? What is it painted on top of? Honestly it would make more sense to > me not to support transparency at all, but apparently we've handled it > gracefully in the past. I suspect this changed because the canvas we're getting > is no longer created as opaque: > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/canvas.cc&l=37 > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i... Does this patch make sense in light of the conversation the other bug?
Yes, LGTM
Still LGTM, but could you document where the black comes from.
On 2016/04/07 18:24:46, sky wrote: > Still LGTM, but could you document where the black comes from. Done --- I elaborated on the comment, hopefully it's an improvement.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/1857353002/#ps20001 (title: "more commentary")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1857353002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1857353002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Draw background opaquely on MD download items. Fixes the text rendering bug, as well as a separate rendering bug evident on custom themes with a non-opaque COLOR_TOOLBAR (similar to bug 596370) BUG=600637 ========== to ========== Draw background opaquely on MD download items. Fixes the text rendering bug, as well as a separate rendering bug evident on custom themes with a non-opaque COLOR_TOOLBAR (similar to bug 596370) BUG=600637 Committed: https://crrev.com/409085a57835af711f9e917b14e19dc7cf15ef37 Cr-Commit-Position: refs/heads/master@{#385877} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/409085a57835af711f9e917b14e19dc7cf15ef37 Cr-Commit-Position: refs/heads/master@{#385877} |
