Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
121223002:
DevTools: Change paint rectangles' colors in compositor to shades of green. (Closed)
DescriptionChange paint rectangles' colors in compositor to shades of green.
Review URL: https://codereview.chromium.org/121223002
BUG=324535
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251053
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixed code style. #
Total comments: 3
Patch Set 3 : Made refactoring. Moved cycling logic to HUD code. #Patch Set 4 : Small refactorings. #
Total comments: 1
Patch Set 5 : Refactored cycling logic. #
Total comments: 2
Patch Set 6 : Made cycling counter a member of class. #Patch Set 7 : Made colors closer in hue space. #
Total comments: 3
Patch Set 8 : Fixed small issues. #Patch Set 9 : Removed 'mutable' modifier. #
Messages
Total messages: 33 (0 generated)
Overall looks good, but please fix style nits. +Nat for owners. https://codereview.chromium.org/121223002/diff/1/cc/debug/debug_colors.cc File cc/debug/debug_colors.cc (right): https://codereview.chromium.org/121223002/diff/1/cc/debug/debug_colors.cc#new... cc/debug/debug_colors.cc:16: static int currentPaintRectColor = 2; please use s_ on statics. https://codereview.chromium.org/121223002/diff/1/cc/debug/debug_colors.cc#new... cc/debug/debug_colors.cc:177: paintRectColors[currentPaintRectColor][0], Line continuation indent is 4 spaces.
Is there a bug that explains why we're making this change? If not, can it at least go into the CL description and the review?
On 2013/12/27 09:50:10, tomhudson wrote: > Is there a bug that explains why we're making this change? If not, can it at > least go into the CL description and the review? The bug is here: https://code.google.com/p/chromium/issues/detail?id=324535
Tom, besides the idea that we want to cycle colors (as we used to do for non-composited code path in InspectorPageAgent) to make continuous repaints more evident, we're changing paint rectangles to green everywhere to make it consistent with green being the color of paint in Timeline.
https://codereview.chromium.org/121223002/diff/70001/cc/debug/debug_colors.cc File cc/debug/debug_colors.cc (right): https://codereview.chromium.org/121223002/diff/70001/cc/debug/debug_colors.cc... cc/debug/debug_colors.cc:11: static int s_paintRectColors[][3] = { these should actually be const static int kPaintRectColors const static int kCurrentPaintRectColor since they are compile-time constants. https://codereview.chromium.org/121223002/diff/70001/cc/debug/debug_colors.cc... cc/debug/debug_colors.cc:16: static int s_currentPaintRectColor = 2; s_current_paint_rect_color https://codereview.chromium.org/121223002/diff/70001/cc/debug/debug_colors.cc... cc/debug/debug_colors.cc:175: s_currentPaintRectColor = (s_currentPaintRectColor + 1) % 3; While this happens to work for now, it's a fragile way to do this. Can you instead expose 3 colors here, and have the HUD layer cycle through them?
lgtm on architecture and change-being-the-right-thing side. Dana should give a final lg for style ec
On 2014/01/06 20:38:27, danakj wrote: > https://codereview.chromium.org/121223002/diff/70001/cc/debug/debug_colors.cc > File cc/debug/debug_colors.cc (right): > > https://codereview.chromium.org/121223002/diff/70001/cc/debug/debug_colors.cc... > cc/debug/debug_colors.cc:11: static int s_paintRectColors[][3] = { > these should actually be > > const static int kPaintRectColors > const static int kCurrentPaintRectColor > > since they are compile-time constants. > > https://codereview.chromium.org/121223002/diff/70001/cc/debug/debug_colors.cc... > cc/debug/debug_colors.cc:16: static int s_currentPaintRectColor = 2; > s_current_paint_rect_color > > https://codereview.chromium.org/121223002/diff/70001/cc/debug/debug_colors.cc... > cc/debug/debug_colors.cc:175: s_currentPaintRectColor = (s_currentPaintRectColor > + 1) % 3; > While this happens to work for now, it's a fragile way to do this. Can you > instead expose 3 colors here, and have the HUD layer cycle through them?
> https://codereview.chromium.org/121223002/diff/70001/cc/debug/debug_colors.cc... > > cc/debug/debug_colors.cc:175: s_currentPaintRectColor = > (s_currentPaintRectColor > > + 1) % 3; > > While this happens to work for now, it's a fragile way to do this. Can you > > instead expose 3 colors here, and have the HUD layer cycle through them? er sizeof might be sufficient here. sizeof the paint colors array divided by one color for instance
On Wed, Jan 8, 2014 at 1:07 PM, <nduca@chromium.org> wrote: > > https://codereview.chromium.org/121223002/diff/70001/cc/ > debug/debug_colors.cc#newcode175 > >> > cc/debug/debug_colors.cc:175: s_currentPaintRectColor = >> (s_currentPaintRectColor >> > + 1) % 3; >> > While this happens to work for now, it's a fragile way to do this. Can >> you >> > instead expose 3 colors here, and have the HUD layer cycle through them? >> > > er sizeof might be sufficient here. sizeof the paint colors array divided > by one > color for instance > arraysize() would definitely be better than just "3". I'm mostly :Y about the fact that if you call the function twice you get different results back, I'd prefer cycling logic to live in the hud code and keep this code as just a list of colors to choose from. > https://codereview.chromium.org/121223002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
actually that does make sense dana. all the paint rects from the same frame should get the same color.
https://codereview.chromium.org/121223002/diff/210001/cc/debug/debug_colors.cc File cc/debug/debug_colors.cc (right): https://codereview.chromium.org/121223002/diff/210001/cc/debug/debug_colors.c... cc/debug/debug_colors.cc:198: s_currentPaintRectColor = Can you move this variable to be a member of the HUDLayerImpl, and make this file expose the array of colors instead of one at a time. Then HUDLayerImpl can just grab the color it wants to use directly.
@danakj I think this way is slightly better than your suggestion. What do you think?
On 2014/01/30 12:21:07, malch1 wrote: > @danakj I think this way is slightly better than your suggestion. What do you > think? Sorry if my comment sounded offensive. Didn't mean to be =)
Sure, but one question https://codereview.chromium.org/121223002/diff/220001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/121223002/diff/220001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:594: static int s_currentPaintRectColor = 0; any reason why this is a static variable instead of just a member variable on the class?
On 2014/01/30 20:20:37, danakj wrote: > Sure, but one question > > https://codereview.chromium.org/121223002/diff/220001/cc/layers/heads_up_disp... > File cc/layers/heads_up_display_layer_impl.cc (right): > > https://codereview.chromium.org/121223002/diff/220001/cc/layers/heads_up_disp... > cc/layers/heads_up_display_layer_impl.cc:594: static int s_currentPaintRectColor > = 0; > any reason why this is a static variable instead of just a member variable on > the class? It's just used in one place only. So I made it local inside function - it has minimal scope.
On Fri, Jan 31, 2014 at 4:08 AM, <malch@google.com> wrote: > On 2014/01/30 20:20:37, danakj wrote: > >> Sure, but one question >> > > > https://codereview.chromium.org/121223002/diff/220001/cc/ > layers/heads_up_display_layer_impl.cc > >> File cc/layers/heads_up_display_layer_impl.cc (right): >> > > > https://codereview.chromium.org/121223002/diff/220001/cc/ > layers/heads_up_display_layer_impl.cc#newcode594 > >> cc/layers/heads_up_display_layer_impl.cc:594: static int >> > s_currentPaintRectColor > >> = 0; >> any reason why this is a static variable instead of just a member >> variable on >> the class? >> > > It's just used in one place only. So I made it local inside function - it > has > minimal scope. > I see, it's just not a pattern we follow elsewhere in the code. Being a static, multiple compositors in the same renderer process will end up sharing the variable. Can you move it to be a member variable instead? To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sure, will do it. On Jan 31, 2014 8:00 PM, "Dana Jansens" <danakj@chromium.org> wrote: > On Fri, Jan 31, 2014 at 4:08 AM, <malch@google.com> wrote: > >> On 2014/01/30 20:20:37, danakj wrote: >> >>> Sure, but one question >>> >> >> >> https://codereview.chromium.org/121223002/diff/220001/cc/ >> layers/heads_up_display_layer_impl.cc >> >>> File cc/layers/heads_up_display_layer_impl.cc (right): >>> >> >> >> https://codereview.chromium.org/121223002/diff/220001/cc/ >> layers/heads_up_display_layer_impl.cc#newcode594 >> >>> cc/layers/heads_up_display_layer_impl.cc:594: static int >>> >> s_currentPaintRectColor >> >>> = 0; >>> any reason why this is a static variable instead of just a member >>> variable on >>> the class? >>> >> >> It's just used in one place only. So I made it local inside function - it >> has >> minimal scope. >> > > I see, it's just not a pattern we follow elsewhere in the code. Being a > static, multiple compositors in the same renderer process will end up > sharing the variable. Can you move it to be a member variable instead? > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/121223002/diff/220001/cc/debug/debug_colors.cc File cc/debug/debug_colors.cc (right): https://codereview.chromium.org/121223002/diff/220001/cc/debug/debug_colors.c... cc/debug/debug_colors.cc:13: static const int kPaintRectColors[][3] = {{0, 95, 0}, {0, 175, 0}, {0, 255, 0}}; please make the colors closer together in hue space. This amount of variation is too large. Perhaps 255, 200, 175.
On 2014/01/31 18:15:50, nduca wrote: > https://codereview.chromium.org/121223002/diff/220001/cc/debug/debug_colors.cc > File cc/debug/debug_colors.cc (right): > > https://codereview.chromium.org/121223002/diff/220001/cc/debug/debug_colors.c... > cc/debug/debug_colors.cc:13: static const int kPaintRectColors[][3] = {{0, 95, > 0}, {0, 175, 0}, {0, 255, 0}}; > please make the colors closer together in hue space. This amount of variation is > too large. Perhaps 255, 200, 175. I guess we better keep colors consistent between cc and DevTools, so adding Pavel for his UX opinion. I played with this locally a bit and found the darkest green in original a bit too dark. Nat's option is fine, though 170 and 200 are a bit too close. 155 - 195 - 235 works fine for me.
Made cycling variable a member of class. Still waiting for a decision on colors.
Changed colors according to caseq's suggestion.
On 2014/02/03 13:51:41, malch1 wrote: > Changed colors according to caseq's suggestion. @danakj @nduca Gentle ping.
Thanks, couple more questions https://codereview.chromium.org/121223002/diff/310001/cc/debug/debug_colors.cc File cc/debug/debug_colors.cc (right): https://codereview.chromium.org/121223002/diff/310001/cc/debug/debug_colors.c... cc/debug/debug_colors.cc:13: static const int kPaintRectColors[][3] = {{0, 155, 0}, {0, 195, 0}, nit: can you move this down just above the 2 methods that use it? (maybe just under the comment on line 179?) https://codereview.chromium.org/121223002/diff/310001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.cc (right): https://codereview.chromium.org/121223002/diff/310001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.cc:595: // static int s_currentPaintRectColor = 0; remove this https://codereview.chromium.org/121223002/diff/310001/cc/layers/heads_up_disp... File cc/layers/heads_up_display_layer_impl.h (right): https://codereview.chromium.org/121223002/diff/310001/cc/layers/heads_up_disp... cc/layers/heads_up_display_layer_impl.h:122: mutable int current_paint_rect_color_; why mutable?
Done. It's mutable because function that uses it is const.
On 2014/02/11 06:48:17, malch wrote: > Done. It's mutable because function that uses it is const. Ah, let's just make the method not const then? Might affect a couple other methods, but if they're not const anymore, they're not const.
@nduca Okay, done.
LGTM
The CQ bit was checked by caseq@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/malch@chromium.org/121223002/440001
Message was sent while issue was closed.
Change committed as 251053 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
