Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2247493003:
[Mac] Clean up SadTabView and let SadTabController provide its content (Closed)
|
Created:
4 years, 4 months ago by Sidney San Martín Modified:
4 years, 4 months ago CC:
chromium-reviews, Mark Mentovai Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Mac] Clean up SadTabView and let SadTabController provide its content
Prep for making the sad tab's content more dynamic, plus some touch-up
and shuffling of responsibilities.
- Adds methods to `SadTabView` to set the text of each interface element
and the help URL, and makes `SadTabController` responsible for setting
them. `SadTabView` still controls appearance and layout.
- Gives `SadTabView` a delegate to signal clicks. The button is now
private, and tests now use a `WebContentsDelegate` to detect a click
on the help link instead of extending `NSApplication`.
- Removes some uses of `scoped_nsobject` for instance variables that
just need to be weak references.
BUG=623690
Committed: https://crrev.com/7c635ed1edb8cbcb68fe5a0646cb554b8dd32b8a
Cr-Commit-Position: refs/heads/master@{#411932}
Patch Set 1 #
Total comments: 25
Patch Set 2 : Address most of Avi’s first round of comments #Patch Set 3 : Adopt NSMake*, cast message_.cell to NSCell #
Total comments: 4
Patch Set 4 : Use base::mac::ObjCCast #
Messages
Total messages: 22 (10 generated)
The CQ bit was checked by sdy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sdy@chromium.org changed reviewers: + avi@chromium.org
https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tab_contents/sad_tab_controller.mm (right): https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_controller.mm:6: #import "chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.h" Put this down with the other includes. The reason sad_tab_controller.h is alone is because it's the corresponding .h file for this .mm. Other .h files live in the main include block. https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_controller.mm:46: } Doesn't this block go on the interface with a class extension? https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tab_contents/sad_tab_controller_unittest.mm (right): https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_controller_unittest.mm:36: struct ContentsDelegate : public content::WebContentsDelegate { This should be a class. https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm (right): https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:32: NSTextField* ret = [[[NSTextField alloc] initWithFrame:frame] autorelease]; Is it style nowadays to use NSRectFromCGRect or is that passe? (Honestly I don't know modern style here.) https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:61: } Same question; I thought variables for an extension went on the interface. https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:91: message_.cell.wraps = YES; ../../chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:91:19: error: property 'wraps' not found on object of type 'id' message_.cell.wraps = YES; https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:131: message_.frameOrigin = {0, NSMaxY(title_.frame) + kTitleMessageSpacing}; Why this initializer style rather than NSMakePoint?
This addresses the easy ones, need feedback on the others. https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tab_contents/sad_tab_controller.mm (right): https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_controller.mm:6: #import "chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.h" On 2016/08/12 23:06:22, Avi wrote: > Put this down with the other includes. The reason sad_tab_controller.h is alone > is because it's the corresponding .h file for this .mm. Other .h files live in > the main include block. Ah, that makes sense. Done. https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_controller.mm:46: } On 2016/08/12 23:06:22, Avi wrote: > Doesn't this block go on the interface with a class extension? This is the closest I can find to a comment on it: http://stackoverflow.com/a/10407891/84745 It includes this excerpt from The Objective-C Programming Language: > Historically, the interface required declarations of a class’s instance variables, the data structures that are part of each instance of the class. ... Instance variables represent an implementation detail, and should typically not be accessed outside of the class itself. Moreover, you can declare them in the implementation block or synthesize them using declared properties. Typically you should not, therefore, declare instance variables in the public interface and so you should omit the braces. So I think it's recommended by Apple (and meant to be thought of like @synthesize, which also declares ivars in the @implementation section), but I'm not sure if we have a stance on it. https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tab_contents/sad_tab_controller_unittest.mm (right): https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_controller_unittest.mm:36: struct ContentsDelegate : public content::WebContentsDelegate { On 2016/08/12 23:06:22, Avi wrote: > This should be a class. > > https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes Done. https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm (right): https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:32: NSTextField* ret = [[[NSTextField alloc] initWithFrame:frame] autorelease]; On 2016/08/12 23:06:22, Avi wrote: > Is it style nowadays to use NSRectFromCGRect or is that passe? (Honestly I don't > know modern style here.) I'm not sure either, but I should just be taking an NSRect, instead. https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:61: } On 2016/08/12 23:06:22, Avi wrote: > Same question; I thought variables for an extension went on the interface. See above, I think this is the recommended way now externally, but not sure if we have a policy. https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:91: message_.cell.wraps = YES; On 2016/08/12 23:06:22, Avi wrote: > ../../chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:91:19: error: > property 'wraps' not found on object of type 'id' > message_.cell.wraps = YES; Weird. I don't get this, and the cell property is declared as an NSCell. Any theories? https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:131: message_.frameOrigin = {0, NSMaxY(title_.frame) + kTitleMessageSpacing}; On 2016/08/12 23:06:22, Avi wrote: > Why this initializer style rather than NSMakePoint? I used this style because it takes the same arguments in the same order, more concisely, w/o a function call. If you think it's weird, I'm OK to use NSMakePoint instead.
https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tab_contents/sad_tab_controller.mm (right): https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_controller.mm:46: } I'm working from https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Progra... which in the section "Class Extensions Extend the Internal Implementation" has: It’s also possible to use a class extension to add custom instance variables. These are declared inside braces in the class extension interface: @interface XYZPerson () { id _someCustomInstanceVariable; } ... @end https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm (right): https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:91: message_.cell.wraps = YES; On 2016/08/12 23:35:10, Sidney San Martín wrote: > On 2016/08/12 23:06:22, Avi wrote: > > ../../chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:91:19: error: > > property 'wraps' not found on object of type 'id' > > message_.cell.wraps = YES; > > Weird. I don't get this, and the cell property is declared as an NSCell. Any > theories? It is? message_ is an NSTextField, which doesn't have a cell property, and the superclass NSControl has a cell property that is an id. https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:131: message_.frameOrigin = {0, NSMaxY(title_.frame) + kTitleMessageSpacing}; On 2016/08/12 23:35:10, Sidney San Martín wrote: > On 2016/08/12 23:06:22, Avi wrote: > > Why this initializer style rather than NSMakePoint? > > I used this style because it takes the same arguments in the same order, more > concisely, w/o a function call. If you think it's weird, I'm OK to use > NSMakePoint instead. But you don't have a problem with function calls with NSMaxX/Y. Let's stick with the constructor functions.
https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tab_contents/sad_tab_controller.mm (right): https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_controller.mm:46: } On 2016/08/12 23:43:16, Avi wrote: > I'm working from > https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Progra... Turns out the doc is inconsistent. Another section, https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Progra..., says: > If you do need to define your own instance variables without declaring a property, you can add them inside braces at the top of the class interface or implementation, like this: […] I favor the implementation over a class extension because: - @synthesize declares instance variables for properties and can only appear in the implementation. Having other instance variables there keeps them together[1]. - Instance variables end up closer to the code that uses them. - When the class doesn't otherwise need an extension, you don't have to write one. [1]: From what I can tell, support for declaring ivars in the implementation was added to ObjC in the same pass as properties. https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm (right): https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:91: message_.cell.wraps = YES; On 2016/08/12 23:43:16, Avi wrote: > It is? message_ is an NSTextField, which doesn't have a cell property, and the > superclass NSControl has a cell property that is an id. Done. Huh, my NSControl.h declares the cell property like this: @property (nullable, strong) __kindof NSCell *cell; …and I don't get that build error. The trybots agree with your result, though. I added a cast, but I'm not sure how to make my machine act like yours. https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:131: message_.frameOrigin = {0, NSMaxY(title_.frame) + kTitleMessageSpacing}; On 2016/08/12 23:43:16, Avi wrote: > But you don't have a problem with function calls with NSMaxX/Y. Let's stick with > the constructor functions. Done. I think I see those functions as convenience macros. Changing button_.frameOrigin = { containerSize.width - button_.bounds.size.width, message_.frame.origin.y + message_.frame.size.height + kMessageButtonSpacing }; to button_.frameOrigin = { containerSize.width - NSWidth(button_.bounds), NSMaxY(message_.frame) + kMessageButtonSpacing }; increases clarity and gets rid of some repetition, but changing NSSize containerSize{ std::min(size.width - 2 * kTabMargin, kMaxContainerWidth), size.height }; button_.frameOrigin = { containerSize.width - NSWidth(button_.bounds), NSMaxY(message_.frame) + kMessageButtonSpacing }; to NSSize containerSize = NSMakeSize( std::min(size.width - 2 * kTabMargin, kMaxContainerWidth), size.height ); button_.frameOrigin = NSMakePoint( containerSize.width - NSWidth(button_.bounds), NSMaxY(message_.frame) + kMessageButtonSpacing ); doesn't, really. They're all inlined, so it turns out there's no extra cost, but could you help me grok why you prefer the constructors? I feel like there are elements of "it's explicit" and "it feels familiar in the codebase", but I suspect that doesn't totally cover your uneasiness.
Description was changed from ========== [Mac] Clean up sad tab and let SadTabController provide its content Prep for making the sad tab's content more dynamic, plus some touch up and shuffling of responsibilities. - Adds methods to `SadTabView` to set the text of each interface element and the help URL, and makes `SadTabController` responsible for setting them. `SadTabView` still controls appearance and layout. - Gives `SadTabView` a delegate to signal clicks. The button is now private, and tests now use a `WebContentsDelegate` to detect a click on the help link instead of extending `NSApplication`. - Removes some uses of `scoped_nsobject` for instance variables that just need to be weak references. BUG=623690 ========== to ========== [Mac] Clean up SadTabView and let SadTabController provide its content Prep for making the sad tab's content more dynamic, plus some touch-up and shuffling of responsibilities. - Adds methods to `SadTabView` to set the text of each interface element and the help URL, and makes `SadTabController` responsible for setting them. `SadTabView` still controls appearance and layout. - Gives `SadTabView` a delegate to signal clicks. The button is now private, and tests now use a `WebContentsDelegate` to detect a click on the help link instead of extending `NSApplication`. - Removes some uses of `scoped_nsobject` for instance variables that just need to be weak references. BUG=623690 ==========
LGTM with nit. https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tab_contents/sad_tab_controller.mm (right): https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_controller.mm:46: } On 2016/08/14 00:34:32, Sidney San Martín wrote: > On 2016/08/12 23:43:16, Avi wrote: > > I'm working from > > > https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Progra... > > Turns out the doc is inconsistent. Another section, > https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Progra..., > says: > > > If you do need to define your own instance variables without declaring a > property, you can add them inside braces at the top of the class interface or > implementation, like this: […] > > I favor the implementation over a class extension because: > > - @synthesize declares instance variables for properties and can only appear in > the implementation. Having other instance variables there keeps them > together[1]. > > - Instance variables end up closer to the code that uses them. > > - When the class doesn't otherwise need an extension, you don't have to write > one. > > [1]: From what I can tell, support for declaring ivars in the implementation was > added to ObjC in the same pass as properties. Hmmm. Go for it, then. https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm (right): https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:61: } On 2016/08/12 23:35:10, Sidney San Martín wrote: > On 2016/08/12 23:06:22, Avi wrote: > > Same question; I thought variables for an extension went on the interface. > > See above, I think this is the recommended way now externally, but not sure if > we have a policy. /me shrugs. https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:91: message_.cell.wraps = YES; On 2016/08/14 00:34:32, Sidney San Martín wrote: > On 2016/08/12 23:43:16, Avi wrote: > > It is? message_ is an NSTextField, which doesn't have a cell property, and the > > superclass NSControl has a cell property that is an id. > > Done. Huh, my NSControl.h declares the cell property like this: > > @property (nullable, strong) __kindof NSCell *cell; > > …and I don't get that build error. The trybots agree with your result, though. I > added a cast, but I'm not sure how to make my machine act like yours. What version of the SDK are you using? My docs reference is 10.10 I think. In any case, cast away and get it to work. https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:131: message_.frameOrigin = {0, NSMaxY(title_.frame) + kTitleMessageSpacing}; On 2016/08/14 00:34:32, Sidney San Martín wrote: > On 2016/08/12 23:43:16, Avi wrote: > > But you don't have a problem with function calls with NSMaxX/Y. Let's stick > with > > the constructor functions. > > Done. I think I see those functions as convenience macros. Changing > > button_.frameOrigin = { > containerSize.width - button_.bounds.size.width, > message_.frame.origin.y + message_.frame.size.height + > kMessageButtonSpacing > }; > > to > > button_.frameOrigin = { > containerSize.width - NSWidth(button_.bounds), > NSMaxY(message_.frame) + kMessageButtonSpacing > }; > > increases clarity and gets rid of some repetition, but changing > > NSSize containerSize{ > std::min(size.width - 2 * kTabMargin, kMaxContainerWidth), > size.height > }; > button_.frameOrigin = { > containerSize.width - NSWidth(button_.bounds), > NSMaxY(message_.frame) + kMessageButtonSpacing > }; > > to > > NSSize containerSize = NSMakeSize( > std::min(size.width - 2 * kTabMargin, kMaxContainerWidth), > size.height > ); > button_.frameOrigin = NSMakePoint( > containerSize.width - NSWidth(button_.bounds), > NSMaxY(message_.frame) + kMessageButtonSpacing > ); > > doesn't, really. They're all inlined, so it turns out there's no extra cost, but > could you help me grok why you prefer the constructors? I feel like there are > elements of "it's explicit" and "it feels familiar in the codebase", but I > suspect that doesn't totally cover your uneasiness. I'm old school, I guess, or preferring consistency. https://codereview.chromium.org/2247493003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm (right): https://codereview.chromium.org/2247493003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:92: ((NSCell*)message_.cell).wraps = YES; base::mac::ObjCCast
The CQ bit was checked by sdy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org Link to the patchset: https://codereview.chromium.org/2247493003/#ps60001 (title: "Use base::mac::ObjCCast")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Mac] Clean up SadTabView and let SadTabController provide its content Prep for making the sad tab's content more dynamic, plus some touch-up and shuffling of responsibilities. - Adds methods to `SadTabView` to set the text of each interface element and the help URL, and makes `SadTabController` responsible for setting them. `SadTabView` still controls appearance and layout. - Gives `SadTabView` a delegate to signal clicks. The button is now private, and tests now use a `WebContentsDelegate` to detect a click on the help link instead of extending `NSApplication`. - Removes some uses of `scoped_nsobject` for instance variables that just need to be weak references. BUG=623690 ========== to ========== [Mac] Clean up SadTabView and let SadTabController provide its content Prep for making the sad tab's content more dynamic, plus some touch-up and shuffling of responsibilities. - Adds methods to `SadTabView` to set the text of each interface element and the help URL, and makes `SadTabController` responsible for setting them. `SadTabView` still controls appearance and layout. - Gives `SadTabView` a delegate to signal clicks. The button is now private, and tests now use a `WebContentsDelegate` to detect a click on the help link instead of extending `NSApplication`. - Removes some uses of `scoped_nsobject` for instance variables that just need to be weak references. BUG=623690 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Mac] Clean up SadTabView and let SadTabController provide its content Prep for making the sad tab's content more dynamic, plus some touch-up and shuffling of responsibilities. - Adds methods to `SadTabView` to set the text of each interface element and the help URL, and makes `SadTabController` responsible for setting them. `SadTabView` still controls appearance and layout. - Gives `SadTabView` a delegate to signal clicks. The button is now private, and tests now use a `WebContentsDelegate` to detect a click on the help link instead of extending `NSApplication`. - Removes some uses of `scoped_nsobject` for instance variables that just need to be weak references. BUG=623690 ========== to ========== [Mac] Clean up SadTabView and let SadTabController provide its content Prep for making the sad tab's content more dynamic, plus some touch-up and shuffling of responsibilities. - Adds methods to `SadTabView` to set the text of each interface element and the help URL, and makes `SadTabController` responsible for setting them. `SadTabView` still controls appearance and layout. - Gives `SadTabView` a delegate to signal clicks. The button is now private, and tests now use a `WebContentsDelegate` to detect a click on the help link instead of extending `NSApplication`. - Removes some uses of `scoped_nsobject` for instance variables that just need to be weak references. BUG=623690 Committed: https://crrev.com/7c635ed1edb8cbcb68fe5a0646cb554b8dd32b8a Cr-Commit-Position: refs/heads/master@{#411932} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7c635ed1edb8cbcb68fe5a0646cb554b8dd32b8a Cr-Commit-Position: refs/heads/master@{#411932}
Message was sent while issue was closed.
Thanks for all of the quick replies. I definitely wasn't expecting a response over the weekend — let me know if it came across that way. https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... File chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm (right): https://codereview.chromium.org/2247493003/diff/1/chrome/browser/ui/cocoa/tab... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:91: message_.cell.wraps = YES; On 2016/08/14 04:54:28, Avi wrote: > What version of the SDK are you using? My docs reference is 10.10 I think. In > any case, cast away and get it to work. I have 10.11. Should the build system enforce only building against our standard SDK? I could look into that, but not sure if it's desirable. https://codereview.chromium.org/2247493003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm (right): https://codereview.chromium.org/2247493003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:92: ((NSCell*)message_.cell).wraps = YES; On 2016/08/14 04:54:28, Avi wrote: > base::mac::ObjCCast Done. ObjCCast really surprises me though — it returns nil if the object isn't the expected type, and since a message send to nil is a noop it'll hide most wrong casts. ObjCCastStrict just adds a DCHECK. It's essentially dynamic_cast, but extra dangerous. Do you know why we use it so much?
Message was sent while issue was closed.
https://codereview.chromium.org/2247493003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm (right): https://codereview.chromium.org/2247493003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:92: ((NSCell*)message_.cell).wraps = YES; On 2016/08/15 03:40:29, Sidney San Martín wrote: > On 2016/08/14 04:54:28, Avi wrote: > > base::mac::ObjCCast > > Done. ObjCCast really surprises me though — it returns nil if the object isn't > the expected type, and since a message send to nil is a noop it'll hide most > wrong casts. ObjCCastStrict just adds a DCHECK. > > It's essentially dynamic_cast, but extra dangerous. Do you know why we use it so > much? Interesting way of thinking about it. Talk to Mark.
Message was sent while issue was closed.
erikchen@chromium.org changed reviewers: + erikchen@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2247493003/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm (right): https://codereview.chromium.org/2247493003/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/tab_contents/sad_tab_view_cocoa.mm:92: ((NSCell*)message_.cell).wraps = YES; On 2016/08/15 03:43:59, Avi wrote: > On 2016/08/15 03:40:29, Sidney San Martín wrote: > > On 2016/08/14 04:54:28, Avi wrote: > > > base::mac::ObjCCast > > > > Done. ObjCCast really surprises me though — it returns nil if the object isn't > > the expected type, and since a message send to nil is a noop it'll hide most > > wrong casts. ObjCCastStrict just adds a DCHECK. > > > > It's essentially dynamic_cast, but extra dangerous. Do you know why we use it > so > > much? > > Interesting way of thinking about it. Talk to Mark. I think that downcasting is an appropriate use case for dynamic casting. We don't use dynamic cast in C++ because it requires RTTI. So there are two possibilities here: If the cast would have been safe, there is virtually no difference between static and dynamic cast. (The performance difference is negligible). If the cast would be unsafe, then with a static cast, the behavior is not well defined. In this case, there is going to be memory corruption. If we used a dynamic cast, then instead the call to .wraps = YES would become a no-op. Neither outcome is great. For this case, I don't think it matters because the cast should always be safe. ObjCCastStrict seems appropriate, as it adds a DCHECK which will catch early developer errors.
Message was sent while issue was closed.
Description was changed from ========== [Mac] Clean up SadTabView and let SadTabController provide its content Prep for making the sad tab's content more dynamic, plus some touch-up and shuffling of responsibilities. - Adds methods to `SadTabView` to set the text of each interface element and the help URL, and makes `SadTabController` responsible for setting them. `SadTabView` still controls appearance and layout. - Gives `SadTabView` a delegate to signal clicks. The button is now private, and tests now use a `WebContentsDelegate` to detect a click on the help link instead of extending `NSApplication`. - Removes some uses of `scoped_nsobject` for instance variables that just need to be weak references. BUG=623690 Committed: https://crrev.com/7c635ed1edb8cbcb68fe5a0646cb554b8dd32b8a Cr-Commit-Position: refs/heads/master@{#411932} ========== to ========== [Mac] Clean up SadTabView and let SadTabController provide its content Prep for making the sad tab's content more dynamic, plus some touch-up and shuffling of responsibilities. - Adds methods to `SadTabView` to set the text of each interface element and the help URL, and makes `SadTabController` responsible for setting them. `SadTabView` still controls appearance and layout. - Gives `SadTabView` a delegate to signal clicks. The button is now private, and tests now use a `WebContentsDelegate` to detect a click on the help link instead of extending `NSApplication`. - Removes some uses of `scoped_nsobject` for instance variables that just need to be weak references. BUG=623690 Committed: https://crrev.com/7c635ed1edb8cbcb68fe5a0646cb554b8dd32b8a Cr-Commit-Position: refs/heads/master@{#411932} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
