Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2803583003:
ui: Remove use of bitmaps when painting tab backgrounds. (Closed)
|
Created:
3 years, 8 months ago by danakj Modified:
3 years, 8 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, miu+watch_chromium.org, enne (OOO), piman Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionui: Remove use of bitmaps when painting tab backgrounds.
This removes the use of gfx::Canvas::ExtractImageRep() which was being
used for inactive tab backgrounds, to generate two bitmaps and draw
them into the recording canvas when a clip on the second one. Instead,
we just record the commands all directly to the recording canvas, and
insert a save+clip+restore around the commands that were going into
the second bitmap previously.
Because we're no longer using bitmaps, this alleviates the need for
caching and we remove the global tab background-bitmap cache. Also we
collapse all cases in Tab::PaintInactiveTabBackground down to a single
code path with some variables to control the output.
The only change in behaviour due to collapsing code is that when
TabController::MaySetClip() is true we always apply the clip for
inactive tabs, whereas before the clip ould be ignored when a hover
effect or custom fill image was present. This is more correct as
described by pkasting in the CL review comments.
R=pkasting@chromium.org
BUG=671433
Review-Url: https://codereview.chromium.org/2803583003
Cr-Commit-Position: refs/heads/master@{#465266}
Committed: https://chromium.googlesource.com/chromium/src/+/87648f2cf83870acac8d73ea6bb61a5dcf11b921
Patch Set 1 #Patch Set 2 : tabbackground: . #
Total comments: 2
Patch Set 3 : tabbackground: return #Patch Set 4 : tabbackground: pointers #
Total comments: 20
Patch Set 5 : tabbackground: pkasting-nits #Patch Set 6 : tabbackground: cache-paths #
Total comments: 11
Patch Set 7 : tabbackground: with-cache #Patch Set 8 : tabbackground: with-cache #
Total comments: 23
Patch Set 9 : tabbackground: fix-cache-bug-write-to-correct-canvas #Patch Set 10 : tabbackground: review-feedback #Patch Set 11 : tabbackground: no-path-cache #Patch Set 12 : tabbackground: abovecomment #Patch Set 13 : tabbackground: whitespacenit #Patch Set 14 : tabbackground: rip-DCHECK_IMPLIES #
Total comments: 12
Patch Set 15 : tabbackground: rebase #Patch Set 16 : tabbackground: nits #Patch Set 17 : tabbackground: . #
Dependent Patchsets: Messages
Total messages: 89 (60 generated)
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2803583003/diff/60001/ui/gfx/scoped_canvas.h File ui/gfx/scoped_canvas.h (right): https://codereview.chromium.org/2803583003/diff/60001/ui/gfx/scoped_canvas.h#... ui/gfx/scoped_canvas.h:34: o.canvas_ = nullptr; Oops, was missing the return here when I changed these from =default at the last minute. I realized that would lead to 2 restore for 1 save. The code currently only uses the constructor (to emplace into optional).
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2803583003/diff/60001/ui/gfx/scoped_canvas.h File ui/gfx/scoped_canvas.h (right): https://codereview.chromium.org/2803583003/diff/60001/ui/gfx/scoped_canvas.h#... ui/gfx/scoped_canvas.h:34: o.canvas_ = nullptr; On 2017/04/05 22:01:37, danakj wrote: > Oops, was missing the return here when I changed these from =default at the last > minute. I realized that would lead to 2 restore for 1 save. The code currently > only uses the constructor (to emplace into optional). Alternatively i could keep passing a null instead of using optional, optional just seemed more obvious when reading it, lmk.
Patchset #2 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/04/05 22:07:01, danakj wrote: > https://codereview.chromium.org/2803583003/diff/60001/ui/gfx/scoped_canvas.h > File ui/gfx/scoped_canvas.h (right): > > https://codereview.chromium.org/2803583003/diff/60001/ui/gfx/scoped_canvas.h#... > ui/gfx/scoped_canvas.h:34: o.canvas_ = nullptr; > On 2017/04/05 22:01:37, danakj wrote: > > Oops, was missing the return here when I changed these from =default at the > last > > minute. I realized that would lead to 2 restore for 1 save. The code currently > > only uses the constructor (to emplace into optional). > > Alternatively i could keep passing a null instead of using optional, optional > just seemed more obvious when reading it, lmk. Have you done any performance analysis of this? Way back when the caching made a difference. It was done as almost all inactive tabs look the same.
Yeah, I share Scott's concerns. This is sort of like how the code worked long, long ago, and we moved to cached images because it was faster than what we were doing back then. How slow is blitting an already-drawn image versus constructing the complicated fill and stroke paths, clipping, and drawing/filling them? https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1040: gfx::Path* no_clip = nullptr; Nit: Please don't pull this kind of thing out. It's actually more cumbersome for me to read than the inline definition, where if I can't recall the meaning of the arg both my IDE and codesearch provide it for me very quickly, primarily because the majority of the time I'm scanning and don't need to know what any of the params mean. I'm not trying to ban doing this across Chromium... just don't do it here :) https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1086: may_clip ? &clip : nullptr); Nit: All the temps have made this function hard for me to understand, because I need to keep looking from the function call to multiple different places above to determine what the values are. How about: bool has_custom_image; int fill_id = controller_->GetBackgroundResourceId(&has_custom_image); if (has_custom_image) { // ... const bool fill_id_is_custom = GetThemeProvider()->HasCustomImage(fill_id); const int y_offset = fill_id_is_custom ? 0 : background_offset_.y(); PaintTabBackground(canvas, false, fill_id, y_offset, nullptr); } else { PaintTabBackground(canvas, false, 0, 0, controller_->MaySetClip() ? &clip : nullptr); } This makes it much more immediately obvious to me that one case paints a custom image at a possibly-custom offset, and the other doesn't, but might clip. https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1098: base::Optional<gfx::ScopedCanvas> scoped_canvas; Nit: It seems like it would be marginally simpler to just do: gfx::ScopedCanvas clip_scoper(nullptr); if (clip) { clip_scoper = gfx::ScopedCanvas(canvas); canvas->sk_canvas()->clipPath(*clip, SkClipOp::kDifference, true); } PaintTabBackgroundStroke(canvas, fill_path, active); Of course, just always initing the ScopedCanvas, as the old code did, would be even simpler. Can we ensure that Save()/Restore() have no perf cost if there's no change to the clip/transform/etc. (i.e. the save wasn't actually necessary)? That would mean callers could use this freely, without worrying about scoping only in the cases where it's necessary. https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.h (right): https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:239: bool active, Nit: For naming consistency, make this and the arg in PaintTabBackgroundStroke() |is_active|. https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:243: void PaintTabBackgroundFill(gfx::Canvas* canvas, Nit: Maybe separate these two from the function above, with a comment like "Helpers for PaintTabBackground() that paint the fill and stroke, respectively." https://codereview.chromium.org/2803583003/diff/100001/ui/gfx/scoped_canvas.h File ui/gfx/scoped_canvas.h (right): https://codereview.chromium.org/2803583003/diff/100001/ui/gfx/scoped_canvas.h... ui/gfx/scoped_canvas.h:26: ScopedCanvas(ScopedCanvas&& o) { Nit: Per Google style guide constructors and assignment operator all go above destructor
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
> Have you done any performance analysis of this? Way back when the caching made a > difference. It was done as almost all inactive tabs look the same. > Yeah, I share Scott's concerns. This is sort of like how the code worked long, > long ago, and we moved to cached images because it was faster than what we were > doing back then. How slow is blitting an already-drawn image versus > constructing the complicated fill and stroke paths, clipping, and > drawing/filling them? My observations was that most of the code paths do not cache, only the path that made bitmaps, which requires allocations etc, was using a cache. In general recording is cheap but caching is better, which is why we're doing it on a per-view basis. Inserting an SkPath means adding a refcount on an SkPathRef, which is cheap: https://cs.chromium.org/chromium/src/third_party/skia/include/core/SkPath.h?r... Constructing the path can be cached, which can be reused more often also as it stays the same for hover/active/colors. I've done that in the next patchset. I'll grab some CPU time numbers for you too. https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1040: gfx::Path* no_clip = nullptr; On 2017/04/06 00:52:11, Peter Kasting wrote: > Nit: Please don't pull this kind of thing out. It's actually more cumbersome > for me to read than the inline definition, where if I can't recall the meaning > of the arg both my IDE and codesearch provide it for me very quickly, primarily > because the majority of the time I'm scanning and don't need to know what any of > the params mean. > > I'm not trying to ban doing this across Chromium... just don't do it here :) If I didn't give it a name this way I'd want to write write it as "nullptr /* clip */", but the way you wrote example code below makes me think you don't like that either. This makes the code harder to read outside of your IDE though. :\ I can do this but I will have to go look at the function decl to figure out what false/0/0/nullptr mean here as will most devs, and this gets particularly less pleasant in places like git blame or looking at diffs which are not part of an IDE for you either notably. Does this apply to the active bool also? I am assuming it does. https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1086: may_clip ? &clip : nullptr); On 2017/04/06 00:52:11, Peter Kasting wrote: > Nit: All the temps have made this function hard for me to understand, because I > need to keep looking from the function call to multiple different places above > to determine what the values are. > > How about: > > bool has_custom_image; > int fill_id = controller_->GetBackgroundResourceId(&has_custom_image); > if (has_custom_image) { > // ... > const bool fill_id_is_custom = GetThemeProvider()->HasCustomImage(fill_id); > const int y_offset = fill_id_is_custom ? 0 : background_offset_.y(); > PaintTabBackground(canvas, false, fill_id, y_offset, nullptr); > } else { > PaintTabBackground(canvas, false, 0, 0, > controller_->MaySetClip() ? &clip : nullptr); > } > > This makes it much more immediately obvious to me that one case paints a custom > image at a possibly-custom offset, and the other doesn't, but might clip. I really liked not splitting this into multiple calls to PaintTabBackground, as that makes much harder to call it with the wrong things IMO, as there's no copypasta problems. For instance, was it intended that we only clip in the last case? It seems weird to me, and there's no comment explaining why like there is for other things. I think this code makes it more explicit by setting may_clip = false in the block, rather than 2 function calls where one may have just forgotten to use the clip over time. But done. https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1098: base::Optional<gfx::ScopedCanvas> scoped_canvas; On 2017/04/06 00:52:11, Peter Kasting wrote: > Nit: It seems like it would be marginally simpler to just do: > > gfx::ScopedCanvas clip_scoper(nullptr); > if (clip) { > clip_scoper = gfx::ScopedCanvas(canvas); > canvas->sk_canvas()->clipPath(*clip, SkClipOp::kDifference, true); > } > PaintTabBackgroundStroke(canvas, fill_path, active); > > Of course, just always initing the ScopedCanvas, as the old code did, would be > even simpler. Can we ensure that Save()/Restore() have no perf cost if there's > no change to the clip/transform/etc. (i.e. the save wasn't actually necessary)? > That would mean callers could use this freely, without worrying about scoping > only in the cases where it's necessary. Ya that's fair, if you can move, then we can init with nullptr since ScopedCanvas has a do-nothing mode. I was kinda building around assuming that making a ScopedCanvas would always do something but that's not the case. Doing this the old way has the disadvantage of duplicating the conditional in the call to the constructor and then below it, which is a potential for bugs if they diverged at some point unintentionally, and is more branches for a reader to look at. I do think that this is less obvious to a reader than optional though: gfx::ScopedCanvas scoped_canvas(nullptr); if (clip) { scoped_canvas = gfx::ScopedCanvas(canvas); As now you have to know what each constructor of ScopedCanvas would do. Whereas with optional there's no ScopedCanvas at all so nothing to really know for the first case. Skia's raster does have peephole optimizations for removing ops that do nothing, but I don't think there's anything to handle as complex an interaction as you propose here. While it's possible we could add something like that into our future raster implementation, it's always cheaper to avoid doing work than to undo it later. I left this the way it was before I got here, and removed the changes to ScopedCanvas. In the future I think we can make PaintCanvas::Save() return a scoped object, and then code like this could be more clear without having to call constructors explicitly and more difficult to do it wrong. https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.h (right): https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:239: bool active, On 2017/04/06 00:52:11, Peter Kasting wrote: > Nit: For naming consistency, make this and the arg in PaintTabBackgroundStroke() > |is_active|. I changed it because is_active is incorrect. This is "paint as if active". There is one case where IsActive() is false, but we pass true to paint as active. https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:243: void PaintTabBackgroundFill(gfx::Canvas* canvas, On 2017/04/06 00:52:11, Peter Kasting wrote: > Nit: Maybe separate these two from the function above, with a comment like > "Helpers for PaintTabBackground() that paint the fill and stroke, respectively." Done. https://codereview.chromium.org/2803583003/diff/100001/ui/gfx/scoped_canvas.h File ui/gfx/scoped_canvas.h (right): https://codereview.chromium.org/2803583003/diff/100001/ui/gfx/scoped_canvas.h... ui/gfx/scoped_canvas.h:26: ScopedCanvas(ScopedCanvas&& o) { On 2017/04/06 00:52:11, Peter Kasting wrote: > Nit: Per Google style guide constructors and assignment operator all go above > destructor I didn't realize this, thanks. https://google.github.io/styleguide/cppguide.html#Declaration_Order Removed these for now though.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Here's some numbers with the current patch. I'll cache the recording and see how
that looks.
After (path cache): 0.00782645 ms per PaintInactiveTabBackground
Before (bitmap cache): 0.000617507 ms per PaintInactiveTabBackground
Before (no cache): 0.0178381 ms per PaintInactiveTabBackground
Here's what I did to measure:
+static auto& lap_timer = *new cc::LapTimer(0, base::TimeDelta(), 20);
+ lap_timer.Start();
PaintInactiveTabBackground(canvas, clip);
+ lap_timer.NextLap();
+ LOG(ERROR) << lap_timer.MsPerLap() << " ms per PaintInactiveTabBackground";
On Thu, Apr 6, 2017 at 1:22 PM, <danakj@chromium.org> wrote: > Here's some numbers with the current patch. I'll cache the recording and > see how > that looks. > And because I was curious: After (no cache): 0.00827871 ms per PaintInactiveTabBackground > > After (path cache): 0.00782645 ms per PaintInactiveTabBackground > Before (bitmap cache): 0.000617507 ms per PaintInactiveTabBackground > Before (no cache): 0.0178381 ms per PaintInactiveTabBackground > > Here's what I did to measure: > > +static auto& lap_timer = *new cc::LapTimer(0, base::TimeDelta(), 20); > + lap_timer.Start(); > PaintInactiveTabBackground(canvas, clip); > + lap_timer.NextLap(); > + LOG(ERROR) << lap_timer.MsPerLap() << " ms per > PaintInactiveTabBackground"; > > > > https://codereview.chromium.org/2803583003/ > -- 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.
Reading your speed testing methodology, it looks like this will log the current average time on every call, right? So were the numbers you reported the ones you got at the end of playing around with Chrome for a bit? If so, what sort of tabstrip population did you test? The torture test is basically a couple of windows with 200 tabs each, and we're wiggling the mouse over one. If we can keep unthemed performance strong enough in that case and improve themed performance, this will be a good change. https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1040: gfx::Path* no_clip = nullptr; On 2017/04/06 17:05:28, danakj wrote: > On 2017/04/06 00:52:11, Peter Kasting wrote: > > Nit: Please don't pull this kind of thing out. It's actually more cumbersome > > for me to read than the inline definition, where if I can't recall the meaning > > of the arg both my IDE and codesearch provide it for me very quickly, > primarily > > because the majority of the time I'm scanning and don't need to know what any > of > > the params mean. > > > > I'm not trying to ban doing this across Chromium... just don't do it here :) > > If I didn't give it a name this way I'd want to write write it as "nullptr /* > clip */", but the way you wrote example code below makes me think you don't like > that either. don't love it enough to do it myself, but I don't mind it near so much, because it keeps the annotation inline in the call, so it makes it easy to scan past when you're not caring about that stuff. If you really wanted to pull something out, I'd do something like constexpr bool kDrawAsActive = true; constexpr gfx::Path* kDontClip = nullptr; above the top conditional so these can be reused in the two arms. Using constexpr kConstantName makes it clearer to me that these aren't temporaries, just ways of giving values a name. WDYT? Is that clearer or less clear than naming the args inline? https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1086: may_clip ? &clip : nullptr); On 2017/04/06 17:05:28, danakj wrote: > On 2017/04/06 00:52:11, Peter Kasting wrote: > > Nit: All the temps have made this function hard for me to understand, because > I > > need to keep looking from the function call to multiple different places above > > to determine what the values are. > > > > How about: > > > > bool has_custom_image; > > int fill_id = controller_->GetBackgroundResourceId(&has_custom_image); > > if (has_custom_image) { > > // ... > > const bool fill_id_is_custom = > GetThemeProvider()->HasCustomImage(fill_id); > > const int y_offset = fill_id_is_custom ? 0 : background_offset_.y(); > > PaintTabBackground(canvas, false, fill_id, y_offset, nullptr); > > } else { > > PaintTabBackground(canvas, false, 0, 0, > > controller_->MaySetClip() ? &clip : nullptr); > > } > > > > This makes it much more immediately obvious to me that one case paints a > custom > > image at a possibly-custom offset, and the other doesn't, but might clip. > > I really liked not splitting this into multiple calls to PaintTabBackground, as > that makes much harder to call it with the wrong things IMO, as there's no > copypasta problems. For instance, was it intended that we only clip in the last > case? It seems weird to me, and there's no comment explaining why like there is > for other things. I think this code makes it more explicit by setting may_clip = > false in the block, rather than 2 function calls where one may have just > forgotten to use the clip over time. But done. After suggesting rewriting yesterday, I asked myself the same question about how this clipping was really working. I didn't have time to answer that last night, but I took a look at it again today. The short answer is we should clip (if MaySetClip() is true) all the time. The clip is for the stacked tabs case. Consider two stacked tabs overlaid like this: ________ / / \ / A / B \ /___/________\ When A is drawn, |clip| is the outer path of B. We want to clip A's stroke by this path so we don't get weird "darker stroke" overlap artifacts at the top and bottom intersection points. But we _don't_ want to clip A's fill, because B's left stroke needs to "draw atop" it or it will look like there's a weird gap back to the window frame behind. The code was conditional before because when we have cached images, we can't clip the fill and stroke differently, so we have to pick whether to clip both or neither, and clipping neither looks less wrong. What I don't fully understand is why the custom image case (which never caches) didn't also clip, which could have been done by refactoring the code. I suspect the issue is that I implemented stacked tab support in the new programmatic rendering after I implemented caching, and so I noticed the clipping artifacts for the cached images case and fixed them as best I could, but I didn't test stacked tabs + custom tab theme to notice how this also applied there. Or maybe I just didn't realize this could be done better before. In any case, the interesting question now is what would happen if we basically forced MaySetClip() to return true all the time (to all callers). I filed a bug against myself on the ramifications of that. OK, so all that said, how should this code be written? I was trying to come up with a way last night to get the single call you like while having the temp setting feel more readable to me. I realized that there's a way to clean this up more after this refactoring lands, and filed another bug against myself to do that. For now we can write something pretty similar to what you proposed before, and I can simplify later: int fill_id = controller_->GetBackgroundResourceId(&has_custom_image); int y = 0; if (!has_custom_image) { fill_id = 0; } else if (!GetThemeProvider()->HasCustomImage(fill_id)) { // ... y = background_offset_.y(); } PaintTabBackground(canvas, false, fill_id, y, controller_->MaySetClip() ? &clip : nullptr); https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.h (right): https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:239: bool active, On 2017/04/06 17:05:28, danakj wrote: > On 2017/04/06 00:52:11, Peter Kasting wrote: > > Nit: For naming consistency, make this and the arg in > PaintTabBackgroundStroke() > > |is_active|. > > I changed it because is_active is incorrect. This is "paint as if active". There > is one case where IsActive() is false, but we pass true to paint as active. OK; can we name the arg to the fill function |active| then? https://codereview.chromium.org/2803583003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2803583003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:145: static auto& cache = *new std::list<FillPathCache>; Can we use a base::MRUCache instead of manually rolling the cache management? Nit: I tend to like using CR_DEFINE_STATIC_LOCAL for the idiom of declaring a leaky static local https://codereview.chromium.org/2803583003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:146: static const int kCacheSize = 8; Nit: constexpr size_t https://codereview.chromium.org/2803583003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:193: cache.front().scale = scale; Nit: I'd rather define a constructor so we can just call emplace_front() with these args https://codereview.chromium.org/2803583003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1121: { Nit: You could save a line or two and reduce the indenting by just not scoping this, since it's the last thing in the function (so the actual lifetimes won't be expanded), and we're unlikely to add more drawing later... up to you
On 2017/04/06 18:49:45, Peter Kasting wrote: > Reading your speed testing methodology, it looks like this will log the current > average time on every call, right? So were the numbers you reported the ones > you got at the end of playing around with Chrome for a bit? If so, what sort of > tabstrip population did you test? > > The torture test is basically a couple of windows with 200 tabs each, and we're > wiggling the mouse over one. If we can keep unthemed performance strong enough > in that case and improve themed performance, this will be a good change. One question here. What does the number of tabs have to do with this at all? The time spent in PaintInactiveTabBackground (Or PaintTab if you prefer) should be independent of the number of tabs, other than the size is different? Am I missing something here?
On 2017/04/06 19:13:27, danakj wrote: > On 2017/04/06 18:49:45, Peter Kasting wrote: > > Reading your speed testing methodology, it looks like this will log the > current > > average time on every call, right? So were the numbers you reported the ones > > you got at the end of playing around with Chrome for a bit? If so, what sort > of > > tabstrip population did you test? > > > > The torture test is basically a couple of windows with 200 tabs each, and > we're > > wiggling the mouse over one. If we can keep unthemed performance strong > enough > > in that case and improve themed performance, this will be a good change. > > One question here. What does the number of tabs have to do with this at all? The > time spent in PaintInactiveTabBackground (Or PaintTab if you prefer) should be > independent of the number of tabs, other than the size is different? Am I > missing > something here? The sizes of the inactive tabs are not all the same unless the tabstrip width divides evenly by the number of tabs. This can affect the caching-based solutions. More generally, I'm not totally certain about how things like tab shape, overdraw, how often we repaint tabs, etc. can affect some of the caching stuff. Rather than try to reason about it enough to assure myself that it can't have an effect, I figure it's easier to just test in some torture-test scenarios.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> The sizes of the inactive tabs are not all the same unless the tabstrip width > divides evenly by the number of tabs. This can affect the caching-based > solutions. > > More generally, I'm not totally certain about how things like tab shape, > overdraw, how often we repaint tabs, etc. can affect some of the caching stuff. > Rather than try to reason about it enough to assure myself that it can't have an > effect, I figure it's easier to just test in some torture-test scenarios. Hm, ok so AFAICT your concerns are around a global cache between tabs. To try get an idea of best/worst case behaviours here, what I implemented is a cache on each individual tab, for both active and inactivate states. The cache is not used when fill_id is not 0, or when doing hover, since those would require more complex cache invalidations. Those paths are not significantly different though, in terms of recording. They add a draw image, and a draw rect with shader, respectively and they were both not cached before either, maybe for similar reasons. Smaller tabs should benefit the bitmap case a bit more because it makes smaller bitmaps, but with a rolling average we can see the cases of cache misses leave the data set so they become irrelevant. While testing I put a "miss" print in the cache miss to verify that my test case was testing only against cache hits. First, here's some rehashing from before: 0.00782645 ms per PaintInactiveTabBackground --- After (path cache) 0.00827871 ms per PaintInactiveTabBackground --- After (no cache) 0.00061751 ms per PaintInactiveTabBackground --- Before (bitmap cache) 0.01783810 ms per PaintInactiveTabBackground --- Before (no cache) I was not pleased with the LapTimer though because it keeps an average of all runs, so cache misses skew the average forever. Instead I wrote a rolling average which gives much better results as outliers fall out of the average. I used 11 tabs in each of these cases, which completely fills the top bar and starts shrinking the tabs to display them all: 0.00761 ms per PaintInactiveTabBackground --- After (record cache + path cache) 0.15377 ms per PaintInactiveTabBackground --- After (path cache) 0.00748 ms per PaintInactiveTabBackground --- Before (bitmap cache) There's some noise, but it looks like with the PaintRecord cache we get similar performance to before for the cache-hit case. Then I made caching always miss by breaking the comparison function in the code, so I could measure the miss case as well, using the same scenario of 11 tabs. 0.16352 ms per PaintInactiveTabBackground --- After (picture cache misses + path cache hits) 0.31237 ms per PaintInactiveTabBackground --- Before (bitmap cache misses) I just realized I'm running an ASAN build not an official build after doing all this, lol. Sigh. https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.h (right): https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:239: bool active, On 2017/04/06 18:49:45, Peter Kasting wrote: > On 2017/04/06 17:05:28, danakj wrote: > > On 2017/04/06 00:52:11, Peter Kasting wrote: > > > Nit: For naming consistency, make this and the arg in > > PaintTabBackgroundStroke() > > > |is_active|. > > > > I changed it because is_active is incorrect. This is "paint as if active". > There > > is one case where IsActive() is false, but we pass true to paint as active. > > OK; can we name the arg to the fill function |active| then? Yep, the .cc has it as |active| I just missed it here, will do.
On Thu, Apr 6, 2017 at 1:08 PM, <danakj@chromium.org> wrote: > Hm, ok so AFAICT your concerns are around a global cache between tabs. > There's some of that... for example, if the user has two windows, one with lots of tabs and one with few, a processwide cache may have issues where a per-tabstrip/per-window cache wouldn't. > To try > get > an idea of best/worst case behaviours here, what I implemented is a cache > on > each > individual tab, for both active and inactivate states. The cache is not > used > when > fill_id is not 0, or when doing hover, since those would require more > complex > cache invalidations. Those paths are not significantly different though, in > terms > of recording. They add a draw image, and a draw rect with shader, > respectively > and they were both not cached before either, maybe for similar reasons. > They weren't cached before because such a cache would at best help when repainting the same tab in a future round, and the main optimization we were going for was for painting tab N in the current round to be cheap if it looks like N-1. Ultimately I do want to try to test the "nonzero fill ID" case to see if the final solution makes it better than before. I'd really love for theme users to get better perf than they had previously. PK -- 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.
With non-ASAN release build now.. Here's non-themed case: 0.00249 ms per PaintInactiveTabBackground (after - cache hit) 0.04691 ms per PaintInactiveTabBackground (after - cache miss) 0.00197 ms per PaintInactiveTabBackground (before - cache hit) 0.0887 ms per PaintInactiveTabBackground (before - cache miss) This shows cache misses get much better (not a surprise - no bitmaps!) and cache hits are about the same as before. But we'll hit the cache more often because each tab has a cache in what I wrote. The cache is pretty smaller compared to bitmaps so this seems reasonable, similar to how each ui::View has a cached recording, but Tabs don't go down the normal paint path so they aren't using that (TabStrip would though for when there's no invalidation in the strip at all). I tried some themes but none of them fall into the non-zero fill_id so far.
On 2017/04/06 20:31:37, danakj wrote: > With non-ASAN release build now.. Here's non-themed case: > > 0.00249 ms per PaintInactiveTabBackground (after - cache hit) > 0.04691 ms per PaintInactiveTabBackground (after - cache miss) > 0.00197 ms per PaintInactiveTabBackground (before - cache hit) > 0.0887 ms per PaintInactiveTabBackground (before - cache miss) > > This shows cache misses get much better (not a surprise - no bitmaps!) and cache > hits are about the same as before. But we'll hit the cache more often because > each tab has a cache in what I wrote. The cache is pretty smaller compared to > bitmaps so this seems reasonable, similar to how each ui::View has a cached > recording, but Tabs don't go down the normal paint path so they aren't using > that (TabStrip would though for when there's no invalidation in the strip at > all). > > I tried some themes but none of them fall into the non-zero fill_id so far. Ok found a theme. Here's the fill_id path. 0.04987 ms per PaintInactiveTabBackground (before - no cache) 0.05004 ms per PaintInactiveTabBackground (after - path cache)
On 2017/04/06 20:35:03, danakj wrote: > On 2017/04/06 20:31:37, danakj wrote: > > With non-ASAN release build now.. Here's non-themed case: > > > > 0.00249 ms per PaintInactiveTabBackground (after - cache hit) > > 0.04691 ms per PaintInactiveTabBackground (after - cache miss) > > 0.00197 ms per PaintInactiveTabBackground (before - cache hit) > > 0.0887 ms per PaintInactiveTabBackground (before - cache miss) > > > > This shows cache misses get much better (not a surprise - no bitmaps!) and > cache > > hits are about the same as before. But we'll hit the cache more often because > > each tab has a cache in what I wrote. The cache is pretty smaller compared to > > bitmaps so this seems reasonable, similar to how each ui::View has a cached > > recording, but Tabs don't go down the normal paint path so they aren't using > > that (TabStrip would though for when there's no invalidation in the strip at > > all). > > > > I tried some themes but none of them fall into the non-zero fill_id so far. > > Ok found a theme. Here's the fill_id path. > > 0.04987 ms per PaintInactiveTabBackground (before - no cache) > 0.05004 ms per PaintInactiveTabBackground (after - path cache) For future referece, the theme: https://chrome.google.com/webstore/detail/cute-a-cat-puppy-theme/pgchgahmkhep... This ofc made me wonder what the path cache is accomplishing in my non-ASAN build. So I tried a nontheme scenario with all cache misses and no path cache. 0.05297 ms per PaintInactiveTabBackground (after - cache miss no path cache) 0.04691 ms per PaintInactiveTabBackground (after - cache miss with path cache) So the cache is doing something, but overall this change isn't changing the perf of the theme case. We could extend caching to the theme case though, since the cache is per-tab.
Thanks a ton for those performance numbers. They seem good enough to me to move forward. All the remaining stuff is minor style stuff and I'm confident that whatever outcome we get from it is fine and you won't need further review from me. LGTM
Here's the rolling average I did for future reference as well, in case you want
to steal it:
static auto& times = *new std::vector<base::TimeDelta>;
static int N = 0;
auto start = base::TimeTicks::Now();
PaintInactiveTabBackground(canvas, clip); <---- around that function call
auto end = base::TimeTicks::Now();
if (times.size() < 100)
times.push_back(end - start);
else
times[N % 100] = end - start;
N = (N + 1) % 100;
if (N == 0) {
double avg = 0;
for (const auto& time : times)
avg += time.InMillisecondsF() / times.size();
LOG(ERROR) << avg << " ms per PaintInactiveTabBackground";
}
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
On 2017/04/06 20:40:02, Peter Kasting wrote: > Thanks a ton for those performance numbers. They seem good enough to me to move > forward. > > All the remaining stuff is minor style stuff and I'm confident that whatever > outcome we get from it is fine and you won't need further review from me. LGTM Cool thanks! I've uploaded a patch set with caching of the PaintRecords if you want to have a look at how I did that. I didn't address other comments yet, and will get to them tomorrow.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1120: DCHECK(!y_offset || !fill_id); Nit: I'm not sure why this DCHECK is here since, while it's true, it's not actually an invariant this function relies on or needs to understand. https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1123: SkColor active_color = tp->GetColor(ThemeProperties::COLOR_TOOLBAR); Nit: These could all be const https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1129: // changed, and invalidating when more position values changed. Nit: more position values changed -> the X coordinate changes? https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1140: canvas->sk_canvas()->clipPath(*clip, SkClipOp::kDifference, true); These bits about scoping a canvas, then clipping, are repeated below. I wonder if we can use a lambda or binding or something to only write them once. https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1147: bool matches_cache = cache.scale == canvas->image_scale() && Nit: I liked how the old code had a metadata struct for this stuff that could be constructed, compared, and used when enqueueing new entries. It is more verbose when defining the cache structs, but it lets the code here just say "here's the metadata for the current object" and then do things like checking for a match or resetting the cached entry very simply and with minimal repetitiveness. https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1197: const ui::ThemeProvider* tp = GetThemeProvider(); Nit: Just inline this below? https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1211: if (hover) { Nit: Maybe name this |draw_highlight| or |draw_hover|? https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.h (right): https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:372: struct BackgroundCache { Nit: I think the Google style guide requests that structs go atop the section, even though I understand why you defined it down here next to the use of the type. https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:379: SkColor active_color; Nit: If you're going to init |scale| in the declaration list I'd also init these colors (which are really BTs as well).
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1034: const int kActiveTabFillId = btw while we're pointing at naming, these should not be kSyntax cuz they are not constant for the duration of the program. https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1040: gfx::Path* no_clip = nullptr; On 2017/04/06 18:49:44, Peter Kasting wrote: > On 2017/04/06 17:05:28, danakj wrote: > > On 2017/04/06 00:52:11, Peter Kasting wrote: > > > Nit: Please don't pull this kind of thing out. It's actually more > cumbersome > > > for me to read than the inline definition, where if I can't recall the > meaning > > > of the arg both my IDE and codesearch provide it for me very quickly, > > primarily > > > because the majority of the time I'm scanning and don't need to know what > any > > of > > > the params mean. > > > > > > I'm not trying to ban doing this across Chromium... just don't do it here :) > > > > If I didn't give it a name this way I'd want to write write it as "nullptr /* > > clip */", but the way you wrote example code below makes me think you don't > like > > that either. > don't love it enough to do it myself, but I don't mind it near so much, because > it keeps the annotation inline in the call, so it makes it easy to scan past > when you're not caring about that stuff. > > If you really wanted to pull something out, I'd do something like > > constexpr bool kDrawAsActive = true; > constexpr gfx::Path* kDontClip = nullptr; > > above the top conditional so these can be reused in the two arms. > > Using constexpr kConstantName makes it clearer to me that these aren't > temporaries, just ways of giving values a name. > > WDYT? Is that clearer or less clear than naming the args inline? Sure, I think that's nicer than inline comments. I don't super care for kSyntax for local variables since it makes inconsistent names with compile-vs-runtime const variables, but I don't mind and I can see your point about them. https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1086: may_clip ? &clip : nullptr); On 2017/04/06 18:49:44, Peter Kasting wrote: > On 2017/04/06 17:05:28, danakj wrote: > > On 2017/04/06 00:52:11, Peter Kasting wrote: > > > Nit: All the temps have made this function hard for me to understand, > because > > I > > > need to keep looking from the function call to multiple different places > above > > > to determine what the values are. > > > > > > How about: > > > > > > bool has_custom_image; > > > int fill_id = controller_->GetBackgroundResourceId(&has_custom_image); > > > if (has_custom_image) { > > > // ... > > > const bool fill_id_is_custom = > > GetThemeProvider()->HasCustomImage(fill_id); > > > const int y_offset = fill_id_is_custom ? 0 : background_offset_.y(); > > > PaintTabBackground(canvas, false, fill_id, y_offset, nullptr); > > > } else { > > > PaintTabBackground(canvas, false, 0, 0, > > > controller_->MaySetClip() ? &clip : nullptr); > > > } > > > > > > This makes it much more immediately obvious to me that one case paints a > > custom > > > image at a possibly-custom offset, and the other doesn't, but might clip. > > > > I really liked not splitting this into multiple calls to PaintTabBackground, > as > > that makes much harder to call it with the wrong things IMO, as there's no > > copypasta problems. For instance, was it intended that we only clip in the > last > > case? It seems weird to me, and there's no comment explaining why like there > is > > for other things. I think this code makes it more explicit by setting may_clip > = > > false in the block, rather than 2 function calls where one may have just > > forgotten to use the clip over time. But done. > > After suggesting rewriting yesterday, I asked myself the same question about how > this clipping was really working. I didn't have time to answer that last night, > but I took a look at it again today. > > The short answer is we should clip (if MaySetClip() is true) all the time. > > The clip is for the stacked tabs case. Consider two stacked tabs overlaid like > this: > > ________ > / / \ > / A / B \ > /___/________\ > > When A is drawn, |clip| is the outer path of B. We want to clip A's stroke by > this path so we don't get weird "darker stroke" overlap artifacts at the top and > bottom intersection points. But we _don't_ want to clip A's fill, because B's > left stroke needs to "draw atop" it or it will look like there's a weird gap > back to the window frame behind. > > The code was conditional before because when we have cached images, we can't > clip the fill and stroke differently, so we have to pick whether to clip both or > neither, and clipping neither looks less wrong. What I don't fully understand > is why the custom image case (which never caches) didn't also clip, which could > have been done by refactoring the code. I suspect the issue is that I > implemented stacked tab support in the new programmatic rendering after I > implemented caching, and so I noticed the clipping artifacts for the cached > images case and fixed them as best I could, but I didn't test stacked tabs + > custom tab theme to notice how this also applied there. Or maybe I just didn't > realize this could be done better before. > > In any case, the interesting question now is what would happen if we basically > forced MaySetClip() to return true all the time (to all callers). I filed a bug > against myself on the ramifications of that. > > OK, so all that said, how should this code be written? > > I was trying to come up with a way last night to get the single call you like > while having the temp setting feel more readable to me. I realized that there's > a way to clean this up more after this refactoring lands, and filed another bug > against myself to do that. For now we can write something pretty similar to > what you proposed before, and I can simplify later: > > int fill_id = controller_->GetBackgroundResourceId(&has_custom_image); > int y = 0; > if (!has_custom_image) { > fill_id = 0; > } else if (!GetThemeProvider()->HasCustomImage(fill_id)) { > // ... > y = background_offset_.y(); > } > PaintTabBackground(canvas, false, fill_id, y, > controller_->MaySetClip() ? &clip : nullptr); Done. Also observing that we always now clip inactive tabs if MaySetClip(), and we never clip active tabs. https://codereview.chromium.org/2803583003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2803583003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:145: static auto& cache = *new std::list<FillPathCache>; On 2017/04/06 18:49:45, Peter Kasting wrote: > Can we use a base::MRUCache instead of manually rolling the cache management? We could, I had looked there first, but it just looked like a heck of a lot of code to do erase and push_front, it makes us use a std::map and to define operator<, which both seem overkill for a list of 8 items. We can just walk the list searching. Do you really think it's a good fit here? > Nit: I tend to like using CR_DEFINE_STATIC_LOCAL for the idiom of declaring a > leaky static local Oh, I didn't know that exists and TBH I'm not sure it should. It's comment is wrong now, and we've been converting LazyInstances to be what I wrote here. I get the feeling that macro exists to not write |type| twice, but auto does that for us now. What value do u get out of the macro that you prefer it? https://codereview.chromium.org/2803583003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:146: static const int kCacheSize = 8; On 2017/04/06 18:49:45, Peter Kasting wrote: > Nit: constexpr size_t Done. https://codereview.chromium.org/2803583003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:193: cache.front().scale = scale; On 2017/04/06 18:49:45, Peter Kasting wrote: > Nit: I'd rather define a constructor so we can just call emplace_front() with > these args Done. Made a constructor and a HasMatchingParameters, and it's now a class not a struct. https://codereview.chromium.org/2803583003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1121: { On 2017/04/06 18:49:45, Peter Kasting wrote: > Nit: You could save a line or two and reduce the indenting by just not scoping > this, since it's the last thing in the function (so the actual lifetimes won't > be expanded), and we're unlikely to add more drawing later... up to you Done. https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1120: DCHECK(!y_offset || !fill_id); On 2017/04/07 00:59:19, Peter Kasting wrote: > Nit: I'm not sure why this DCHECK is here since, while it's true, it's not > actually an invariant this function relies on or needs to understand. I added this because I think it should be an invariant. Anything that is passed to PaintTabBackground{Fill,Stroke} needs to be used to invalidate the cache. However y_offset does not but only because we have a branch earlier that skips the cache when there's a fill_id, and y_offset is only used with fill_id. This DCHECK proves that y_offset is always 0 to the caching code. This was failing in some tests because in the active case we were sometimes passing a y offset without a fill id. I've fixed that, and it also relates to the naming of those variables being wrong..they are fixed in the process. https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1123: SkColor active_color = tp->GetColor(ThemeProperties::COLOR_TOOLBAR); On 2017/04/07 00:59:19, Peter Kasting wrote: > Nit: These could all be const Sure, I guess I don't see much reason for putting const on local value-type variables, but I can. https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1129: // changed, and invalidating when more position values changed. On 2017/04/07 00:59:19, Peter Kasting wrote: > Nit: more position values changed -> the X coordinate changes? Also background_offset_ (both x() which is used directly and y() which is passed in as the y_offset). https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1140: canvas->sk_canvas()->clipPath(*clip, SkClipOp::kDifference, true); On 2017/04/07 00:59:19, Peter Kasting wrote: > These bits about scoping a canvas, then clipping, are repeated below. I wonder > if we can use a lambda or binding or something to only write them once. It's a nice thought, the problem is we have to change the PaintTabBackgroundFill calls out with painting from the cache in the middle of it. I can't think of an easy way to do this that would not be harder to read. https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1147: bool matches_cache = cache.scale == canvas->image_scale() && On 2017/04/07 00:59:19, Peter Kasting wrote: > Nit: I liked how the old code had a metadata struct for this stuff that could be > constructed, compared, and used when enqueueing new entries. It is more verbose > when defining the cache structs, but it lets the code here just say "here's the > metadata for the current object" and then do things like checking for a match or > resetting the cached entry very simply and with minimal repetitiveness. It feels like it should be less, but It's a more lines of code, and they move further away, to do the same thing, as well as duplicates the storage of all the fields in the metadata. 2 files changed, 34 insertions(+), 17 deletions(-) Is just adding a metadata class, 2 constructors, and a comparator. Counter proposal, I make a Check and Set methods so we can compile-time verify that we're not skipping fields? 2 files changed, 39 insertions(+), 22 deletions(-) This adds more code but it feels less indirect without making a key structure I guess, cuz we're comparing and setting to the same values that we see written as arguments to PaintTabBackground{Fill,Stroke}(). wdyt? https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1197: const ui::ThemeProvider* tp = GetThemeProvider(); On 2017/04/07 00:59:19, Peter Kasting wrote: > Nit: Just inline this below? Done. https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1211: if (hover) { On 2017/04/07 00:59:20, Peter Kasting wrote: > Nit: Maybe name this |draw_highlight| or |draw_hover|? went with paint_hover_effect. "draw" is a overloaded term that i try to only use when referring to compositor drawing its final output to the display. here we're painting. I expect our PaintCanvas and such APIs to reflect this better over time. https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.h (right): https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:372: struct BackgroundCache { On 2017/04/07 00:59:20, Peter Kasting wrote: > Nit: I think the Google style guide requests that structs go atop the section, > even though I understand why you defined it down here next to the use of the > type. yeh.. that seems unfortunate here. The guide says "Within each section, generally prefer grouping similar kinds of declarations together" but its not strictly always. I'd prefer to keep this where it is if you're okay with that. I think it would reduce readability to move it. https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:379: SkColor active_color; On 2017/04/07 00:59:20, Peter Kasting wrote: > Nit: If you're going to init |scale| in the declaration list I'd also init these > colors (which are really BTs as well). Right, don't want to compare uninit things, even tho scale is enough to cause the initial cache to not be value.
Description was changed from ========== ui: Remove use of bitmaps when painting tab backgrounds. This removes the use of gfx::Canvas::ExtractImageRep() which was being used for inactive tab backgrounds, to generate two bitmaps and draw them into the recording canvas when a clip on the second one. Instead, we just record the commands all directly to the recording canvas, and insert a save+clip+restore around the commands that were going into the second bitmap previously. Because we're no longer using bitmaps, this alleviates the need for caching and we remove the global tab background-bitmap cache. Also we collapse all cases in Tab::PaintInactiveTabBackground down to a single code path with some variables to control the output. The only change in behaviour due to collapsing code is that when a hover effect is displayed, and also TabController::MaySetClip() is true, this allows clipping out for the stroke, whereas before the clip would be ignored when a hover effect was present. This seems reasonable and/or an improvement since the hover effect is entirely part of the fill, not the stroke, and so does not interact with the clip anyhow. R=pkasting@chromium.org BUG=671433 ========== to ========== ui: Remove use of bitmaps when painting tab backgrounds. This removes the use of gfx::Canvas::ExtractImageRep() which was being used for inactive tab backgrounds, to generate two bitmaps and draw them into the recording canvas when a clip on the second one. Instead, we just record the commands all directly to the recording canvas, and insert a save+clip+restore around the commands that were going into the second bitmap previously. Because we're no longer using bitmaps, this alleviates the need for caching and we remove the global tab background-bitmap cache. Also we collapse all cases in Tab::PaintInactiveTabBackground down to a single code path with some variables to control the output. The only change in behaviour due to collapsing code is that when TabController::MaySetClip() is true we always apply the clip for inactive tabs, whereas before the clip ould be ignored when a hover effect or custom fill image was present. This is more correct as described by pkasting in the CL review comments. R=pkasting@chromium.org BUG=671433 ==========
I tried templating the FillPathCache code into a PathCache, and made use of it for the border cache as well. I hardcoded missing the PaintRecord cache always. With no path cache: 0.0541833 ms per PaintInactiveTabBackground 0.04972 ms per PaintInactiveTabBackground 0.0500833 ms per PaintInactiveTabBackground With path cache for fill and stroke: 0.0512 ms per PaintInactiveTabBackground 0.0520233 ms per PaintInactiveTabBackground 0.0497367 ms per PaintInactiveTabBackground With fill cache but not stroke: 0.0523367 ms per PaintInactiveTabBackground 0.04978 ms per PaintInactiveTabBackground 0.0537533 ms per PaintInactiveTabBackground I'm not seeing the path cache do more than noise here anymore. I double checked before pulling the cache out to a template class, which is using the std::list directly: 0.05112 ms per PaintInactiveTabBackground 0.0492533 ms per PaintInactiveTabBackground 0.05287 ms per PaintInactiveTabBackground So I just removed the path cache. Maybe it was ASAN biasing it into looking useful. One more double-check of the last patch set I'm uploading with the path cache removed: 0.05021 ms per PaintInactiveTabBackground 0.0517067 ms per PaintInactiveTabBackground 0.0495367 ms per PaintInactiveTabBackground
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1120: DCHECK(!y_offset || !fill_id); On 2017/04/07 19:10:49, danakj wrote: > On 2017/04/07 00:59:19, Peter Kasting wrote: > > Nit: I'm not sure why this DCHECK is here since, while it's true, it's not > > actually an invariant this function relies on or needs to understand. > > I added this because I think it should be an invariant. Anything that is passed > to PaintTabBackground{Fill,Stroke} needs to be used to invalidate the cache. > However y_offset does not but only because we have a branch earlier that skips > the cache when there's a fill_id, and y_offset is only used with fill_id. This > DCHECK proves that y_offset is always 0 to the caching code. > > This was failing in some tests because in the active case we were sometimes > passing a y offset without a fill id. I've fixed that, and it also relates to > the naming of those variables being wrong..they are fixed in the process. Oh I had the logic inverted though. I miss DCHECK_IMPLIES. Fixed it to !y_offset || fill_id because y_offset => fill_id.
The CQ bit was checked by danakj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Have not re-reviewed https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2803583003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1086: may_clip ? &clip : nullptr); On 2017/04/07 19:10:49, danakj wrote: > On 2017/04/06 18:49:44, Peter Kasting wrote: > > For now we can write something pretty similar to > > what you proposed before, and I can simplify later: > > > > int fill_id = controller_->GetBackgroundResourceId(&has_custom_image); > > int y = 0; > > if (!has_custom_image) { > > fill_id = 0; > > } else if (!GetThemeProvider()->HasCustomImage(fill_id)) { > > // ... > > y = background_offset_.y(); > > } > > PaintTabBackground(canvas, false, fill_id, y, > > controller_->MaySetClip() ? &clip : nullptr); > > Done. Also observing that we always now clip inactive tabs if MaySetClip(), and > we never clip active tabs. Correct; the active tab should always be frontmost, so it should never need to be clipped. https://codereview.chromium.org/2803583003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2803583003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:145: static auto& cache = *new std::list<FillPathCache>; On 2017/04/07 19:10:49, danakj wrote: > On 2017/04/06 18:49:45, Peter Kasting wrote: > > Can we use a base::MRUCache instead of manually rolling the cache management? > > We could, I had looked there first, but it just looked like a heck of a lot of > code to do erase and push_front, it makes us use a std::map and to define > operator<, which both seem overkill for a list of 8 items. We can just walk the > list searching. Do you really think it's a good fit here? Would it make sense to move MRUCache to one of the flat container types since its size is likely small? Would that also avoid the need to define operator<()? That would be done as a separate CL of course... It seems like if we have an MRUCache type, but it doesn't actually fit well where people want an MRU cache, it's not serving its purpose very well, so I'd like to fix that. From the perspective of code here, it's kinda the same set of tradeoffs as using things from <algorithm>: you get to express more directly and hopefully with known-bug-free code what you're trying to do, though you may have to jump through hoops to be able to do it. > > Nit: I tend to like using CR_DEFINE_STATIC_LOCAL for the idiom of declaring a > > leaky static local > > Oh, I didn't know that exists and TBH I'm not sure it should. It's comment is > wrong now, and we've been converting LazyInstances to be what I wrote here. I > get the feeling that macro exists to not write |type| twice, but auto does that > for us now. What value do u get out of the macro that you prefer it? Not writing the type twice is nice; trying to be consistent with what other Chrome code does is nice (at least until threadsafe statics, I think most people used this macro instead of writing it out); and I find the macro name a bit more obvious as to its purpose than the actual underlying syntax as used directly here. None of these are huge things. It's possible this is worth a quick discussion on cxx or chromium-dev to get a feel for how we should recommend people do things going forward and whether we want to retire the macro. https://codereview.chromium.org/2803583003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:153: cache.splice(cache.begin(), std::move(hit)); Can we just use std::rotate? This should work with lists and seems clearer and safer than using a temp and some splices. https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1147: bool matches_cache = cache.scale == canvas->image_scale() && On 2017/04/07 19:10:49, danakj wrote: > On 2017/04/07 00:59:19, Peter Kasting wrote: > > Nit: I liked how the old code had a metadata struct for this stuff that could > be > > constructed, compared, and used when enqueueing new entries. It is more > verbose > > when defining the cache structs, but it lets the code here just say "here's > the > > metadata for the current object" and then do things like checking for a match > or > > resetting the cached entry very simply and with minimal repetitiveness. > > It feels like it should be less, but It's a more lines of code, and they move > further away, to do the same thing, as well as duplicates the storage of all the > fields in the metadata. It's definitely more verbose overall. I'm OK with that -- they get out of the way of code here, so they let the code here be more expressive about what it's trying to do without spelling things out. I don't think I follow the duplicating the storage comment. > Counter proposal, I make a Check and Set methods so we can compile-time verify > that we're not skipping fields? If I understand what you're proposing correctly, I'm pretty much fine with this, as it accomplishes the same goal of moving the minutiae of the checks away from here. It seems like it would still force you to pass all the args to each function each time, which is kind of unfortunate. Maybe we can construct a new cache entry up-front (without paint records in it) and use it for the comparison, then if we want to add it set its paint records and add? https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.h (right): https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:372: struct BackgroundCache { On 2017/04/07 19:10:49, danakj wrote: > On 2017/04/07 00:59:20, Peter Kasting wrote: > > Nit: I think the Google style guide requests that structs go atop the section, > > even though I understand why you defined it down here next to the use of the > > type. > > yeh.. that seems unfortunate here. The guide says "Within each section, > generally prefer grouping similar kinds of declarations together" but its not > strictly always. I'd prefer to keep this where it is if you're okay with that. I > think it would reduce readability to move it. It's OK to keep it here. It'd be even better to move it into the .cc file somehow, but not at the expense of having to create unique_ptrs for this type for the members and do heap allocations.
(You still have standing approval) https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1140: canvas->sk_canvas()->clipPath(*clip, SkClipOp::kDifference, true); On 2017/04/07 19:10:49, danakj wrote: > On 2017/04/07 00:59:19, Peter Kasting wrote: > > These bits about scoping a canvas, then clipping, are repeated below. I > wonder > > if we can use a lambda or binding or something to only write them once. > > It's a nice thought, the problem is we have to change the PaintTabBackgroundFill > calls out with painting from the cache in the middle of it. I can't think of an > easy way to do this that would not be harder to read. The way I can think of, which may not be a win, is to basically have something akin to: * Create locals for "paint fill" and "paint stroke" which are Closures, or lambdas or something * Do something like: if (fill_id || hover) { paint_fill = Bind(PaintTabBackgroundFill, args); paint_stroke = Bind(PaintTabBackgroundStroke, args); } else { ... check cache paint_fill = Bind(PlaybackPaintRecord, args); paint_stroke = Bind(PlaybackPaintRecord, args); } paint_fill(); gfx::ScopedCanvas scoped_canvas(clip ? canvas : nullptr); if (clip) canvas->sk_canvas()->clipPath(*clip, SkClipOp::kDifference, true); canvas->sk_canvas()->PlaybackPaintRecord(cache.stroke_record); One can imagine splitting the last bit into a function, so the caller here could avoid temps by instead just calling the function and passing the correct things directly. https://codereview.chromium.org/2803583003/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2803583003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1044: active_tab_y_offset, nullptr /* clip */); Nit: Optional, but if you defined this above the conditional: const auto PaintBackgroundAsActive = [&]() { PaintTabBackground(canvas, true /* active */, active_tab_fill_id, active_tab_y_offset, nullptr /* clip */); } ...then you could use it in both arms to make it clear both do the same thing, and avoid copy/paste It's longer overall. I dunno if it's worth it. https://codereview.chromium.org/2803583003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1064: const ui::ThemeProvider* tp = GetThemeProvider(); Nit: I suggest inlining this into the one use below. This will help when I refactor that code later to make sure I don't forget to remove this variable. https://codereview.chromium.org/2803583003/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.h (right): https://codereview.chromium.org/2803583003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:374: struct BackgroundCache { Nit: class? https://codereview.chromium.org/2803583003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:378: bool CacheKeyIsValid(float scale, Nit: IsValid -> Matches? I mean, the key is always "valid", in some sense. https://codereview.chromium.org/2803583003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:378: bool CacheKeyIsValid(float scale, Nit: Since these are named CamelCase, I'd probably define them out-of-line in the .cc file as you've done with the constructor https://codereview.chromium.org/2803583003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:408: SkColor active_color_ = 0; Nit: Maybe use SK_ColorTRANSPARENT instead, which has the value of 0 but doesn't expose to the reader here that the type is an int?
The CQ bit was checked by danakj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2803583003/#ps340001 (title: "tabbackground: nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by danakj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2803583003/#ps360001 (title: "tabbackground: .")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2803583003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2803583003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:153: cache.splice(cache.begin(), std::move(hit)); On 2017/04/07 23:34:02, Peter Kasting wrote: > Can we just use std::rotate? This should work with lists and seems clearer and > safer than using a temp and some splices. I deleted the cache here anyhow, but I think rotate() would require moving/copying SkPaths around in the container. I used splice because that let me use a pointer indirection (list nodes) without dealing with pointers myself via unique_ptr. https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2803583003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1147: bool matches_cache = cache.scale == canvas->image_scale() && On 2017/04/07 23:34:03, Peter Kasting wrote: > On 2017/04/07 19:10:49, danakj wrote: > > On 2017/04/07 00:59:19, Peter Kasting wrote: > > > Nit: I liked how the old code had a metadata struct for this stuff that > could > > be > > > constructed, compared, and used when enqueueing new entries. It is more > > verbose > > > when defining the cache structs, but it lets the code here just say "here's > > the > > > metadata for the current object" and then do things like checking for a > match > > or > > > resetting the cached entry very simply and with minimal repetitiveness. > > > > It feels like it should be less, but It's a more lines of code, and they move > > further away, to do the same thing, as well as duplicates the storage of all > the > > fields in the metadata. > > It's definitely more verbose overall. I'm OK with that -- they get out of the > way of code here, so they let the code here be more expressive about what it's > trying to do without spelling things out. > > I don't think I follow the duplicating the storage comment. > > > Counter proposal, I make a Check and Set methods so we can compile-time verify > > that we're not skipping fields? > > If I understand what you're proposing correctly, I'm pretty much fine with this, > as it accomplishes the same goal of moving the minutiae of the checks away from > here. It seems like it would still force you to pass all the args to each > function each time, which is kind of unfortunate. Maybe we can construct a new > cache entry up-front (without paint records in it) and use it for the > comparison, then if we want to add it set its paint records and add? The duplicate storage I mean like.. float scale .. Size size.. SkColor active_color.. Metadata m(scale, size, active_color); if (m != m_) DoThings(scale, size, active_color); m_ = m; Here we're storing scale/size/active_color a second time in this function, inside m. But then using it to compare but not using it for the methods. It disconnects the comparison from passing the things to do the actual work, and we have 2 scales 2 sizes, 2 colors etc. Whereas what I did is float scale .. Size size.. SkColor active_color.. if (!m_.Compare(scale, size, active_color)) DoThings(scale, size, active_color); M_.Set(scale, size, active_color); This is the same overall work. m_=m would be copying all the local fields too. But this you see the comparison and the cache are the same physical fields as used to do the work, and you only have 1 instance of each variable inside the method. https://codereview.chromium.org/2803583003/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2803583003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1044: active_tab_y_offset, nullptr /* clip */); On 2017/04/08 00:59:37, Peter Kasting wrote: > Nit: Optional, but if you defined this above the conditional: > > const auto PaintBackgroundAsActive = [&]() { > PaintTabBackground(canvas, true /* active */, active_tab_fill_id, > active_tab_y_offset, nullptr /* clip */); > } > > ...then you could use it in both arms to make it clear both do the same thing, > and avoid copy/paste > > It's longer overall. I dunno if it's worth it. Yeah, I'm not sure it's making this easier to understand. https://codereview.chromium.org/2803583003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.cc:1064: const ui::ThemeProvider* tp = GetThemeProvider(); On 2017/04/08 00:59:37, Peter Kasting wrote: > Nit: I suggest inlining this into the one use below. This will help when I > refactor that code later to make sure I don't forget to remove this variable. clang should complain about an unused var, but done. https://codereview.chromium.org/2803583003/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab.h (right): https://codereview.chromium.org/2803583003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:374: struct BackgroundCache { On 2017/04/08 00:59:38, Peter Kasting wrote: > Nit: class? Done. https://codereview.chromium.org/2803583003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:378: bool CacheKeyIsValid(float scale, On 2017/04/08 00:59:38, Peter Kasting wrote: > Nit: Since these are named CamelCase, I'd probably define them out-of-line in > the .cc file as you've done with the constructor I expect hacker_case to be inlined but I've always considered CamelCase can go either way as appropriate. https://codereview.chromium.org/2803583003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:378: bool CacheKeyIsValid(float scale, On 2017/04/08 00:59:38, Peter Kasting wrote: > Nit: IsValid -> Matches? I mean, the key is always "valid", in some sense. Done. https://codereview.chromium.org/2803583003/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab.h:408: SkColor active_color_ = 0; On 2017/04/08 00:59:37, Peter Kasting wrote: > Nit: Maybe use SK_ColorTRANSPARENT instead, which has the value of 0 but doesn't > expose to the reader here that the type is an int? Technically these don't matter, cuz when the scale is 0, the comparison will fail already, and I just wanted to init to anything. I'm not sure that TRANSPARENT is hiding much. If it was a complex type you could not initialize it this way. And 0 works for any primitive type pretty much. I think I'd prefer to leave them. I'd agree if I was trying to actually set these to a color though fwiw.
CQ is committing da patch.
Bot data: {"patchset_id": 360001, "attempt_start_ts": 1492530754616280,
"parent_rev": "7076919baf44c4b45db4ea699b833c1485702506", "commit_rev":
"87648f2cf83870acac8d73ea6bb61a5dcf11b921"}
Message was sent while issue was closed.
Description was changed from ========== ui: Remove use of bitmaps when painting tab backgrounds. This removes the use of gfx::Canvas::ExtractImageRep() which was being used for inactive tab backgrounds, to generate two bitmaps and draw them into the recording canvas when a clip on the second one. Instead, we just record the commands all directly to the recording canvas, and insert a save+clip+restore around the commands that were going into the second bitmap previously. Because we're no longer using bitmaps, this alleviates the need for caching and we remove the global tab background-bitmap cache. Also we collapse all cases in Tab::PaintInactiveTabBackground down to a single code path with some variables to control the output. The only change in behaviour due to collapsing code is that when TabController::MaySetClip() is true we always apply the clip for inactive tabs, whereas before the clip ould be ignored when a hover effect or custom fill image was present. This is more correct as described by pkasting in the CL review comments. R=pkasting@chromium.org BUG=671433 ========== to ========== ui: Remove use of bitmaps when painting tab backgrounds. This removes the use of gfx::Canvas::ExtractImageRep() which was being used for inactive tab backgrounds, to generate two bitmaps and draw them into the recording canvas when a clip on the second one. Instead, we just record the commands all directly to the recording canvas, and insert a save+clip+restore around the commands that were going into the second bitmap previously. Because we're no longer using bitmaps, this alleviates the need for caching and we remove the global tab background-bitmap cache. Also we collapse all cases in Tab::PaintInactiveTabBackground down to a single code path with some variables to control the output. The only change in behaviour due to collapsing code is that when TabController::MaySetClip() is true we always apply the clip for inactive tabs, whereas before the clip ould be ignored when a hover effect or custom fill image was present. This is more correct as described by pkasting in the CL review comments. R=pkasting@chromium.org BUG=671433 Review-Url: https://codereview.chromium.org/2803583003 Cr-Commit-Position: refs/heads/master@{#465266} Committed: https://chromium.googlesource.com/chromium/src/+/87648f2cf83870acac8d73ea6bb6... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:360001) as https://chromium.googlesource.com/chromium/src/+/87648f2cf83870acac8d73ea6bb6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
