Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1905163002:
[Mac][Material Design] Rework how location bar shadow is drawn. (Closed)
Description[Mac][Material Design] Rework how location bar shadow is drawn.
The location bar in Material Design has a very faint line of shadow
beneath it in Incognito mode. I originally accomplished this by drawing
it within the bounds of the location bar but by borrowing a pixel row I
changed the height of the interior of the location bar, causing items
within the bar to no longer be vertically centered. This change moves
the shadow into a separate view.
R=tapted@chromium.org
BUG=594847
Committed: https://crrev.com/56996c30e8a829fb4f7e4733845717e23f9f1b14
Cr-Commit-Position: refs/heads/master@{#390764}
Patch Set 1 #Patch Set 2 : Fix for Retina screens. #
Total comments: 14
Patch Set 3 : Address comments. #
Total comments: 11
Patch Set 4 : Fix nits. #Patch Set 5 : Fix crasher with installing a theme. #
Messages
Total messages: 27 (10 generated)
PTAL
Description was changed from ========== [Mac][Material Design] Rework how location bar shadow is drawn. The location bar in Material Design has a very faint line of shadow beneath it in Incognito mode. I originally accomplished this by drawing it within the bounds of the location bar but by borrowing a pixel row I changed the height of the interior of the location bar, causing items within the bar to no longer be vertically centered. This change moves the shadow into a separate view. R=avi@chromium.org BUG=594847 ========== to ========== [Mac][Material Design] Rework how location bar shadow is drawn. The location bar in Material Design has a very faint line of shadow beneath it in Incognito mode. I originally accomplished this by drawing it within the bounds of the location bar but by borrowing a pixel row I changed the height of the interior of the location bar, causing items within the bar to no longer be vertically centered. This change moves the shadow into a separate view. R=avi@chromium.org BUG=594847 ==========
shrike@chromium.org changed reviewers: + tapted@chromium.org - avi@chromium.org
avi@ has been reviewing my cls but I see that he's OOO for two weeks. tapted@, PTAL.
https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm (right): https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:29: @interface AutocompleteTextFieldShadowView : NSView { nit: curlies not needed https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:42: if ([self cr_lineWidth] < 1) { optional nit: curlies not needed (but I'd keep a blank line after the `return` https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:45: [[NSColor colorWithCalibratedWhite:69 / 255. alpha:1] set]; hmmmm this should be `colorWithGenericGamma22White` (sRGB is gamma 2.2, I think "calibrated" will use a gamma of 1.8 which is what OSX used before 10.6 -- see https://support.apple.com/en-au/ht3712 or http://crbug.com/254361 ). But there's more `colorWithCalibratedWhite` over in -[AutocompleteTextFieldCell drawWithFrame:], so they should be consistent. I guess it depends how the levels were determined - e.g. are they "fresh" from a recent spec? Or were they revised after a designer looked at them in Canary and said "this is too light". https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:244: // Overridden to adjust the shadow view whenever the textfield's origin changes. I think the typical solution for this under c/b/ui/cocoa is to subscribe to NSViewFrameDidChangeNotification inside the AutocompleteTextFieldShadowView itself Then adjustShadowViewFrame can be declared on AutocompleteTextFieldShadowView as well https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm (right): https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:384: [[NSColor colorWithCalibratedWhite:69 / 255. alpha:1] set]; This `69` needs to be a constant accessible from a header somewhere. I'd maybe go all the way to something like const SkColor kOmniboxSomethingShadowColor = 0xFF454545; But since other stuff is using `colorWithCalibratedWhite` and I'm scared to change it, perhaps static NSColor* OmniboxViewMac::ShadowColor() (there is already an OmniboxViewMac::SuggestTextColor()).. and a `static` OmniboxViewMac::BaseTextColor . bleh - they should both be static)
https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm (right): https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:45: [[NSColor colorWithCalibratedWhite:69 / 255. alpha:1] set]; On 2016/04/22 01:11:46, tapted wrote: > hmmmm this should be `colorWithGenericGamma22White` (sRGB is gamma 2.2, I think > "calibrated" will use a gamma of 1.8 which is what OSX used before 10.6 -- see > https://support.apple.com/en-au/ht3712 or http://crbug.com/254361 ). > > But there's more `colorWithCalibratedWhite` over in -[AutocompleteTextFieldCell > drawWithFrame:], so they should be consistent. I guess it depends how the levels > were determined - e.g. are they "fresh" from a recent spec? Or were they revised > after a designer looked at them in Canary and said "this is too light". I'm confused, because the link you sent says 2.2 is the default gamma that's used by the system as of 10.6 so to me that implies it's not necessary to jump through hoops to get a color with that gamma. There are zero occurrences of colorWithGenericGamma22White in the Chromium. https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:244: // Overridden to adjust the shadow view whenever the textfield's origin changes. On 2016/04/22 01:11:46, tapted wrote: > I think the typical solution for this under c/b/ui/cocoa is to subscribe to > NSViewFrameDidChangeNotification inside the AutocompleteTextFieldShadowView > itself > > Then adjustShadowViewFrame can be declared on AutocompleteTextFieldShadowView as > well OK. I can't remember off the top of my head if it's easy to get to the autocompletion textfield from an arbitrary place in the view hierarchy. If it's not, it means the shadow view having an ivar for the textfield. You are OK with that? https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm (right): https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:384: [[NSColor colorWithCalibratedWhite:69 / 255. alpha:1] set]; On 2016/04/22 01:11:46, tapted wrote: > This `69` needs to be a constant accessible from a header somewhere. > > I'd maybe go all the way to something like > > const SkColor kOmniboxSomethingShadowColor = 0xFF454545; > > But since other stuff is using `colorWithCalibratedWhite` and I'm scared to > change it, perhaps > > static NSColor* OmniboxViewMac::ShadowColor() > > (there is already an OmniboxViewMac::SuggestTextColor()).. and a `static` > OmniboxViewMac::BaseTextColor . bleh - they should both be static) You mention OmniboxViewMac - you aren't saying I need to stick this color in there? This color/shadow is a bit of polish that's only used in the textfield cell. The color does appear (now) in two places so I agree it needs to be defined in one. I think a function makes sense.
https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm (right): https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:45: [[NSColor colorWithCalibratedWhite:69 / 255. alpha:1] set]; On 2016/04/22 02:09:00, shrike wrote: > On 2016/04/22 01:11:46, tapted wrote: > > hmmmm this should be `colorWithGenericGamma22White` (sRGB is gamma 2.2, I > think > > "calibrated" will use a gamma of 1.8 which is what OSX used before 10.6 -- see > > https://support.apple.com/en-au/ht3712 or http://crbug.com/254361 ). > > > > But there's more `colorWithCalibratedWhite` over in > -[AutocompleteTextFieldCell > > drawWithFrame:], so they should be consistent. I guess it depends how the > levels > > were determined - e.g. are they "fresh" from a recent spec? Or were they > revised > > after a designer looked at them in Canary and said "this is too light". > > I'm confused, because the link you sent says 2.2 is the default gamma that's > used by the system as of 10.6 so to me that implies it's not necessary to jump > through hoops to get a color with that gamma. I don't think it's clear what gamma you get with `colorWithCalibratedWhite`, so we shouldn't use it. If a designer gives us a color value, they probably assume sRGB which is gamma of 2.2, so we should ensure we get 2.2. I'm pretty sure you get 1.8 otherwise, which won't match up with the same color value shown in the content area, or on any other platform. There's also a tidbit in the 10.4 AppKit release notes - https://developer.apple.com/library/mac/releasenotes/AppKit/RN-AppKitOlderNot... - on NSColor it says, """ Calibrated NSColors (those created with colorWithCalibratedRed:.., colorWithCalibratedHue:.., or colorWithCalibratedWhite:...) now use Quartz generic color spaces, rather than the "display" (aka "device") color spaces. """ So in 10.4 it switched from "device" AKA "whatever-the-heck-the-user's-screen-is-like" to "generic" which, at the time, was gamma 1.8. Then in 10.6 having the OS using 2.2 means that color values specified with colorWithCalibratedWhite will get converted from a gamma of 1.8 to 2.2, which usually makes them lighter. But, after all that's said, it's probably more important that these color values are consistent with other parts of the cocoa UI than with stuff in the webcontent area. And since we never audited the color values used for the Cocoa UI to see whether they each one should actually have been specified to have an sRGB color space, we can probably ignore this particular case. > There are zero occurrences of > colorWithGenericGamma22White in the Chromium. colorWithGenericGamma22White wasn't in the 10.6 SDK, so that's likely why. But we should be using it - that's why I filed http://crbug.com/254361 :) https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:244: // Overridden to adjust the shadow view whenever the textfield's origin changes. On 2016/04/22 02:09:00, shrike wrote: > On 2016/04/22 01:11:46, tapted wrote: > > I think the typical solution for this under c/b/ui/cocoa is to subscribe to > > NSViewFrameDidChangeNotification inside the AutocompleteTextFieldShadowView > > itself > > > > Then adjustShadowViewFrame can be declared on AutocompleteTextFieldShadowView > as > > well > > OK. I can't remember off the top of my head if it's easy to get to the > autocompletion textfield from an arbitrary place in the view hierarchy. If it's > not, it means the shadow view having an ivar for the textfield. You are OK with > that? Would it work to pass in `self` into new initializer for AutocompleteTextFieldShadowView, when it's initialized inside -[AutocompleteTextField updateColorsToMatchTheme]? https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm (right): https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:384: [[NSColor colorWithCalibratedWhite:69 / 255. alpha:1] set]; On 2016/04/22 02:09:00, shrike wrote: > On 2016/04/22 01:11:46, tapted wrote: > > This `69` needs to be a constant accessible from a header somewhere. > > > > I'd maybe go all the way to something like > > > > const SkColor kOmniboxSomethingShadowColor = 0xFF454545; > > > > But since other stuff is using `colorWithCalibratedWhite` and I'm scared to > > change it, perhaps > > > > static NSColor* OmniboxViewMac::ShadowColor() > > > > (there is already an OmniboxViewMac::SuggestTextColor()).. and a `static` > > OmniboxViewMac::BaseTextColor . bleh - they should both be static) > > You mention OmniboxViewMac - you aren't saying I need to stick this color in > there? This color/shadow is a bit of polish that's only used in the textfield > cell. The color does appear (now) in two places so I agree it needs to be > defined in one. I think a function makes sense. OmniboxViewMac is the C++ wrapper around the AutocompleteTextField, and it already has some utility functions like what we are after. But I'm not really too fussed where the constant is declared, just so long as it's shared.
PTAL https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm (right): https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:29: @interface AutocompleteTextFieldShadowView : NSView { On 2016/04/22 01:11:46, tapted wrote: > nit: curlies not needed Done. https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:42: if ([self cr_lineWidth] < 1) { On 2016/04/22 01:11:46, tapted wrote: > optional nit: curlies not needed (but I'd keep a blank line after the `return` Acknowledged. https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm (right): https://codereview.chromium.org/1905163002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:384: [[NSColor colorWithCalibratedWhite:69 / 255. alpha:1] set]; On 2016/04/22 04:41:34, tapted wrote: > On 2016/04/22 02:09:00, shrike wrote: > > On 2016/04/22 01:11:46, tapted wrote: > > > This `69` needs to be a constant accessible from a header somewhere. > > > > > > I'd maybe go all the way to something like > > > > > > const SkColor kOmniboxSomethingShadowColor = 0xFF454545; > > > > > > But since other stuff is using `colorWithCalibratedWhite` and I'm scared to > > > change it, perhaps > > > > > > static NSColor* OmniboxViewMac::ShadowColor() > > > > > > (there is already an OmniboxViewMac::SuggestTextColor()).. and a `static` > > > OmniboxViewMac::BaseTextColor . bleh - they should both be static) > > > > You mention OmniboxViewMac - you aren't saying I need to stick this color in > > there? This color/shadow is a bit of polish that's only used in the textfield > > cell. The color does appear (now) in two places so I agree it needs to be > > defined in one. I think a function makes sense. > > OmniboxViewMac is the C++ wrapper around the AutocompleteTextField, and it > already has some utility functions like what we are after. But I'm not really > too fussed where the constant is declared, just so long as it's shared. Done.
lgtm - just a bunch of nits. (there's also an R=avi in the CL description, but I think there's a script that fixes that up as part of the CQ process) https://codereview.chromium.org/1905163002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm (right): https://codereview.chromium.org/1905163002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:32: AutocompleteTextField* textField_; // weak nit: @private before this? Also, I'd go for "// Weak. Owns this." https://codereview.chromium.org/1905163002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:38: - (instancetype)initWithTextField:(AutocompleteTextField*)aTextField { 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) https://codereview.chromium.org/1905163002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:39: self = [self initWithFrame:NSZeroRect]; if ((self = ..)) https://codereview.chromium.org/1905163002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:66: [self adjustFrame]; 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. https://codereview.chromium.org/1905163002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:461: : [NSColor colorWithGenericGamma22White:1 alpha:1]]; 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];
https://codereview.chromium.org/1905163002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm (right): https://codereview.chromium.org/1905163002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:32: AutocompleteTextField* textField_; // weak On 2016/04/27 00:55:32, tapted wrote: > nit: @private before this? Also, I'd go for "// Weak. Owns this." Done. https://codereview.chromium.org/1905163002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:38: - (instancetype)initWithTextField:(AutocompleteTextField*)aTextField { On 2016/04/27 00:55:32, tapted wrote: > 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) Done. https://codereview.chromium.org/1905163002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:39: self = [self initWithFrame:NSZeroRect]; On 2016/04/27 00:55:32, tapted wrote: > if ((self = ..)) Done. I was never in the habit of doing this check. Without it, what happens when I assign a value to the textField_ ivar? https://codereview.chromium.org/1905163002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:66: [self adjustFrame]; On 2016/04/27 00:55:32, tapted wrote: > 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. 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. https://codereview.chromium.org/1905163002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:461: : [NSColor colorWithGenericGamma22White:1 alpha:1]]; On 2016/04/27 00:55:32, tapted wrote: > 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]; 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.
The CQ bit was checked by shrike@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/1905163002/#ps60001 (title: "Fix nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1905163002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905163002/60001
Message was sent while issue was closed.
Description was changed from ========== [Mac][Material Design] Rework how location bar shadow is drawn. The location bar in Material Design has a very faint line of shadow beneath it in Incognito mode. I originally accomplished this by drawing it within the bounds of the location bar but by borrowing a pixel row I changed the height of the interior of the location bar, causing items within the bar to no longer be vertically centered. This change moves the shadow into a separate view. R=avi@chromium.org BUG=594847 ========== to ========== [Mac][Material Design] Rework how location bar shadow is drawn. The location bar in Material Design has a very faint line of shadow beneath it in Incognito mode. I originally accomplished this by drawing it within the bounds of the location bar but by borrowing a pixel row I changed the height of the interior of the location bar, causing items within the bar to no longer be vertically centered. This change moves the shadow into a separate view. R=avi@chromium.org BUG=594847 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Mac][Material Design] Rework how location bar shadow is drawn. The location bar in Material Design has a very faint line of shadow beneath it in Incognito mode. I originally accomplished this by drawing it within the bounds of the location bar but by borrowing a pixel row I changed the height of the interior of the location bar, causing items within the bar to no longer be vertically centered. This change moves the shadow into a separate view. R=avi@chromium.org BUG=594847 ========== to ========== [Mac][Material Design] Rework how location bar shadow is drawn. The location bar in Material Design has a very faint line of shadow beneath it in Incognito mode. I originally accomplished this by drawing it within the bounds of the location bar but by borrowing a pixel row I changed the height of the interior of the location bar, causing items within the bar to no longer be vertically centered. This change moves the shadow into a separate view. R=tapted@chromium.org BUG=594847 ==========
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1928783003/ by shrike@chromium.org. The reason for reverting is: Change can cause a crash when installing a theme..
Message was sent while issue was closed.
The code that notifies NSViews of Chrome theme changes loops through the views with a for (NSVIew *nextView in [self subviews]) for loop. This means that you can't add or remove subviews in response to a theme change, otherwise you'll get a fast enumeration crash. To work around this I changed the shadow view to always be present except when the autocomplete textfield leaves a window. PTAL
Message was sent while issue was closed.
lgtm https://codereview.chromium.org/1905163002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm (right): https://codereview.chromium.org/1905163002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:39: self = [self initWithFrame:NSZeroRect]; On 2016/04/27 17:00:22, shrike wrote: > On 2016/04/27 00:55:32, tapted wrote: > > if ((self = ..)) > > Done. I was never in the habit of doing this check. Without it, what happens > when I assign a value to the textField_ ivar? 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..)
The CQ bit was checked by shrike@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1905163002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1905163002/80001
Message was sent while issue was closed.
Description was changed from ========== [Mac][Material Design] Rework how location bar shadow is drawn. The location bar in Material Design has a very faint line of shadow beneath it in Incognito mode. I originally accomplished this by drawing it within the bounds of the location bar but by borrowing a pixel row I changed the height of the interior of the location bar, causing items within the bar to no longer be vertically centered. This change moves the shadow into a separate view. R=tapted@chromium.org BUG=594847 ========== to ========== [Mac][Material Design] Rework how location bar shadow is drawn. The location bar in Material Design has a very faint line of shadow beneath it in Incognito mode. I originally accomplished this by drawing it within the bounds of the location bar but by borrowing a pixel row I changed the height of the interior of the location bar, causing items within the bar to no longer be vertically centered. This change moves the shadow into a separate view. R=tapted@chromium.org BUG=594847 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c1bb0685bfb2257b22548240af9c622402697780 Cr-Commit-Position: refs/heads/master@{#390115}
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/56996c30e8a829fb4f7e4733845717e23f9f1b14 Cr-Commit-Position: refs/heads/master@{#390764}
Message was sent while issue was closed.
Description was changed from ========== [Mac][Material Design] Rework how location bar shadow is drawn. The location bar in Material Design has a very faint line of shadow beneath it in Incognito mode. I originally accomplished this by drawing it within the bounds of the location bar but by borrowing a pixel row I changed the height of the interior of the location bar, causing items within the bar to no longer be vertically centered. This change moves the shadow into a separate view. R=tapted@chromium.org BUG=594847 ========== to ========== [Mac][Material Design] Rework how location bar shadow is drawn. The location bar in Material Design has a very faint line of shadow beneath it in Incognito mode. I originally accomplished this by drawing it within the bounds of the location bar but by borrowing a pixel row I changed the height of the interior of the location bar, causing items within the bar to no longer be vertically centered. This change moves the shadow into a separate view. R=avi@chromium.org BUG=594847 Committed: https://crrev.com/c1bb0685bfb2257b22548240af9c622402697780 Cr-Commit-Position: refs/heads/master@{#390115} ==========
Message was sent while issue was closed.
Description was changed from ========== [Mac][Material Design] Rework how location bar shadow is drawn. The location bar in Material Design has a very faint line of shadow beneath it in Incognito mode. I originally accomplished this by drawing it within the bounds of the location bar but by borrowing a pixel row I changed the height of the interior of the location bar, causing items within the bar to no longer be vertically centered. This change moves the shadow into a separate view. R=avi@chromium.org BUG=594847 Committed: https://crrev.com/c1bb0685bfb2257b22548240af9c622402697780 Cr-Commit-Position: refs/heads/master@{#390115} ========== to ========== [Mac][Material Design] Rework how location bar shadow is drawn. The location bar in Material Design has a very faint line of shadow beneath it in Incognito mode. I originally accomplished this by drawing it within the bounds of the location bar but by borrowing a pixel row I changed the height of the interior of the location bar, causing items within the bar to no longer be vertically centered. This change moves the shadow into a separate view. R=tapted@chromium.org BUG=594847 Committed: https://crrev.com/56996c30e8a829fb4f7e4733845717e23f9f1b14 Cr-Commit-Position: refs/heads/master@{#390764} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
