Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2197613002:
Make PaintTabBackgroundUsingFillId() non-method (Closed)
|
Created:
4 years, 4 months ago by Greg Levin Modified:
4 years, 4 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, miu+watch_chromium.org, oshima Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake PaintTabBackgroundUsingFillId() non-method
BUG=621139
As per off-line request, I've separated part of
https://codereview.chromium.org/2118853002/
into this CL. This moves the main PaintTabBackgroundUsingFillId()
implementation into a non-method in anonymous namespace. This CL contains
no functional changes, and is intended to make the functional changes in
the original CL easier to see.
Committed: https://crrev.com/4c7ae812a813610568c15c2ed7b24cc4e605646f
Cr-Commit-Position: refs/heads/master@{#410749}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address review #
Total comments: 10
Patch Set 3 : Merge #Patch Set 4 : Address review 2 #
Total comments: 1
Messages
Total messages: 15 (6 generated)
Description was changed from ========== Make PaintTabBackgroundUsingFillId() non-method BUG=621139 As per off-line request, I've separated part of https://codereview.chromium.org/2118853002/ into this CL. This moves the main PaintTabBackgroundUsingFillId() implementation into a non-method in anonymous namespace. There are no functional changes in this CL. ========== to ========== Make PaintTabBackgroundUsingFillId() non-method BUG=621139 As per off-line request, I've separated part of https://codereview.chromium.org/2118853002/ into this CL. This moves the main PaintTabBackgroundUsingFillId() implementation into a non-method in anonymous namespace. This CL contains no functional changes, and is intended to make the functional changes in the original CL easier to see. ==========
glevin@chromium.org changed reviewers: + pkasting@chromium.org
pkasting@ - As per offline request, I've separated the PaintTabBackgroundUsingFillId() refactor into this CL. Please have a look!
I added a few comments regarding ongoing conversations in the other code review that could be resolved there. pkasting@, any thoughts on these? https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:127: gfx::Size size; Are offset and size logically connected enough that it would make sense to combine them into gfx::Rect offset_rect; https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:432: if (params.has_custom_image) { Given the change below at 1586, it looks like we can remove has_custom_image, and instead use if (!fill_image.isNull()) { here instead. Do you see any problem with that? https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:1586: params.fill_image = *GetThemeProvider()->GetImageSkiaNamed(fill_id); Changed so that we only load the image if it's going to be used.
https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:122: struct PaintBackgroundParams { Nit: I'd probably define this just above the function that uses it, with no ///// dividers around it. (This file uses a lot of those, and it's a bit of an outlier in doing so.) Regardless, wherever you put it the struct should have a comment above it saying briefly what it's for. https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:127: gfx::Size size; On 2016/07/29 15:02:56, Greg Levin wrote: > Are offset and size logically connected enough that it would make sense to > combine them into > gfx::Rect offset_rect; Probably, but if you do this I would also change PaintTabFill() to take a rect. https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:407: // For use by Tab and TabImageSource Nit: There's no TabImageSource yet, so this comment is kinda weird, but more importantly, I don't think this helps understand what this function does, and it's the sort of comment that could get out of date when refactoring. If you think the function deserves a comment, it should probably be something more like "Helper function to paint a tab's background using the information in |params|.", but honestly that looks to me like it just restates the declaration, so I'd probably omit it entirely. https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:432: if (params.has_custom_image) { On 2016/07/29 15:02:56, Greg Levin wrote: > Given the change below at 1586, it looks like we can remove has_custom_image, > and instead use > if (!fill_image.isNull()) { > here instead. Do you see any problem with that? I think that would work. https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:1582: hover_controller_.ShouldDraw() ? &hover_controller_ : nullptr; Nit: Looking at the original CL, I see why you didn't put the ShouldDraw() call here in the callee. But I'm not sure why you didn't put |canvas| or |hc| into |params|. Seems like those are parameters too. I know one reason not to put |canvas| in, which is that back in your original CL, |canvas| is the only thing changing on subsequent calls from TabImageSource. So if you omit it, and you add a constructor for PaintBackgroundParams, you could use that constructor to initialize TabImageSource::params_, and declare it const. (Which would be nice.) But you could still have the hover controller be a param and have that work. Given that, I suggest making the hover controller part of the params, and adding a constructor for the params struct, which you can use here too. I don't think we lose a lot of readability converting the separate named field assignments into arguments to a constructor call, since the list of argument names is above in this same file to reference. https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:1599: ::PaintTabBackgroundUsingFillId(canvas, hc, params); Nit: This no longer takes a fill ID, so the name isn't quite right. I'd probably call it PaintTabBackgroundUsingParams(). That allows eliminating the :: too.
I'm back. Here are requested revisions with comments. Please have a look! https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:122: struct PaintBackgroundParams { On 2016/07/29 18:58:52, Peter Kasting wrote: > Nit: I'd probably define this just above the function that uses it, with no > ///// dividers around it. (This file uses a lot of those, and it's a bit of an > outlier in doing so.) Regardless, wherever you put it the struct should have a > comment above it saying briefly what it's for. In the final CL, this comes right above TabImageSource, a new locally defined class like ImageCacheEntryMetadata. That class does call PaintTabBackgroundUsingFillId(), which will have a prototype between this struct and TabImageSource. So putting this struct here is (or will be) sort of right above the function that uses it. I've removed the //// divider. https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:127: gfx::Size size; On 2016/07/29 18:58:52, Peter Kasting wrote: > On 2016/07/29 15:02:56, Greg Levin wrote: > > Are offset and size logically connected enough that it would make sense to > > combine them into > > gfx::Rect offset_rect; > > Probably, but if you do this I would also change PaintTabFill() to take a rect. Done ... Do you think this is an improvement? I'm not sure about it's merits, but it's caused about 10 new line wraps. https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:407: // For use by Tab and TabImageSource On 2016/07/29 18:58:52, Peter Kasting wrote: > Nit: There's no TabImageSource yet, so this comment is kinda weird, but more > importantly, I don't think this helps understand what this function does, and > it's the sort of comment that could get out of date when refactoring. If you > think the function deserves a comment, it should probably be something more like > "Helper function to paint a tab's background using the information in > |params|.", but honestly that looks to me like it just restates the declaration, > so I'd probably omit it entirely. Done. https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:432: if (params.has_custom_image) { On 2016/07/29 18:58:52, Peter Kasting wrote: > On 2016/07/29 15:02:56, Greg Levin wrote: > > Given the change below at 1586, it looks like we can remove has_custom_image, > > and instead use > > if (!fill_image.isNull()) { > > here instead. Do you see any problem with that? > > I think that would work. Done. https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:1582: hover_controller_.ShouldDraw() ? &hover_controller_ : nullptr; On 2016/07/29 18:58:52, Peter Kasting wrote: > Nit: Looking at the original CL, I see why you didn't put the ShouldDraw() call > here in the callee. But I'm not sure why you didn't put |canvas| or |hc| into > |params|. Seems like those are parameters too. > > I know one reason not to put |canvas| in, which is that back in your original > CL, |canvas| is the only thing changing on subsequent calls from TabImageSource. > So if you omit it, and you add a constructor for PaintBackgroundParams, you > could use that constructor to initialize TabImageSource::params_, and declare it > const. (Which would be nice.) But you could still have the hover controller be > a param and have that work. Added constructor, will declare params_ const in TabImageSource in other CL. > Given that, I suggest making the hover controller part of the params, and adding > a constructor for the params struct, which you can use here too. I don't think > we lose a lot of readability converting the separate named field assignments > into arguments to a constructor call, since the list of argument names is above > in this same file to reference. The hover controller was originally in params, and was removed as per explicit request by oshima@ (see first comment on Left): https://codereview.chromium.org/2118853002/diff2/60001:80001/chrome/browser/u... As noted, making it a separate argument was a suggestion he made offline. I don't recall what his objection was to having hc in params, as it's always NULL (and never cached) in TabImageSource. As owner, the final call is yours... you want me to put it back in params? https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:1599: ::PaintTabBackgroundUsingFillId(canvas, hc, params); On 2016/07/29 18:58:52, Peter Kasting wrote: > Nit: This no longer takes a fill ID, so the name isn't quite right. I'd > probably call it PaintTabBackgroundUsingParams(). That allows eliminating the > :: too. Done.
LGTM https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2197613002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:1582: hover_controller_.ShouldDraw() ? &hover_controller_ : nullptr; On 2016/08/08 18:01:32, Greg Levin wrote: > On 2016/07/29 18:58:52, Peter Kasting wrote: > > Nit: Looking at the original CL, I see why you didn't put the ShouldDraw() > call > > here in the callee. But I'm not sure why you didn't put |canvas| or |hc| into > > |params|. Seems like those are parameters too. > > > > I know one reason not to put |canvas| in, which is that back in your original > > CL, |canvas| is the only thing changing on subsequent calls from > TabImageSource. > > So if you omit it, and you add a constructor for PaintBackgroundParams, you > > could use that constructor to initialize TabImageSource::params_, and declare > it > > const. (Which would be nice.) But you could still have the hover controller > be > > a param and have that work. > > Added constructor, will declare params_ const in TabImageSource in other CL. > > > Given that, I suggest making the hover controller part of the params, and > adding > > a constructor for the params struct, which you can use here too. I don't > think > > we lose a lot of readability converting the separate named field assignments > > into arguments to a constructor call, since the list of argument names is > above > > in this same file to reference. > > The hover controller was originally in params, and was removed as per explicit > request by oshima@ (see first comment on Left): > > https://codereview.chromium.org/2118853002/diff2/60001:80001/chrome/browser/u... > > As noted, making it a separate argument was a suggestion he made offline. I > don't recall what his objection was to having hc in params, as it's always NULL > (and never cached) in TabImageSource. > > As owner, the final call is yours... you want me to put it back in params? It sounds like the objection was to depending on the full HoverController object instead of just on the data that one can get from the hover controller. (This would matter for e.g. testing if you wanted to test functionality that would otherwise require a non-null hover controller, but you didn't want to have to create a hover controller to do so.) I don't think your current patch does that; it still depends on the hover controller but takes it as a separate arg, which sounds strictly worse to me. So I would either put it back in the params, or I would not pass it at all and find a way to pass the relevant info to the function. (I don't know whether that's terribly feasible, and I wouldn't spend a lot of time and effort trying to do it.) https://codereview.chromium.org/2197613002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2197613002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:124: gfx::Rect offset_rect, |offset_rect| feels like a misleading name here. A point is an offset; the origin of this rect is an offset. But that ignores the purpose of the rect's size. It seems like this isn't just about giving an offset, but defining the whole region to paint in. Probably a more general name like just |rect| would be less misleading. (multiple places) Note that a shorter name would have the side effect of reducing the line-wrapping you noted was added. https://codereview.chromium.org/2197613002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:144: SkColor background_color; Nit: All of these but |fill_image| could be const. That could be const too with some clever code in the initializer list. https://codereview.chromium.org/2197613002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:402: fill_image, offset_rect.x() + offset_rect.width() - g_mask_images.r_width, Nit: Use right() in place of x() + width() https://codereview.chromium.org/2197613002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:418: offset_rect.height() - tab_insets.top() - toolbar_overlap); Nit: Shorter: const int left = g_mask_images.l_width; const int top = tab_insets.top(); offset_rect.Inset(left, top, g_mask_images.r_width, toolbar_overlap); canvas->TileImageInt(fill_image, offset_rect.x(), offset_rect.y(), left, top, offset_rect.width(), offset_rect.height()); (This is all making me think for the millionth time that we should really change TileImageInt()'s signature, and those of the related functions, to take Points and Sizes instead of ints. Then we could just do this:) const gfx::Point dest(g_mask_images.l_width, tab_insets.top()); canvas->TileImageInt(fill_image, offset_rect.origin(), dest, offset_rect.size()); https://codereview.chromium.org/2197613002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1609: width(), height()); Nit: Do you think it's any more readable to do this? This calls out a little more (to me) that we're trying to paint "our tab's bounds, relative to some other point". gfx::Rect rect(GetLocalBounds()); rect.Offset(GetMirroredX() + background_offset_.x(), y_offset);
https://codereview.chromium.org/2197613002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2197613002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:124: gfx::Rect offset_rect, On 2016/08/09 02:52:31, Peter Kasting wrote: > |offset_rect| feels like a misleading name here. A point is an offset; the > origin of this rect is an offset. But that ignores the purpose of the rect's > size. It seems like this isn't just about giving an offset, but defining the > whole region to paint in. Probably a more general name like just |rect| would > be less misleading. (multiple places) > > Note that a shorter name would have the side effect of reducing the > line-wrapping you noted was added. Done. https://codereview.chromium.org/2197613002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:144: SkColor background_color; On 2016/08/09 02:52:31, Peter Kasting wrote: > Nit: All of these but |fill_image| could be const. That could be const too with > some clever code in the initializer list. Done. https://codereview.chromium.org/2197613002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:402: fill_image, offset_rect.x() + offset_rect.width() - g_mask_images.r_width, On 2016/08/09 02:52:31, Peter Kasting wrote: > Nit: Use right() in place of x() + width() Done. https://codereview.chromium.org/2197613002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:418: offset_rect.height() - tab_insets.top() - toolbar_overlap); On 2016/08/09 02:52:31, Peter Kasting wrote: > Nit: Shorter: > > const int left = g_mask_images.l_width; > const int top = tab_insets.top(); > offset_rect.Inset(left, top, g_mask_images.r_width, toolbar_overlap); > canvas->TileImageInt(fill_image, offset_rect.x(), offset_rect.y(), left, top, > offset_rect.width(), offset_rect.height()); The offset_rect -> rect change had already shortened this to 5 lines, so I went with a 4 line version w/o new left & top variables. > (This is all making me think for the millionth time that we should really change > TileImageInt()'s signature, and those of the related functions, to take Points > and Sizes instead of ints. Then we could just do this:) > > const gfx::Point dest(g_mask_images.l_width, tab_insets.top()); > canvas->TileImageInt(fill_image, offset_rect.origin(), dest, > offset_rect.size()); https://codereview.chromium.org/2197613002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1609: width(), height()); On 2016/08/09 02:52:31, Peter Kasting wrote: > Nit: Do you think it's any more readable to do this? This calls out a little > more (to me) that we're trying to paint "our tab's bounds, relative to some > other point". > > gfx::Rect rect(GetLocalBounds()); > rect.Offset(GetMirroredX() + background_offset_.x(), y_offset); Done. https://codereview.chromium.org/2197613002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2197613002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:524: params.rect.height() * scale - 1)); NOTE: Change missed in previous Merge
The CQ bit was checked by glevin@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/2197613002/#ps60001 (title: "Address review 2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make PaintTabBackgroundUsingFillId() non-method BUG=621139 As per off-line request, I've separated part of https://codereview.chromium.org/2118853002/ into this CL. This moves the main PaintTabBackgroundUsingFillId() implementation into a non-method in anonymous namespace. This CL contains no functional changes, and is intended to make the functional changes in the original CL easier to see. ========== to ========== Make PaintTabBackgroundUsingFillId() non-method BUG=621139 As per off-line request, I've separated part of https://codereview.chromium.org/2118853002/ into this CL. This moves the main PaintTabBackgroundUsingFillId() implementation into a non-method in anonymous namespace. This CL contains no functional changes, and is intended to make the functional changes in the original CL easier to see. ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Make PaintTabBackgroundUsingFillId() non-method BUG=621139 As per off-line request, I've separated part of https://codereview.chromium.org/2118853002/ into this CL. This moves the main PaintTabBackgroundUsingFillId() implementation into a non-method in anonymous namespace. This CL contains no functional changes, and is intended to make the functional changes in the original CL easier to see. ========== to ========== Make PaintTabBackgroundUsingFillId() non-method BUG=621139 As per off-line request, I've separated part of https://codereview.chromium.org/2118853002/ into this CL. This moves the main PaintTabBackgroundUsingFillId() implementation into a non-method in anonymous namespace. This CL contains no functional changes, and is intended to make the functional changes in the original CL easier to see. Committed: https://crrev.com/4c7ae812a813610568c15c2ed7b24cc4e605646f Cr-Commit-Position: refs/heads/master@{#410749} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4c7ae812a813610568c15c2ed7b24cc4e605646f Cr-Commit-Position: refs/heads/master@{#410749} |
