Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1393193002:
Paint tab-loading throbbers into a ui::Layer. (Closed)
DescriptionPaint tab-loading throbbers into a ui::Layer.
In views browsers, updating the tab loading throbber currently requires
a repaint that starts at the BrowserView and trickles the damage
rectangle down until it reaches the favicon area. This uses a lot of CPU
and energy: about 11 watts above idle on a MacBook Pro (and significant
CPU on Linux) for a single "waiting" spinner.
Giving the throbber a layer allows the repaint to be isolated, with
compositing done on the GPU. This brings total energy usage down to
about 2.5 watts above idle.
Plots at http://crbug.com/391646#c44
BUG=391646
TEST=Spinners should look and behave the same, but use less energy/CPU.
Committed: https://crrev.com/23f218dbb7d9f4fd2401e71bfa5f23af32d13800
Cr-Commit-Position: refs/heads/master@{#355514}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Handle stacking/dragging, respond to comments #Patch Set 3 : Neater logic, nits, fix bug in Tab::SetData #Patch Set 4 : Fix tooltips #Patch Set 5 : cl format #
Total comments: 15
Patch Set 6 : Also need to reset now that we hide (tab_strip.cc delta from rebase) #Patch Set 7 : pkasting comments #Patch Set 8 : de-inline #
Total comments: 4
Patch Set 9 : overrides -> private #
Total comments: 2
Patch Set 10 : rebase for crash animation update #Patch Set 11 : Fix clipping on the right, and animations #
Total comments: 1
Patch Set 12 : V1: Switch to unlayered on-the-fly #Patch Set 13 : simpler: TabController::CanPaintThrobberToLayer. Also: a test. #
Messages
Total messages: 39 (10 generated)
tapted@chromium.org changed reviewers: + sky@chromium.org
Hi sky - WDYT?
danakj@chromium.org changed reviewers: + danakj@chromium.org
Thanks for doing this! LGTM w/ a couple comments. https://codereview.chromium.org/1393193002/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/1393193002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:144: SetPaintToLayer(true); Can you leave a comment that this paints to separate layer because it animates its contents? https://codereview.chromium.org/1393193002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:183: Tab* owner_; // Weak. Owns this. "Owns this" can be a bit ambiguous. "Owns |this|"? https://codereview.chromium.org/1393193002/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab.h (right): https://codereview.chromium.org/1393193002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.h:358: views::View* throbber_; can this be a scoped_ptr?
I supsect you'll get bleed through of the throbber with stacked tabs, as well as tab dragging while the throbber is going. I wouldn't be surprised if there are problems with non-stacked tabs when the tabs are small enough, but that may depend upon the overlap. https://codereview.chromium.org/1393193002/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/1393193002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:159: ui::ThemeProvider* tp = owner_->GetThemeProvider(); Do you really need owner_ here? https://codereview.chromium.org/1393193002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:1451: delete throbber_; I would hide the throbber when you don't need it rather than destroying. Less view shuffling. https://codereview.chromium.org/1393193002/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab.h (right): https://codereview.chromium.org/1393193002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.h:358: views::View* throbber_; On 2015/10/08 14:09:01, danakj_OOO_til_10-12 wrote: > can this be a scoped_ptr? Generally we don't do that as views are implicitly owned by their parent. See the next three members as examples of what I mean.
I'll also admit I'm surprised that we need a layer here. Does this mean anything we animate needs it's own layer?
On Thu, Oct 8, 2015 at 11:53 AM, <sky@chromium.org> wrote: > I'll also admit I'm surprised that we need a layer here. Does this mean > anything > we animate needs it's own layer? > Yah, it's cheaper to animate content in a small layer than a small part of a larger layer. Equivalently, blink has exposed "will-change: contents" to the web platform to specify that it will be invalidating a lot, and it promotes those to layers. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
It's pretty common in the ui to animate smaller things. In addition to the throbber other things I'm aware of: throbbing buttons in some bubbles, background tabs, throbbing buttons on the bookmark bar... Having to switch all these to layers on the chance they may animate seems prohibitively expensive. Hm... -Scott On Thu, Oct 8, 2015 at 8:55 AM, Dana Jansens <danakj@chromium.org> wrote: > On Thu, Oct 8, 2015 at 11:53 AM, <sky@chromium.org> wrote: >> >> I'll also admit I'm surprised that we need a layer here. Does this mean >> anything >> we animate needs it's own layer? > > > Yah, it's cheaper to animate content in a small layer than a small part of a > larger layer. > > Equivalently, blink has exposed "will-change: contents" to the web platform > to specify that it will be invalidating a lot, and it promotes those to > layers. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Oct 8, 2015 at 11:59 AM, Scott Violet <sky@chromium.org> wrote: > It's pretty common in the ui to animate smaller things. In addition to > the throbber other things I'm aware of: throbbing buttons in some > bubbles, background tabs, throbbing buttons on the bookmark bar... > Having to switch all these to layers on the chance they may animate > seems prohibitively expensive. Hm... > I should say, rare/short animations probably aren't worth it. The win there is very short lived and you trade memory for it. But animations that run for a long time, I think so. Throbber is the poster child for this tradeoff being worth it. The tab blinking is another that runs continuously, though it's much more rare. I wouldn't promote things just for a short animation on mouse over for example, I don't think. > > -Scott > > On Thu, Oct 8, 2015 at 8:55 AM, Dana Jansens <danakj@chromium.org> wrote: > > On Thu, Oct 8, 2015 at 11:53 AM, <sky@chromium.org> wrote: > >> > >> I'll also admit I'm surprised that we need a layer here. Does this mean > >> anything > >> we animate needs it's own layer? > > > > > > Yah, it's cheaper to animate content in a small layer than a small part > of a > > larger layer. > > > > Equivalently, blink has exposed "will-change: contents" to the web > platform > > to specify that it will be invalidating a lot, and it promotes those to > > layers. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by tapted@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/1393193002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393193002/80001
There was some subtle stuff... (maybe needs tests, but it will have to wait until I find some spare time while I'm travelling to MTV next week) On 2015/10/08 15:52:23, sky wrote: > I supsect you'll get bleed through of the throbber with stacked tabs, as well as > tab dragging while the throbber is going. Ah, yup. I think I found a neat fix for this. Tested both stacked / non-stacked cases. The non-stacked case needed a small change to TabStrip::ShouldPaintTab(). > I wouldn't be surprised if there are > problems with non-stacked tabs when the tabs are small enough, but that may > depend upon the overlap. I couldn't make anything bad happen here - by the time they're small enough, the favicon bounds is collapsed and replaced by text (and the layer will disappear as soon as it inherits the favicon bounds). https://codereview.chromium.org/1393193002/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/1393193002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:144: SetPaintToLayer(true); On 2015/10/08 14:09:01, danakj_OOO_til_10-12 wrote: > Can you leave a comment that this paints to separate layer because it animates > its contents? Done. https://codereview.chromium.org/1393193002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:159: ui::ThemeProvider* tp = owner_->GetThemeProvider(); On 2015/10/08 15:52:23, sky wrote: > Do you really need owner_ here? Whoops - removed. https://codereview.chromium.org/1393193002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:183: Tab* owner_; // Weak. Owns this. On 2015/10/08 14:09:01, danakj_OOO_til_10-12 wrote: > "Owns this" can be a bit ambiguous. "Owns |this|"? Done. https://codereview.chromium.org/1393193002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:1451: delete throbber_; On 2015/10/08 15:52:23, sky wrote: > I would hide the throbber when you don't need it rather than destroying. Less > view shuffling. Done. https://codereview.chromium.org/1393193002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/1393193002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:560: UpdateLoadingAnimation(data.network_state); I noticed the throbber would hang around with some navigations (maybe those involving pre-rendering). Without this line it's possible to "lose" a transition from LOADING -> NONE and have both favicon and throbber get drawn.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
Drive-by since I'm in the middle of touching some of this code Should we be updating ui/views/controls/throbber.* as well? It looks like that also uses some of this machinery. (Maybe that should be converted into the One True Throbber and the tab code made to use it?) https://codereview.chromium.org/1393193002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/1393193002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:140: // Layer-backed view for updating a waiting or loading tab spinner. Tiny nit: Please be consistent about using the word "throbber" (in the CL title too), as mixing different terms is confusing. https://codereview.chromium.org/1393193002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:143: explicit ThrobberView(Tab* owner) : owner_(owner) { Don't define any of these class methods inline; define the methods separately below the class definition. https://codereview.chromium.org/1393193002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:144: // Since the throbber animates, paint to a separate layer do reduce repaint Nit: do -> to ? Maybe you want "can animate for a long time" instead of "animates" as well. https://codereview.chromium.org/1393193002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:145: // overheads. Nit: overheads -> overhead https://codereview.chromium.org/1393193002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:158: const gfx::Rect bounds = GetLocalBounds(); Nit: Might as well define this just below |tp|, there's no real reason to separate it up here. https://codereview.chromium.org/1393193002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:160: // Paint network activity (aka throbber) animation frame. Nit: It's weird to say "aka throbber" in a class whose name is basically "throbber". I wonder if this comment should just go away entirely. It doesn't seem to really add to the code. https://codereview.chromium.org/1393193002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1461: throbber_->SetVisible(needs_throbber); Nit: For clarity, you might want to reverse these two lines, so readers don't wonder if the ScheduleIconPaint() call might synchronously paint before we've changed the throbber visibility.
On 2015/10/09 08:03:24, Peter Kasting wrote: > Should we be updating ui/views/controls/throbber.* as well? It looks like that > also uses some of this machinery. (Maybe that should be converted into the One > True Throbber and the tab code made to use it?) There's not much overlap in the drawing logic for the two-state tab throbber vs the other ones (the timer situation for tabs is also a bit complex..). But I like the idea of combining the throbbers in some way (admittedly, the tab's throbber was even a bit hard to locate since it wasn't separate from `class Tab` and I kept running into the other ones). Interestingly, there are two subclasses of views::Throbber. One has the ash::ThrobberView (which owns a child that subclasses views::Throbber). It already has a ` SetPaintToLayer(true)` call, but the layer is in the superview of the views::Throbber. (I think the superview just centers the throbber in itself, so that's maybe the wrong place). But perhaps there are cases where a particular view hierarchy can make a better decision about how to optimize the placement of layers, so views::Throbber shouldn't do it automatically? (#conjecture). The other is in ash::tray::NetworkStateListDetailedView when doing a wifi scan. I don't see any layers nearby, so the throbber there might benefit from one. https://codereview.chromium.org/1393193002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/1393193002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:140: // Layer-backed view for updating a waiting or loading tab spinner. On 2015/10/09 08:03:23, Peter Kasting wrote: > Tiny nit: Please be consistent about using the word "throbber" (in the CL title > too), as mixing different terms is confusing. Done. https://codereview.chromium.org/1393193002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:143: explicit ThrobberView(Tab* owner) : owner_(owner) { On 2015/10/09 08:03:24, Peter Kasting wrote: > Don't define any of these class methods inline; define the methods separately > below the class definition. Done. https://codereview.chromium.org/1393193002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:144: // Since the throbber animates, paint to a separate layer do reduce repaint On 2015/10/09 08:03:24, Peter Kasting wrote: > Nit: do -> to ? > > Maybe you want "can animate for a long time" instead of "animates" as well. Done. https://codereview.chromium.org/1393193002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:145: // overheads. On 2015/10/09 08:03:23, Peter Kasting wrote: > Nit: overheads -> overhead Done. https://codereview.chromium.org/1393193002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:158: const gfx::Rect bounds = GetLocalBounds(); On 2015/10/09 08:03:23, Peter Kasting wrote: > Nit: Might as well define this just below |tp|, there's no real reason to > separate it up here. Done. https://codereview.chromium.org/1393193002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:160: // Paint network activity (aka throbber) animation frame. On 2015/10/09 08:03:24, Peter Kasting wrote: > Nit: It's weird to say "aka throbber" in a class whose name is basically > "throbber". > > I wonder if this comment should just go away entirely. It doesn't seem to > really add to the code. Done. https://codereview.chromium.org/1393193002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1461: throbber_->SetVisible(needs_throbber); On 2015/10/09 08:03:23, Peter Kasting wrote: > Nit: For clarity, you might want to reverse these two lines, so readers don't > wonder if the ScheduleIconPaint() call might synchronously paint before we've > changed the throbber visibility. Done.
LGTM, but do still get sky's signoff. I leave the decision about what changes to do to the other throbbers, and how to unify these if at all, in your hands. Make sure you glance at https://codereview.chromium.org/1394473004/ to be sure it won't cause you problems (I left some comments there as to what). https://codereview.chromium.org/1393193002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/1393193002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:395: // views::View: Nit: These overrides can probably be private instead of public. https://codereview.chromium.org/1393193002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1479: throbber_->ResetStartTimes(); Is this a functional change/fix? If so, you might want to separate it, and any other fixes you make, into a separate CL from the one that merely does the power savings.
Wow I thought I was up late.. Thanks Peter! On 2015/10/09 11:06:26, Peter Kasting wrote: > Make sure you glance at https://codereview.chromium.org/1394473004/ to be sure > it won't cause you problems (I left some comments there as to what). Hm - you're right - it does raise the question of whether a crash while loading could leave a spinner visible. I think the crash animation always happens via Tab::SetData and Looking at void BrowserTabStripController::SetTabRendererDataFromModel(), it would require contents->GetCrashedStatus() to be `crashed` with contents->IsLoading() still set. But that looks impossible, since WebContentsImpl::RenderViewTerminated() does SetIsLoading(false, true, nullptr); NotifyDisconnected(); SetIsCrashed(status, error_code); So, apart form a possible merge conflict in OnPaint, I think it's all fine: With the line I had to add, Tab::SetData() will always hide the throbber before doing tab crashy stuff. https://codereview.chromium.org/1393193002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/1393193002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:395: // views::View: On 2015/10/09 11:06:26, Peter Kasting wrote: > Nit: These overrides can probably be private instead of public. Done. https://codereview.chromium.org/1393193002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1479: throbber_->ResetStartTimes(); On 2015/10/09 11:06:26, Peter Kasting wrote: > Is this a functional change/fix? > > If so, you might want to separate it, and any other fixes you make, into a > separate CL from the one that merely does the power savings. The lines in this function were previously (in master) just above, in Tab::AdvanceLoadingAnimation(). They were removed in patchset 1 since the ThrobberView was re-constructed in AdvanceLoadingAnimation() whenever a throbber was needed, but now that ThrobberView is hidden instead of deleted (based on sky's comment) I needed them back.
On 2015/10/09 11:37:26, tapted wrote: > Wow I thought I was up late.. Thanks Peter! > > On 2015/10/09 11:06:26, Peter Kasting wrote: > > Make sure you glance at https://codereview.chromium.org/1394473004/ to be sure > > it won't cause you problems (I left some comments there as to what). > > Hm - you're right - it does raise the question of whether a crash while loading > could leave a spinner visible. I think the crash animation always happens via > Tab::SetData and Looking at void > BrowserTabStripController::SetTabRendererDataFromModel(), it would require > contents->GetCrashedStatus() to be `crashed` with contents->IsLoading() still > set. But that looks impossible, since > > WebContentsImpl::RenderViewTerminated() > > does > > SetIsLoading(false, true, nullptr); > NotifyDisconnected(); > SetIsCrashed(status, error_code); > > > So, apart form a possible merge conflict in OnPaint, I think it's all fine: With > the line I had to add, Tab::SetData() will always hide the throbber before doing > tab crashy stuff. Thanks for that investigation. Based on that I think I'll remove the conditional I added over in that CL.
https://codereview.chromium.org/1393193002/diff/150005/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/1393193002/diff/150005/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1472: clip.x() == 0 && clip.Contains(favicon_bounds_); I don't think this is enough. I can still easily get bleed through if a tab is loading and I drag another tab on top of it.
Patchset #11 (id:190001) has been deleted
https://codereview.chromium.org/1393193002/diff/150005/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/1393193002/diff/150005/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1472: clip.x() == 0 && clip.Contains(favicon_bounds_); On 2015/10/09 15:16:38, sky wrote: > I don't think this is enough. I can still easily get bleed through if a tab is > loading and I drag another tab on top of it. Thanks for trying it! I added handling for two additional cases. The first I didn't notice on a desktop-linux build, but on a ChromeOS build it showed in some tab reorder animations. The fix is in TabStrip::ShouldPaintTab(..) - there are two cases for when the tab being dragged overlaps the right side of a tab animating to the left: the clip rect wasn't updated for these, now it is. The other case was when a tab first attaches to a tab strip from another window. In this case, the right-side of a throbber can get the dragged tab over it (usually the tab reorders to the right before that can happen, but not when it happens in TabDragController::Attach). So, that check replaces the clip.Contains(..) here. I need to stare at the logic in TabStrip::ShouldPaintTab some more -- I think the clipping needs to be different for non-stacked tabs now that it updates the clip rect for both types. But let me know if you think this approach seems sound, and I'll fix that up and work on some tests. https://codereview.chromium.org/1393193002/diff/210001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/1393193002/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1448: const TabRendererData::NetworkState state = data().network_state; I briefly had a `ShouldShowThrobber() const` function, which required this change. ShouldShowThrobber() is gone, but I think it makes sense to keep this bit.
Now you've favored not showing the throbber in many places when we should. How about the simple way of only going with the layer when not using stacked tabs?
Patchset #13 (id:250001) has been deleted
PTAL. Sorry for the delay - wanted to ensure there was no jank introduced when switching the layer on/off (hard to do over CRD!). Had a play today, and I think it's good. I've reverted the TabStrip::ShouldPaintTab() changes and added TabController::CanPaintThrobberToLayer() instead, which is true only if there is no drag, animation, or stacked tabs. Also, a test \o/
What happens if a throbber starts, then a drag starts? On Mon, Oct 19, 2015 at 2:57 AM, <tapted@chromium.org> wrote: > PTAL. Sorry for the delay - wanted to ensure there was no jank introduced > when > switching the layer on/off (hard to do over CRD!). Had a play today, and I > think > it's good. > > I've reverted the TabStrip::ShouldPaintTab() changes and added > TabController::CanPaintThrobberToLayer() instead, which is true only if > there is > no drag, animation, or stacked tabs. Also, a test \o/ > > https://codereview.chromium.org/1393193002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/10/19 15:35:06, sky wrote: > What happens if a throbber starts, then a drag starts? So this was one of the things I wanted to check on a local aura build rather than trusting to RDP/CRD. There's even a unit test for it :), but I wanted to check for any jank as well. I think this case is OK, since as soon as the animation timer in BrowserView triggers, it will switch to non-layered. This transition is smooth/undetectable. I think theoretically, the browser might redraw after the drag starts but before the timer triggers. A call to TabStrip::UpdateLoadingAnimations() at the end of TabStrip::MaybeStartDrag() should avoid that, but I'm not sure it's needed (WDYT?)
LGTM Is there any noticable jank if lots of tabs are loading and you add a tab (which triggers animation and turning off layers)? This could happen on initial start with session restore.
On 2015/10/21 23:34:00, sky wrote: > LGTM > Is there any noticable jank if lots of tabs are loading and you add a tab (which > triggers animation and turning off layers)? This could happen on initial start > with session restore. I tried it out with ~20 tabs, with and without tab discarding. >~20 the favicon/throbber isn't painted so it's not an issue. I think tab discarding results in fewer throbbers in general. In that middle ground, without tab discarding, I think it's OK -- I can't tell any difference between the before/after this patch. I'll try out Canary vs Stable on some lower-end hardware to see if it's noticeable there and report back if I see anything bad.
Description was changed from ========== Paint tab-loading throbbers into a ui::Layer. In views browsers, updating the tab loading throbber currently requires a repaint that starts at the BrowserView and trickles the damage rectangle down until it reaches the favicon area. This uses a lot of CPU and energy: about 11 watts above idle on a MacBook Pro for a single "waiting" spinner. Giving the throbber a layer allows the repaint to be isolated, with compositing done on the GPU. This brings total energy usage down to about 2.5 watts above idle. Plots at http://crbug.com/391646#c44 BUG=391646 TEST=Spinners should look and behave the same, but use less energy/CPU. ========== to ========== Paint tab-loading throbbers into a ui::Layer. In views browsers, updating the tab loading throbber currently requires a repaint that starts at the BrowserView and trickles the damage rectangle down until it reaches the favicon area. This uses a lot of CPU and energy: about 11 watts above idle on a MacBook Pro (and significant CPU on Linux) for a single "waiting" spinner. Giving the throbber a layer allows the repaint to be isolated, with compositing done on the GPU. This brings total energy usage down to about 2.5 watts above idle. Plots at http://crbug.com/391646#c44 BUG=391646 TEST=Spinners should look and behave the same, but use less energy/CPU. ==========
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/1393193002/#ps270001 (title: "simpler: TabController::CanPaintThrobberToLayer. Also: a test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1393193002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393193002/270001
On 2015/10/22 08:01:36, tapted wrote: > On 2015/10/21 23:34:00, sky wrote: > > LGTM > > Is there any noticable jank if lots of tabs are loading and you add a tab > (which > > triggers animation and turning off layers)? This could happen on initial start > > with session restore. > > I tried it out with ~20 tabs, with and without tab discarding. >~20 the > favicon/throbber isn't painted so it's not an issue. I can fit ~80 tabs in my window before the favicons disappear. Might want to try with a maximized window on a widescreen monitor.
Message was sent while issue was closed.
Committed patchset #13 (id:270001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/23f218dbb7d9f4fd2401e71bfa5f23af32d13800 Cr-Commit-Position: refs/heads/master@{#355514}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:270001) has been created in https://codereview.chromium.org/1415873006/ by jonross@chromium.org. The reason for reverting is: This is suspected of causing test failures on the Chrome OS trunk build. See crbug.com/546600 Reverting to verify..
Message was sent while issue was closed.
On 2015/10/22 16:02:16, jonross wrote: > A revert of this CL (patchset #13 id:270001) has been created in > https://codereview.chromium.org/1415873006/ by mailto:jonross@chromium.org. > > The reason for reverting is: This is suspected of causing test failures on the > Chrome OS trunk build. See crbug.com/546600 > > Reverting to verify.. The flaky (and non-flaky asan-but-chromeos-only) failures should be resolved by a fix I've got up in https://codereview.chromium.org/1474993003/ (tl;dr wm::RecreateLayers() was leaving around some stale ui::Layer pointers). This CL should be fine after that lands, but it's been a while, so I've made https://codereview.chromium.org/1477713002/ to reland this CL - it's identical except for a trivial rebase. Further comments welcome there, otherwise I'll just send to OWNERS, once the fix has landed. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
