Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1048733004:
Add a Material Design Circular Activity Indicator (Spinner) view for Mac. (Closed)
DescriptionAdd a Material Design Circular Activity Indicator (Spinner) view for Mac.
BUG=471829
Committed: https://crrev.com/b68239b76868fa5aa3497d723c0f2aa4ca25c7ca
Cr-Commit-Position: refs/heads/master@{#323248}
Patch Set 1 #
Total comments: 47
Patch Set 2 : Fix nits and address other comments. #
Total comments: 12
Patch Set 3 : Fix nits and rename class to SpinnerView. #
Total comments: 58
Patch Set 4 : Fix nits. #Patch Set 5 : Fix two more nits. #
Total comments: 25
Patch Set 6 : Fix more nits. #Patch Set 7 : Change to mono-color, rounded endings per UX. #
Messages
Total messages: 21 (4 generated)
shrike@chromium.org changed reviewers: + groby@chromium.org
Please take a look.
Thank you! Coming back with a bit of nit-picking, and a few questions to help me understand. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... File chrome/browser/ui/cocoa/circular_activity_indicator_view.h (right): https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.h:9: #import <QuartzCore/QuartzCore.h> Can you forward-declare @class CAShapeLayer/CAAnimationGroup and include the header in the .mm file? https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.h:15: base::scoped_nsobject<CAAnimationGroup> spinner_animation_; Can we move the ivars to the implementation? https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.h:15: base::scoped_nsobject<CAAnimationGroup> spinner_animation_; General question: I assume we ruled out using ui/gfx/animation.h because we lose GPU acceleration? https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.h:16: CAShapeLayer* shape_layer_; Personal nit: I like a // Weak. comment at the end of unowned/non-refcounted objects. It's optional, but I think it makes things slightly clearer when non-Cocoa people read the code. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... File chrome/browser/ui/cocoa/circular_activity_indicator_view.mm (right): https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:10: #define DEGREES_90 (M_PI / 2) Please don't #define constants. Chromium customarily uses the kXYZ notation in an anonymous namespace - i.e. namespace { ... const CGFloat kArcRadius = 12.5; ... } // namespace https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:22: @implementation CircularActivityIndicatorView nit: I'd call it "SpinnerView" - that's the common term in Chrome for spinning indicators. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:74: // of each cycle, which we achieve by drawing the arc using a (solid) dashed tiny nit: "we achieve" -> Please do clarify who "we" is. (It's a Chrome peculiarity - the comments try to avoid "we", "I', etc. and pinpoint the culprit) https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:75: // line pattern and animating the "phase" property. I assume that's lineDashPhase? https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:108: [animation_values addObject:[NSNumber numberWithFloat:58.6 * scale_factor]]; What is 58.6? https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:123: [animations addObject:first_half_animation]; Out of curiosity: Why not [[[first_half_animation] copy] autorelease] To avoid a bunch of autoreleased objects? Or am I missing something subtle here? (Entirely possible. Coffee is running low ;) https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:180: CGColorRef blueColor = [[NSColor colorWithDeviceRed:62.0 / 255.0 It might be worth defining them as constants in the anon namespace: const SkColor kBlue = SkColorSetRGB(...) ... and then converting to an NSColor here with SkColorToCalibratedNSColor That way, we can share them with Views code. (Assuming somebody writes a Views spinner at some point) https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:210: CGFloat transition_offset = 0.05; Probably worth moving out as a constant. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:242: if ([self window] && ![[self window] isMiniaturized] && ![self isHidden] && Do we need the [self window] check? (I assume it's here for the case where you generate a SpinnerView but don't add it to a window immediately?)
One more on CGColors, and a few comments on the tests. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... File chrome/browser/ui/cocoa/circular_activity_indicator_view.mm (right): https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:181: green:130.0 / 255.0 Sorry, forgot that -CGColor doesn't work before 10.8. So, with SKColors, you want to use CGColorCreateFromSkColor https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... File chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm (right): https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm:10: #import "chrome/browser/ui/cocoa/cocoa_test_helper.h" You probably want ui/gfx/test/ui_cocoa_test_helper.h - it comes with a free NSWindow :) https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm:24: - (BOOL)is_animating Wouldn't @property BOOL is_animating; auto-synthesize? https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm:30: Customarily, unit tests for views have a a fixture that contains a view_ member and call TEST_VIEW(CircularActivityIndicatorViewTest, view_) This runs a standard set of tests for views. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm:34: NSWindow* test_window = If you have a ui::CocoaTest, just call test_window() - it should give you a ready-made window. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm:42: TestCircularActivityIndicatorView* test_view = scoped_nsobject, please. Chromium tries to do most resource management through scoped_XYZ objects, to simplify reference counting. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm:48: nit: I'd kill the empty spaces between changes and new expectations. (I.e. only empty line after EXPECT_XYZ) https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm:68: [test_window orderOut:nil]; No need to orderOut
PTAL https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... File chrome/browser/ui/cocoa/circular_activity_indicator_view.h (right): https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.h:9: #import <QuartzCore/QuartzCore.h> On 2015/03/30 23:28:30, groby wrote: > Can you forward-declare @class CAShapeLayer/CAAnimationGroup and include the > header in the .mm file? Done. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.h:15: base::scoped_nsobject<CAAnimationGroup> spinner_animation_; On 2015/03/30 23:28:30, groby wrote: > General question: I assume we ruled out using ui/gfx/animation.h because we lose > GPU acceleration? This appears to be timer-based animation, which would incur an energy penalty for timer usage, plus incur CPU usage (vs. this GPU-based Core Animation code). https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.h:15: base::scoped_nsobject<CAAnimationGroup> spinner_animation_; On 2015/03/30 23:28:30, groby wrote: > Can we move the ivars to the implementation? This class is patterned after sprite_view. I have made the change, but sprite_view's equivalent code defines them in the .h. Except, the unit tests need is_animating_ exposed. Not sure if I should expose the ivar or add API (is_animating) that will only be used by the unit tests. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.h:16: CAShapeLayer* shape_layer_; On 2015/03/30 23:28:30, groby wrote: > Personal nit: I like a // Weak. comment at the end of unowned/non-refcounted > objects. It's optional, but I think it makes things slightly clearer when > non-Cocoa people read the code. Done. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... File chrome/browser/ui/cocoa/circular_activity_indicator_view.mm (right): https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:10: #define DEGREES_90 (M_PI / 2) On 2015/03/30 23:28:31, groby wrote: > Please don't #define constants. Chromium customarily uses the kXYZ notation in > an anonymous namespace - i.e. > > namespace { > ... > const CGFloat kArcRadius = 12.5; > ... > } // namespace Acknowledged. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:22: @implementation CircularActivityIndicatorView On 2015/03/30 23:28:31, groby wrote: > nit: I'd call it "SpinnerView" - that's the common term in Chrome for spinning > indicators. But that's not what the designers call it (per their design spec.). It doesn't seem like calling it by its correct name can be the wrong choice. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:74: // of each cycle, which we achieve by drawing the arc using a (solid) dashed On 2015/03/30 23:28:31, groby wrote: > tiny nit: "we achieve" -> Please do clarify who "we" is. (It's a Chrome > peculiarity - the comments try to avoid "we", "I', etc. and pinpoint the > culprit) Well, the "we" is actually me, but I didn't figure I could put my name in the source. I don't like using "we" in comments, and in fact I don't do that in my own code. I only used it here because I'm following the lead of the chrome codebase (there' a huge number of instances). I have changed it, but writing that the activity indicator achieves the drawing sounds implausible (the activity indicator didn't actually do anything to achieve its abilities). https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:75: // line pattern and animating the "phase" property. On 2015/03/30 23:28:30, groby wrote: > I assume that's lineDashPhase? Eh. I've changed it, but the APIs that do the actual work don't have anything called "lineDashPhase". https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:108: [animation_values addObject:[NSNumber numberWithFloat:58.6 * scale_factor]]; On 2015/03/30 23:28:30, groby wrote: > What is 58.6? It's a value that's just short of the full arc length of 58.9 - if the animation were allowed to reach the full arc length the arc would have zero length at the end of the animation. I have clarified this in the code and removed this anonymous constant. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:123: [animations addObject:first_half_animation]; On 2015/03/30 23:28:30, groby wrote: > Out of curiosity: Why not [[[first_half_animation] copy] autorelease] > > To avoid a bunch of autoreleased objects? Or am I missing something subtle here? > (Entirely possible. Coffee is running low ;) I was thinking that the Chrome codebase frowned upon autorelease but I see that is not the case. This is my first time coding with the scoped_nsobject mechanism, so I'm really just trying to figure out and do the right thing with object lifetimes. What do you recommend? https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:180: CGColorRef blueColor = [[NSColor colorWithDeviceRed:62.0 / 255.0 On 2015/03/30 23:28:30, groby wrote: > It might be worth defining them as constants in the anon namespace: > > const SkColor kBlue = SkColorSetRGB(...) > ... > > and then converting to an NSColor here with SkColorToCalibratedNSColor > > That way, we can share them with Views code. (Assuming somebody writes a Views > spinner at some point) OK. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:181: green:130.0 / 255.0 On 2015/03/30 23:42:06, groby wrote: > Sorry, forgot that -CGColor doesn't work before 10.8. So, with SKColors, you > want to use CGColorCreateFromSkColor Done. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:210: CGFloat transition_offset = 0.05; On 2015/03/30 23:28:31, groby wrote: > Probably worth moving out as a constant. Done. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:242: if ([self window] && ![[self window] isMiniaturized] && ![self isHidden] && On 2015/03/30 23:28:31, groby wrote: > Do we need the [self window] check? (I assume it's here for the case where you > generate a SpinnerView but don't add it to a window immediately?) It's more for the case where you remove the activity indicator from a window (make sure it's no longer spinning), and if you later add it back (make sure it starts spinning). https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... File chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm (right): https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm:10: #import "chrome/browser/ui/cocoa/cocoa_test_helper.h" On 2015/03/30 23:42:07, groby wrote: > You probably want ui/gfx/test/ui_cocoa_test_helper.h - it comes with a free > NSWindow :) > Acknowledged. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm:24: - (BOOL)is_animating On 2015/03/30 23:42:07, groby wrote: > Wouldn't @property BOOL is_animating; auto-synthesize? I don't think it's declared to do so. In general this class doesn't need to expose is_animating to anything other than the unit tests. Do you want me to add this property to the class? https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm:30: On 2015/03/30 23:42:07, groby wrote: > Customarily, unit tests for views have a a fixture that contains a view_ member > and call > TEST_VIEW(CircularActivityIndicatorViewTest, view_) > > This runs a standard set of tests for views. Acknowledged. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm:34: NSWindow* test_window = On 2015/03/30 23:42:07, groby wrote: > If you have a ui::CocoaTest, just call test_window() - it should give you a > ready-made window. Acknowledged. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm:42: TestCircularActivityIndicatorView* test_view = On 2015/03/30 23:42:07, groby wrote: > scoped_nsobject, please. Chromium tries to do most resource management through > scoped_XYZ objects, to simplify reference counting. Acknowledged. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm:48: On 2015/03/30 23:42:07, groby wrote: > nit: I'd kill the empty spaces between changes and new expectations. (I.e. only > empty line after EXPECT_XYZ) Acknowledged. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm:68: [test_window orderOut:nil]; On 2015/03/30 23:42:07, groby wrote: > No need to orderOut Done.
LGTM w/ nits https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... File chrome/browser/ui/cocoa/circular_activity_indicator_view.mm (right): https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:22: @implementation CircularActivityIndicatorView On 2015/03/31 07:05:53, shrike wrote: > On 2015/03/30 23:28:31, groby wrote: > > nit: I'd call it "SpinnerView" - that's the common term in Chrome for spinning > > indicators. > > But that's not what the designers call it (per their design spec.). It doesn't > seem like calling it by its correct name can be the wrong choice. Then do ignore me. I had no idea that's the official name :) https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:74: // of each cycle, which we achieve by drawing the arc using a (solid) dashed On 2015/03/31 07:05:53, shrike wrote: > On 2015/03/30 23:28:31, groby wrote: > > tiny nit: "we achieve" -> Please do clarify who "we" is. (It's a Chrome > > peculiarity - the comments try to avoid "we", "I', etc. and pinpoint the > > culprit) > > Well, the "we" is actually me, but I didn't figure I could put my name in the > source. > > I don't like using "we" in comments, and in fact I don't do that in my own code. > I only used it here because I'm following the lead of the chrome codebase > (there' a huge number of instances). I have changed it, but writing that the > activity indicator achieves the drawing sounds implausible (the activity > indicator didn't actually do anything to achieve its abilities). There's a huge number of instances because it slipped through code review :) (It's also not officially part of the standard, so it varies by section) I prefer non-we for clarity, but if it leads to contorted passive-voice comments, fine. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:108: [animation_values addObject:[NSNumber numberWithFloat:58.6 * scale_factor]]; On 2015/03/31 07:05:53, shrike wrote: > On 2015/03/30 23:28:30, groby wrote: > > What is 58.6? > > It's a value that's just short of the full arc length of 58.9 - if the animation > were allowed to reach the full arc length the arc would have zero length at the > end of the animation. I have clarified this in the code and removed this > anonymous constant. Awesome, thanks. (The proximity to 57.3 - rad-2-deg - confused the heck out of me :) https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:123: [animations addObject:first_half_animation]; On 2015/03/31 07:05:53, shrike wrote: > On 2015/03/30 23:28:30, groby wrote: > > Out of curiosity: Why not [[[first_half_animation] copy] autorelease] > > > > To avoid a bunch of autoreleased objects? Or am I missing something subtle > here? > > (Entirely possible. Coffee is running low ;) > > I was thinking that the Chrome codebase frowned upon autorelease but I see that > is not the case. This is my first time coding with the scoped_nsobject > mechanism, so I'm really just trying to figure out and do the right thing with > object lifetimes. What do you recommend? We're torn on autorelease :) Leaving this as-is is fine - I was just curious if I was missing something. https://codereview.chromium.org/1048733004/diff/1/chrome/browser/ui/cocoa/cir... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:242: if ([self window] && ![[self window] isMiniaturized] && ![self isHidden] && On 2015/03/31 07:05:53, shrike wrote: > On 2015/03/30 23:28:31, groby wrote: > > Do we need the [self window] check? (I assume it's here for the case where you > > generate a SpinnerView but don't add it to a window immediately?) > > It's more for the case where you remove the activity indicator from a window > (make sure it's no longer spinning), and if you later add it back (make sure it > starts spinning). Acknowledged. https://codereview.chromium.org/1048733004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/circular_activity_indicator_view.h (right): https://codereview.chromium.org/1048733004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/circular_activity_indicator_view.h:16: bool is_animating_; nit: Mark @private, please. https://codereview.chromium.org/1048733004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/circular_activity_indicator_view.mm (right): https://codereview.chromium.org/1048733004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:26: const SkColor kBlue = SkColorSetRGB(66.0, 133.0, 244.0); // blue = #4285f4 Tiny nit: Please end all comments with a period. https://codereview.chromium.org/1048733004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:159: // 360 degrees. Thank you for this explanation, btw - makes it very clear why the code does what it does. https://codereview.chromium.org/1048733004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:266: } else { Question: Can we ever enter here with a spinner that's already animating? If not, we should probably DCHECK that. https://codereview.chromium.org/1048733004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:267: } nit: remove empty else https://codereview.chromium.org/1048733004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm (right): https://codereview.chromium.org/1048733004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm:9: @interface TestCircularActivityIndicatorView : CircularActivityIndicatorView nit: common pattern is to just make it an (ExposedForTesting) category.
https://codereview.chromium.org/1048733004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/circular_activity_indicator_view.h (right): https://codereview.chromium.org/1048733004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/circular_activity_indicator_view.h:16: bool is_animating_; On 2015/03/31 23:07:01, groby wrote: > nit: Mark @private, please. Done. https://codereview.chromium.org/1048733004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/circular_activity_indicator_view.mm (right): https://codereview.chromium.org/1048733004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:26: const SkColor kBlue = SkColorSetRGB(66.0, 133.0, 244.0); // blue = #4285f4 On 2015/03/31 23:07:01, groby wrote: > Tiny nit: Please end all comments with a period. Done. https://codereview.chromium.org/1048733004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:159: // 360 degrees. On 2015/03/31 23:07:01, groby wrote: > Thank you for this explanation, btw - makes it very clear why the code does what > it does. You bet! https://codereview.chromium.org/1048733004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:266: } else { On 2015/03/31 23:07:01, groby wrote: > Question: Can we ever enter here with a spinner that's already animating? If > not, we should probably DCHECK that. Done. https://codereview.chromium.org/1048733004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/circular_activity_indicator_view.mm:267: } On 2015/03/31 23:07:01, groby wrote: > nit: remove empty else Done. https://codereview.chromium.org/1048733004/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm (right): https://codereview.chromium.org/1048733004/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/circular_activity_indicator_view_unittest.mm:9: @interface TestCircularActivityIndicatorView : CircularActivityIndicatorView On 2015/03/31 23:07:01, groby wrote: > nit: common pattern is to just make it an (ExposedForTesting) category. Done.
The CQ bit was checked by shrike@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from groby@chromium.org Link to the patchset: https://codereview.chromium.org/1048733004/#ps40001 (title: "Fix nits and rename class to SpinnerView.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1048733004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b68239b76868fa5aa3497d723c0f2aa4ca25c7ca Cr-Commit-Position: refs/heads/master@{#323248}
Message was sent while issue was closed.
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
Message was sent while issue was closed.
Sorry, there are a lot of style issues here. This CL is not lgtm. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/spinner_view.h (right): https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.h:10: #include "base/mac/scoped_nsobject.h" #import over #include https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.h:12: @class CAAnimationGroup; These forward declares are unnecessary. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.h:16: @private nit: only indent @private 1 space http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Insta... https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.h:17: bool is_animating_; naming: isAnimating_ The rule is if writing an ObjC class, use camelCase, otherwise use under_scores. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/spinner_view.mm (right): https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:5: #import "spinner_view.h" Always use fully-qualified import paths. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:10: #include "base/mac/scoped_nsobject.h" Already included in the header. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:14: const CGFloat k90_Degrees = (M_PI / 2); nit: namespaces are not indended. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:32: @interface SpinnerView() nit: space before ( https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:33: { nit: this goes on the previous line https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:34: base::scoped_nsobject<CAAnimationGroup> spinner_animation_; naming: spinnerAnimation_ https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:35: CAShapeLayer* shape_layer_; // Weak. naming: shapeLayer_ https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:55: - (CALayer *)makeBackingLayer { nit: no space before * https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:60: CGFloat scale_factor = bounds.size.width / kDesign_Width; naming: scaleFactor https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:95: CGFloat scale_factor = bounds.size.width / kDesign_Width; naming: scaleFactor https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:105: NSMutableArray* animation_values = [NSMutableArray array]; naming: animationValues https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:112: NSMutableArray* key_times = [NSMutableArray array]; naming: keyTimes https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:122: base::scoped_nsobject<CAKeyframeAnimation> second_half_animation( naming: secondHalfAnimation https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:141: NSUInteger i; Declare i in the for loop. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:199: CAKeyframeAnimation *color_animation = [CAKeyframeAnimation animation]; naming: colorAnimation https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:199: CAKeyframeAnimation *color_animation = [CAKeyframeAnimation animation]; nit: * goes with the type, not the name https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:311: { nit: goes on previous line https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/spinner_view_unittest.mm (right): https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view_unittest.mm:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: no (c) in new files https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view_unittest.mm:9: @interface SpinnerView(ExposedForTesting) nit: space before ( https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view_unittest.mm:11: - (BOOL)is_animating; naming: isAnimating https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view_unittest.mm:18: { nit: goes on previous line https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view_unittest.mm:26: class SpinnerViewTest : public ui::CocoaTest { namespace should not be indented https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view_unittest.mm:27: public: nit: indent 1 space https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view_unittest.mm:69: } nit: " // namespace" comment at the end https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view_unittest.mm:70: nit: extra blakn ilne
Message was sent while issue was closed.
PTAL https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/spinner_view.h (right): https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.h:10: #include "base/mac/scoped_nsobject.h" On 2015/04/01 16:25:01, Robert Sesek wrote: > #import over #include Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.h:12: @class CAAnimationGroup; On 2015/04/01 16:25:01, Robert Sesek wrote: > These forward declares are unnecessary. Remnants of a previous change. Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.h:16: @private On 2015/04/01 16:25:01, Robert Sesek wrote: > nit: only indent @private 1 space > > http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Insta... OK. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.h:17: bool is_animating_; On 2015/04/01 16:25:01, Robert Sesek wrote: > naming: isAnimating_ > > The rule is if writing an ObjC class, use camelCase, otherwise use under_scores. OK. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/spinner_view.mm (right): https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:5: #import "spinner_view.h" On 2015/04/01 16:25:01, Robert Sesek wrote: > Always use fully-qualified import paths. Sorry about that - not sure how that happened. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:14: const CGFloat k90_Degrees = (M_PI / 2); On 2015/04/01 16:25:02, Robert Sesek wrote: > nit: namespaces are not indended. Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:32: @interface SpinnerView() On 2015/04/01 16:25:02, Robert Sesek wrote: > nit: space before ( Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:33: { On 2015/04/01 16:25:02, Robert Sesek wrote: > nit: this goes on the previous line Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:34: base::scoped_nsobject<CAAnimationGroup> spinner_animation_; On 2015/04/01 16:25:02, Robert Sesek wrote: > naming: spinnerAnimation_ Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:35: CAShapeLayer* shape_layer_; // Weak. On 2015/04/01 16:25:01, Robert Sesek wrote: > naming: shapeLayer_ Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:55: - (CALayer *)makeBackingLayer { On 2015/04/01 16:25:02, Robert Sesek wrote: > nit: no space before * Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:60: CGFloat scale_factor = bounds.size.width / kDesign_Width; On 2015/04/01 16:25:02, Robert Sesek wrote: > naming: scaleFactor Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:95: CGFloat scale_factor = bounds.size.width / kDesign_Width; On 2015/04/01 16:25:02, Robert Sesek wrote: > naming: scaleFactor Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:105: NSMutableArray* animation_values = [NSMutableArray array]; On 2015/04/01 16:25:02, Robert Sesek wrote: > naming: animationValues Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:112: NSMutableArray* key_times = [NSMutableArray array]; On 2015/04/01 16:25:02, Robert Sesek wrote: > naming: keyTimes Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:112: NSMutableArray* key_times = [NSMutableArray array]; On 2015/04/01 16:25:02, Robert Sesek wrote: > naming: keyTimes Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:122: base::scoped_nsobject<CAKeyframeAnimation> second_half_animation( On 2015/04/01 16:25:01, Robert Sesek wrote: > naming: secondHalfAnimation Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:141: NSUInteger i; On 2015/04/01 16:25:02, Robert Sesek wrote: > Declare i in the for loop. Great! Didn't think that was kosher. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:199: CAKeyframeAnimation *color_animation = [CAKeyframeAnimation animation]; On 2015/04/01 16:25:01, Robert Sesek wrote: > naming: colorAnimation Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:199: CAKeyframeAnimation *color_animation = [CAKeyframeAnimation animation]; On 2015/04/01 16:25:02, Robert Sesek wrote: > nit: * goes with the type, not the name Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:311: { On 2015/04/01 16:25:01, Robert Sesek wrote: > nit: goes on previous line Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/spinner_view_unittest.mm (right): https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view_unittest.mm:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/04/01 16:25:02, Robert Sesek wrote: > nit: no (c) in new files More copy/paste fallout. Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view_unittest.mm:9: @interface SpinnerView(ExposedForTesting) On 2015/04/01 16:25:02, Robert Sesek wrote: > nit: space before ( Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view_unittest.mm:11: - (BOOL)is_animating; On 2015/04/01 16:25:02, Robert Sesek wrote: > naming: isAnimating Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view_unittest.mm:18: { On 2015/04/01 16:25:02, Robert Sesek wrote: > nit: goes on previous line Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view_unittest.mm:26: class SpinnerViewTest : public ui::CocoaTest { On 2015/04/01 16:25:02, Robert Sesek wrote: > namespace should not be indented Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view_unittest.mm:27: public: On 2015/04/01 16:25:03, Robert Sesek wrote: > nit: indent 1 space Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view_unittest.mm:69: } On 2015/04/01 16:25:02, Robert Sesek wrote: > nit: " // namespace" comment at the end Done. https://codereview.chromium.org/1048733004/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view_unittest.mm:70: On 2015/04/01 16:25:02, Robert Sesek wrote: > nit: extra blakn ilne Done.
Message was sent while issue was closed.
Looking much more stylish now! https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/spinner_view.h (right): https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.h:12: @interface SpinnerView : NSView { This should have a class comment describing what it does (specifically that it's a Material Design element). https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/spinner_view.mm (right): https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:13: const CGFloat k90_Degrees = (M_PI / 2); We don't usually mix underscores and camel case. Consider naming kDegrees90, kDegrees180, etc. https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:17: const CGFloat kDesign_Width = 28.0; And then here and below, just remove _. https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:33: CAShapeLayer* shapeLayer_; // Weak. nit: two spaces before end-of-line comments https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:64: [shapeLayer_ setLineDashPattern:[NSArray arrayWithObject: Consider using @[ @(kArc_Length * scaleFactor) ] https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:69: base::ScopedCFTypeRef<CGMutablePathRef> shape_path(CGPathCreateMutable()); naming: shapePath https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:77: CALayer* parent_layer = [CALayer layer]; naming: parentLayer https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:91: - (void)initializeAnimation { Move private methods below overrides. Generally the order is: - Public class convenience functions - init/dealloc - Public methods - Overridden methods - viewDidX - Private methods https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:97: base::scoped_nsobject<CAMediaTimingFunction> timing_function( naming: timingFunction https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:110: NSMutableArray* keyTimes = [NSMutableArray array]; Does this need to be mutable, or can you jus construct it with the two objects on lines 111 and 112. https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:178: keyTimes = [NSMutableArray array]; Same comment about mutability. https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:219: keyTimes = [NSMutableArray array]; Same. https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:312: nit: extra blank line https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/spinner_view_unittest.mm (right): https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view_unittest.mm:69: } // namespace nit: two spaces before end-of-line comments
> Looking much more stylish now! :-) PTAL https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/spinner_view.h (right): https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.h:12: @interface SpinnerView : NSView { On 2015/04/02 19:58:35, Robert Sesek wrote: > This should have a class comment describing what it does (specifically that it's > a Material Design element). Done. https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/spinner_view.mm (right): https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:13: const CGFloat k90_Degrees = (M_PI / 2); On 2015/04/02 19:58:35, Robert Sesek wrote: > We don't usually mix underscores and camel case. Consider naming kDegrees90, > kDegrees180, etc. Done. https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:17: const CGFloat kDesign_Width = 28.0; On 2015/04/02 19:58:35, Robert Sesek wrote: > And then here and below, just remove _. Done. https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:33: CAShapeLayer* shapeLayer_; // Weak. On 2015/04/02 19:58:35, Robert Sesek wrote: > nit: two spaces before end-of-line comments Sorry - I put the two spaces on the wrong side of the //. https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:64: [shapeLayer_ setLineDashPattern:[NSArray arrayWithObject: On 2015/04/02 19:58:35, Robert Sesek wrote: > Consider using @[ @(kArc_Length * scaleFactor) ] Sounds good. https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:69: base::ScopedCFTypeRef<CGMutablePathRef> shape_path(CGPathCreateMutable()); On 2015/04/02 19:58:35, Robert Sesek wrote: > naming: shapePath Done. https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:77: CALayer* parent_layer = [CALayer layer]; On 2015/04/02 19:58:35, Robert Sesek wrote: > naming: parentLayer Done. https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:91: - (void)initializeAnimation { On 2015/04/02 19:58:35, Robert Sesek wrote: > Move private methods below overrides. Generally the order is: > > - Public class convenience functions > - init/dealloc > - Public methods > - Overridden methods > - viewDidX > - Private methods Done. https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:97: base::scoped_nsobject<CAMediaTimingFunction> timing_function( On 2015/04/02 19:58:36, Robert Sesek wrote: > naming: timingFunction Done. https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:110: NSMutableArray* keyTimes = [NSMutableArray array]; On 2015/04/02 19:58:35, Robert Sesek wrote: > Does this need to be mutable, or can you jus construct it with the two objects > on lines 111 and 112. OK. https://codereview.chromium.org/1048733004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/spinner_view.mm:312: On 2015/04/02 19:58:35, Robert Sesek wrote: > nit: extra blank line Done.
My apologies for missing all these. C++ desensitization :)
Converted from 4-color to 1-color (blue), and rounded ends vs. square, per UX. PTAL.
On 2015/04/03 18:38:14, shrike wrote: > Converted from 4-color to 1-color (blue), and rounded ends vs. square, per UX. > > PTAL. you should upload a different CL (i.e. create a different git branch and rietveld issue)
Message was sent while issue was closed.
On 2015/04/03 18:38:54, Evan Stade wrote: > On 2015/04/03 18:38:14, shrike wrote: > > Converted from 4-color to 1-color (blue), and rounded ends vs. square, per UX. > > > > PTAL. > > you should upload a different CL (i.e. create a different git branch and > rietveld issue) Follow-up issue is https://codereview.chromium.org/1057393003/. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
