[Mac][Material Design] Rework how location bar shadow is drawn.

chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm

Index: chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm
diff --git a/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm b/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm
index 36b9a024ad561c5de3a1eac6b1a9db6e8777a9e6..3156a9cbb79bfcc3b30f631f90f6c41417f8c24a 100644
--- a/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm
+++ b/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm
@@ -22,8 +22,66 @@
 
 namespace {
 const CGFloat kAnimationDuration = 0.2;
+const CGFloat kShadowInset = 3;
+const CGFloat kShadowColor = 69 / 255.;
+
+}
+
+// A view that draws a 1px shadow line beneath the autocomplete textfield.
+@interface AutocompleteTextFieldShadowView : NSView {
+  AutocompleteTextField* textField_;  // weak

[tapted, 2016/04/27 00:55:32]

nit: @private before this? Also, I'd go for "// Weak. Owns this."

[shrike, 2016/04/27 17:00:21]

Done.

+}
+@end
+
+@implementation AutocompleteTextFieldShadowView
+
+- (instancetype)initWithTextField:(AutocompleteTextField*)aTextField {

[tapted, 2016/04/27 00:55:32]

this should have a prototype in @interface to say it's part of the "public" API.
I usually redeclare the private stuff too in a () category, but I guess it
doesn't really add much here. (we could move the @implementation to the end of
the file to be more strict, but I'm indifferent to that)

[shrike, 2016/04/27 17:00:22]

Done.

+  self = [self initWithFrame:NSZeroRect];

[tapted, 2016/04/27 00:55:32]

if ((self = ..))

[shrike, 2016/04/27 17:00:22]

Done. I was never in the habit of doing this check. Without it, what happens
when I assign a value to the textField_ ivar?

[tapted, 2016/04/28 00:41:53]

Interesting question -I haven't thought about it much outside of "that's how you
write an ObjC initializer". I guess  it would depend on what the superclass
initializer does. If I was writing a class and wanted to be rude enough to
return `nil` from my initializer, I think I'd also have to call [self release],
to balance the alloc, and since returning `nil` means I'm not giving anything
else an opportunity to do it. So that means assigning to fields could be writing
to deallocated memory.

That's what Apple's example at
https://developer.apple.com/library/ios/documentation/General/Conceptual/Coco...
does

But of course a superclass calling [self release] would probably start at the
concrete type's dealloc implementation (unless the objc has some obscure tricks
up its sleeve), and we don't really write our deallocs to be robust against
teardown before the initializer has completed. (maybe we should..)

+  textField_ = aTextField;
+  [[NSNotificationCenter defaultCenter]
+       addObserver:self
+          selector:@selector(adjustFrame)
+              name:NSViewFrameDidChangeNotification
+            object:textField_];
+  return self;
+}
+
+- (void)dealloc {
+  [[NSNotificationCenter defaultCenter] removeObserver:self];
+  [super dealloc];
+}
+
+- (void)adjustFrame {
+  // Make the shadow view 1pt tall and slightly inset from the edges of the
+  // autocomplete textfield.
+  NSRect frame = [textField_ frame];
+  frame.origin.x += kShadowInset;
+  frame.size.width -= kShadowInset * 2;
+  frame.origin.y -= 1;
+  frame.size.height = 1;
+  [self setFrame:frame];
 }
 
+- (void)viewDidMoveToWindow {
+  [self adjustFrame];

[tapted, 2016/04/27 00:55:32]

if ([self window]) ?

perhaps in adjustFrame instead, with an early return, so that it's not updated
via the notification when out of the view hierarchy.

otherwise there's a call from textField_'s dealloc (I think). I guess nothing
breaks right now, but it stops people wondering why we'd want to update things
when being removed from the view hierarchy.

It might also be why you needed the [shadowView_ removeFromSuperview]; in the
dealloc - it might become unnecessary, but I think it's good to keep it.

[shrike, 2016/04/27 17:00:21]

Done. [shadowView_ removeFromSuperview] was added because I was concerned about
the case where the textfield gets removed from the view hierarchy before the
shadow view, in which case the shadow view might end up in adjustFrame and crash
on its reference to the textfield. At least by doing that the shadow view
will/should get released as part of the textfield's deallocation. But your
suggestion of checking for a nil window makes guarding against this case a lot
more solid. 

+}
+
+- (void)drawRect:(NSRect)rect {
+  // Don't draw anything on a Retina display because on Retina there's room
+  // for the shadow just beneath the autocomplete textfield path stroke. Why
+  // even add this view? If the user drags the Incognito window between Retina
+  // and non-Retina screens there would have to be logic to add and remove the
+  // view. It's easier just to always add it for Incognito mode and draw
+  // nothing into it.
+  if ([self cr_lineWidth] < 1) {
+    return;
+  }
+  [[AutocompleteTextField shadowColor] set];
+  NSRectFill(rect);
+}
+
+@end
+
 @implementation AutocompleteTextField
 
 @synthesize observer = observer_;
@@ -32,8 +90,13 @@ const CGFloat kAnimationDuration = 0.2;
   return [AutocompleteTextFieldCell class];
 }
 
++ (NSColor*)shadowColor {
+  return [NSColor colorWithGenericGamma22White:kShadowColor alpha:1];
+}
+
 - (void)dealloc {
   [[NSNotificationCenter defaultCenter] removeObserver:self];
+  [shadowView_ removeFromSuperview];
   [super dealloc];
 }
 
@@ -379,15 +442,23 @@ const CGFloat kAnimationDuration = 0.2;
 
 - (void)updateColorsToMatchTheme {
   if (![[self window] inIncognitoMode]) {
+    [shadowView_ removeFromSuperview];
     return;
   }
 
+  // Add 1px shadow below the autocomplete textfield.
+  if (!shadowView_.get()) {
+    shadowView_.reset(
+        [[AutocompleteTextFieldShadowView alloc] initWithTextField:self]);
+  }
+  [[self superview] addSubview:shadowView_];
+
   // Invert the textfield's colors when Material Design and Incognito and not
   // a custom theme.
   bool inDarkMode = [[self window] inIncognitoModeWithSystemTheme];
   [self setBackgroundColor:
-      inDarkMode ? [NSColor colorWithCalibratedWhite:115 / 255. alpha:1]
-                 : [NSColor whiteColor]];
+      inDarkMode ? [NSColor colorWithGenericGamma22White:115 / 255. alpha:1]
+                          : [NSColor colorWithGenericGamma22White:1 alpha:1]];

[tapted, 2016/04/27 00:55:32]

hm, I've always assumed "white" \implies{} "max color" and should look the same
in any colorspace. That is, the colorspace defines the gamut "curve" for how
numbers [0, 255] map to intensity, but black is always "least intense" and white
is always "most intense".  So I think this can stay as whiteColor. 

OTOH consistency is nice too, but I'd do

  CGFloat white = inDarkMode ? 115 / 255. : 1.0;
  [self setBackgroundColor:[NSColor colorWithGenericGamma22White:white alpha:1];

[shrike, 2016/04/27 17:00:22]

OK. I wasn't sure about whiteColor. I'm still not sure 255 in any color space is
the brightest monitor white but I do believe that 0 is the darkest monitor black
so it follows that it would be similar for white. 

   [self setTextColor:OmniboxViewMac::BaseTextColor(inDarkMode)];
 }