Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 6 months ago by sky Modified:
4 years, 6 months ago Reviewers:
Evan Stade CC:
chromium-reviews, tfarina, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChanges pinned tab throbbing indicator to a static image
The throbbing indicator consumes too much cpu on some devices and is
too subtle. See the spec attached to the bug for details on how this
looks in action.
BUG=473898
TEST=none
R=estade@chromium.org
Committed: https://crrev.com/725e93be4ee5c6dbaa7f0ae6ae9517027f4f297c
Cr-Commit-Position: refs/heads/master@{#401946}
Patch Set 1 #Patch Set 2 : properly handle null #
Total comments: 7
Patch Set 3 : feedback #
Total comments: 7
Patch Set 4 : feedback #
Created: 4 years, 6 months ago
Messages
Total messages: 14 (4 generated)
Description was changed from ========== Changes pinned tab throbbing indicator to a static image The throbbing indicator consumes too much cpu on some devices and is too subtle. See the spec attached to the bug for details on how this looks in action. BUG=473898 TEST=none R=estade@chromium.org ========== to ========== Changes pinned tab throbbing indicator to a static image The throbbing indicator consumes too much cpu on some devices and is too subtle. See the spec attached to the bug for details on how this looks in action. BUG=473898 TEST=none R=estade@chromium.org ==========
https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1455: const int circle_x = base::i18n::IsRTL() ? 0 : gfx::kFaviconSize; the spec makes me think the indicator/crop area should actually be centered at 15.5 rather than 16 https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1458: favicon = gfx::ImageSkia(icon_canvas.ExtractImageRep()); I think it would be a little simpler (slightly fewer LOC, fewer conditional blocks) to do if (showing_pinned[...]) { DrawFavicon ClearArea DrawIndicator } else { DrawFavicon } You don't get to share DrawFavicon but that's only one line anyway. https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1469: const SkColor kIndicatorColor = SkColorSetRGB(0x42, 0x85, 0xF4); I suspect you want NativeTheme::kColorId_CallToActionColor (which is this value on the normal theme but differs in incognito or in GTK theme contexts)
https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1455: const int circle_x = base::i18n::IsRTL() ? 0 : gfx::kFaviconSize; On 2016/06/24 17:09:39, Evan Stade wrote: > the spec makes me think the indicator/crop area should actually be centered at > 15.5 rather than 16 I sent Sebastien images of what this code looks like and he liked it. https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1458: favicon = gfx::ImageSkia(icon_canvas.ExtractImageRep()); On 2016/06/24 17:09:39, Evan Stade wrote: > I think it would be a little simpler (slightly fewer LOC, fewer conditional > blocks) to do > > if (showing_pinned[...]) { > DrawFavicon > ClearArea > DrawIndicator > } else { > DrawFavicon > } > > You don't get to share DrawFavicon but that's only one line anyway. Only one line, but a long one:) I moved the indicator drawing into a separate function. Better? https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1469: const SkColor kIndicatorColor = SkColorSetRGB(0x42, 0x85, 0xF4); On 2016/06/24 17:09:39, Evan Stade wrote: > I suspect you want NativeTheme::kColorId_CallToActionColor (which is this value > on the normal theme but differs in incognito or in GTK theme contexts) Done.
https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2091343002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1458: favicon = gfx::ImageSkia(icon_canvas.ExtractImageRep()); On 2016/06/24 17:41:15, sky wrote: > On 2016/06/24 17:09:39, Evan Stade wrote: > > I think it would be a little simpler (slightly fewer LOC, fewer conditional > > blocks) to do > > > > if (showing_pinned[...]) { > > DrawFavicon > > ClearArea > > DrawIndicator > > } else { > > DrawFavicon > > } > > > > You don't get to share DrawFavicon but that's only one line anyway. > > Only one line, but a long one:) I moved the indicator drawing into a separate > function. Better? yes, thanks https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1456: indicator_paint.setXfermodeMode(SkXfermode::kSrcOver_Mode); Fill and SrcOver are the defaults I believe. Did you specify them just to be more explicit? https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1457: const int indicator_x = GetMirroredXWithWidthInView( I'm unclear why this x/y calculation isn't the same as the clear one --- aren't they centered at the same point? https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1458: favicon_bounds_.right() - kIndicatorRadius, kIndicatorRadius * 2); confusing that favicon_bounds != favicon_bounds_
https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1456: indicator_paint.setXfermodeMode(SkXfermode::kSrcOver_Mode); On 2016/06/24 17:58:13, Evan Stade wrote: > Fill and SrcOver are the defaults I believe. Did you specify them just to be > more explicit? Mostly because knowing the defaults requires looking at the source, which is tedious. Updated. https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1457: const int indicator_x = GetMirroredXWithWidthInView( On 2016/06/24 17:58:13, Evan Stade wrote: > I'm unclear why this x/y calculation isn't the same as the clear one --- aren't > they centered at the same point? They are centered on the same absolute point, but the relatives point are different. This is because circle_x on 1439 is relative to the temporary canvas created on 1432 (icon_canvas). indicator_x/y is relative to the tab. https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1458: favicon_bounds_.right() - kIndicatorRadius, kIndicatorRadius * 2); On 2016/06/24 17:58:13, Evan Stade wrote: > confusing that favicon_bounds != favicon_bounds_ Ya, that is confusing. Renamed to favicon_draw_bounds.
https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1457: const int indicator_x = GetMirroredXWithWidthInView( On 2016/06/24 18:16:31, sky wrote: > On 2016/06/24 17:58:13, Evan Stade wrote: > > I'm unclear why this x/y calculation isn't the same as the clear one --- > aren't > > they centered at the same point? > > They are centered on the same absolute point, but the relatives point are > different. This is because circle_x on 1439 is relative to the temporary canvas > created on 1432 (icon_canvas). indicator_x/y is relative to the tab. ah, I missed that you're still using ExtractImageRep(). I guess clearing the canvas directly messes up the background? It seems like an alternative would be to temporarily add an inverted clip path when drawing the favicon; then you could do everything directly on canvas and share coordinate calculations. That might come out clearer, might not. Either way lgtm.
On 2016/06/24 18:32:31, Evan Stade wrote: > https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views... > File chrome/browser/ui/views/tabs/tab.cc (right): > > https://codereview.chromium.org/2091343002/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/tabs/tab.cc:1457: const int indicator_x = > GetMirroredXWithWidthInView( > On 2016/06/24 18:16:31, sky wrote: > > On 2016/06/24 17:58:13, Evan Stade wrote: > > > I'm unclear why this x/y calculation isn't the same as the clear one --- > > aren't > > > they centered at the same point? > > > > They are centered on the same absolute point, but the relatives point are > > different. This is because circle_x on 1439 is relative to the temporary > canvas > > created on 1432 (icon_canvas). indicator_x/y is relative to the tab. > > ah, I missed that you're still using ExtractImageRep(). I guess clearing the > canvas directly messes up the background? > > It seems like an alternative would be to temporarily add an inverted clip path > when drawing the favicon; then you could do everything directly on canvas and > share coordinate calculations. That might come out clearer, might not. Either > way lgtm. Ya, I considered installing a clip, but what I did here seems a lot more straightforward, so I went with it.
The CQ bit was checked by sky@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Changes pinned tab throbbing indicator to a static image The throbbing indicator consumes too much cpu on some devices and is too subtle. See the spec attached to the bug for details on how this looks in action. BUG=473898 TEST=none R=estade@chromium.org ========== to ========== Changes pinned tab throbbing indicator to a static image The throbbing indicator consumes too much cpu on some devices and is too subtle. See the spec attached to the bug for details on how this looks in action. BUG=473898 TEST=none R=estade@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Changes pinned tab throbbing indicator to a static image The throbbing indicator consumes too much cpu on some devices and is too subtle. See the spec attached to the bug for details on how this looks in action. BUG=473898 TEST=none R=estade@chromium.org ========== to ========== Changes pinned tab throbbing indicator to a static image The throbbing indicator consumes too much cpu on some devices and is too subtle. See the spec attached to the bug for details on how this looks in action. BUG=473898 TEST=none R=estade@chromium.org Committed: https://crrev.com/725e93be4ee5c6dbaa7f0ae6ae9517027f4f297c Cr-Commit-Position: refs/heads/master@{#401946} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/725e93be4ee5c6dbaa7f0ae6ae9517027f4f297c Cr-Commit-Position: refs/heads/master@{#401946} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
