Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1861783002:
Make sure to fill canvas opaquely when painting bookmark bar background. (Closed)
|
Created:
4 years, 8 months ago by Evan Stade Modified:
4 years, 8 months ago Reviewers:
sky CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake sure to fill canvas opaquely when painting bookmark bar background.
BUG=596370
Committed: https://crrev.com/046ca0c1fd8e435ee0b15a5a84f517f0a3ca2da8
Cr-Commit-Position: refs/heads/master@{#385892}
Patch Set 1 #
Total comments: 3
Patch Set 2 : color_frame #
Total comments: 2
Patch Set 3 : GetFrameColor() #Patch Set 4 : color control #
Total comments: 1
Patch Set 5 : elaborate on comment #Messages
Total messages: 19 (4 generated)
estade@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/1861783002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1861783002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:427: canvas->DrawColor(SK_ColorWHITE); Why is white the right color here?
https://codereview.chromium.org/1861783002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1861783002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:427: canvas->DrawColor(SK_ColorWHITE); On 2016/04/05 19:14:01, sky wrote: > Why is white the right color here? That is a good question. Initially I used black, but I found that white preserved pre-MD behavior. I didn't investigate thoroughly why this was (presumably before an opaque layer was added, we were painting white at some higher level in the view hierarchy).
This seems wrong to me and it would be good to figure out why it's necessary now. I have similar comments in your downloaditem change as well. On Tue, Apr 5, 2016 at 12:36 PM, <estade@chromium.org> wrote: > > https://codereview.chromium.org/1861783002/diff/1/chrome/browser/ui/views/fra... > File chrome/browser/ui/views/frame/browser_view.cc (right): > > https://codereview.chromium.org/1861783002/diff/1/chrome/browser/ui/views/fra... > chrome/browser/ui/views/frame/browser_view.cc:427: > canvas->DrawColor(SK_ColorWHITE); > On 2016/04/05 19:14:01, sky wrote: >> Why is white the right color here? > > That is a good question. Initially I used black, but I found that white > preserved pre-MD behavior. I didn't investigate thoroughly why this was > (presumably before an opaque layer was added, we were painting white at > some higher level in the view hierarchy). > > https://codereview.chromium.org/1861783002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1861783002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1861783002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/browser_view.cc:427: canvas->DrawColor(SK_ColorWHITE); On 2016/04/05 19:36:10, Evan Stade wrote: > On 2016/04/05 19:14:01, sky wrote: > > Why is white the right color here? > > That is a good question. Initially I used black, but I found that white > preserved pre-MD behavior. I didn't investigate thoroughly why this was > (presumably before an opaque layer was added, we were painting white at some > higher level in the view hierarchy). I looked into it and I think that previously the frame was providing the base color, so I've updated accordingly.
On 2016/04/05 20:31:52, Evan Stade wrote: > https://codereview.chromium.org/1861783002/diff/1/chrome/browser/ui/views/fra... > File chrome/browser/ui/views/frame/browser_view.cc (right): > > https://codereview.chromium.org/1861783002/diff/1/chrome/browser/ui/views/fra... > chrome/browser/ui/views/frame/browser_view.cc:427: > canvas->DrawColor(SK_ColorWHITE); > On 2016/04/05 19:36:10, Evan Stade wrote: > > On 2016/04/05 19:14:01, sky wrote: > > > Why is white the right color here? > > > > That is a good question. Initially I used black, but I found that white > > preserved pre-MD behavior. I didn't investigate thoroughly why this was > > (presumably before an opaque layer was added, we were painting white at some > > higher level in the view hierarchy). > > I looked into it and I think that previously the frame was providing the base > color, so I've updated accordingly. What changed that the frame is not providing it anymore?
On 2016/04/05 22:48:53, sky wrote: > On 2016/04/05 20:31:52, Evan Stade wrote: > > > https://codereview.chromium.org/1861783002/diff/1/chrome/browser/ui/views/fra... > > File chrome/browser/ui/views/frame/browser_view.cc (right): > > > > > https://codereview.chromium.org/1861783002/diff/1/chrome/browser/ui/views/fra... > > chrome/browser/ui/views/frame/browser_view.cc:427: > > canvas->DrawColor(SK_ColorWHITE); > > On 2016/04/05 19:36:10, Evan Stade wrote: > > > On 2016/04/05 19:14:01, sky wrote: > > > > Why is white the right color here? > > > > > > That is a good question. Initially I used black, but I found that white > > > preserved pre-MD behavior. I didn't investigate thoroughly why this was > > > (presumably before an opaque layer was added, we were painting white at some > > > higher level in the view hierarchy). > > > > I looked into it and I think that previously the frame was providing the base > > color, so I've updated accordingly. > > What changed that the frame is not providing it anymore? The bookmark bar is now an opaque layer (it has to be opaque so the text renders on top of it without subpixel issues).
Got it. By frame do you mean BrowserView? Seems like BookmarkBarViewBackground::Paint should call into the same code. If we had that in the first place you wouldn't need this change. -Scott On Tue, Apr 5, 2016 at 3:55 PM, <estade@chromium.org> wrote: > On 2016/04/05 22:48:53, sky wrote: >> On 2016/04/05 20:31:52, Evan Stade wrote: >> > >> > https://codereview.chromium.org/1861783002/diff/1/chrome/browser/ui/views/fra... >> > File chrome/browser/ui/views/frame/browser_view.cc (right): >> > >> > >> > https://codereview.chromium.org/1861783002/diff/1/chrome/browser/ui/views/fra... >> > chrome/browser/ui/views/frame/browser_view.cc:427: >> > canvas->DrawColor(SK_ColorWHITE); >> > On 2016/04/05 19:36:10, Evan Stade wrote: >> > > On 2016/04/05 19:14:01, sky wrote: >> > > > Why is white the right color here? >> > > >> > > That is a good question. Initially I used black, but I found that >> > > white >> > > preserved pre-MD behavior. I didn't investigate thoroughly why this >> > > was >> > > (presumably before an opaque layer was added, we were painting white >> > > at > some >> > > higher level in the view hierarchy). >> > >> > I looked into it and I think that previously the frame was providing the > base >> > color, so I've updated accordingly. >> >> What changed that the frame is not providing it anymore? > > The bookmark bar is now an opaque layer (it has to be opaque so the text > renders > on top of it without subpixel issues). > > https://codereview.chromium.org/1861783002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/04/05 23:00:26, sky wrote: > Got it. By frame do you mean BrowserView? Seems like > BookmarkBarViewBackground::Paint should call into the same code. If we > had that in the first place you wouldn't need this change. > > -Scott It boils down to this: https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/window/fr... I looked into doing something like what TopContainerView does but that solution seems a lot more complicated.
https://codereview.chromium.org/1861783002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1861783002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:429: view->GetThemeProvider()->GetColor(ThemeProperties::COLOR_FRAME)); Shouldn't this route through BrowserNonClientFrameView::GetFrameColor? In fact perhaps there should be a called on BrowserNonClientFrameView for this painting, eg BrowserNonClientFrameView::PaintBackground(...).
https://codereview.chromium.org/1861783002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1861783002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:429: view->GetThemeProvider()->GetColor(ThemeProperties::COLOR_FRAME)); On 2016/04/07 18:23:45, sky wrote: > Shouldn't this route through BrowserNonClientFrameView::GetFrameColor? In fact > perhaps there should be a called on BrowserNonClientFrameView for this painting, > eg BrowserNonClientFrameView::PaintBackground(...). This comment led me to realize I was wrong about the frame being the background, because it wouldn't make sense for the bar to change color based on the frame's activation state. So then I found this[1] and I was able to confirm that if I changed[2] to something weird like SK_ColorGREEN then I could see the green shining through on this theme with the old, non-layered version of the bookmark bar. Thus the new patch starts by painting COLOR_CONTROL_BACKGROUND. (Getting a hold of contents_container_ from here to call ->background()->Paint() on it is a bit of a hassle.) [1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... [2] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
LGTM https://codereview.chromium.org/1861783002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1861783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:230: canvas->DrawColor( nit: document why there are two drawcolor calls here. On a quick look the first DrawColor seems unnecessary.
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/1861783002/#ps80001 (title: "elaborate on comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1861783002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1861783002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Make sure to fill canvas opaquely when painting bookmark bar background. BUG=596370 ========== to ========== Make sure to fill canvas opaquely when painting bookmark bar background. BUG=596370 Committed: https://crrev.com/046ca0c1fd8e435ee0b15a5a84f517f0a3ca2da8 Cr-Commit-Position: refs/heads/master@{#385892} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/046ca0c1fd8e435ee0b15a5a84f517f0a3ca2da8 Cr-Commit-Position: refs/heads/master@{#385892} |
