Mac: Support 10.10's -[NSViewController viewDidLoad] for BookmarkBarController

Mac: Support 10.10's -[NSViewController viewDidLoad] for BookmarkBarController

This is to prevent a crash which appears to be triggered from an
unexpected codepath during NIB loading.

After linking to the 10.10 SDK, a similar crash during NIB loading for
the ToolbarController began. This was fixed by supporting initialization
via the newly introduced -[NSViewController viewDidLoad] on 10.10, and
via [NSViewController awakeFromNib] on older OSes. However, crash logs
suggest that the fault moved on to the next NIB load, in
BookmarkBarController, which happens just after ToolbarController in
-[BrowserWindowController initWithBrowser:..].

To fix, apply a similar fix to the one for BookmarkBarController in
r346604.

BUG=528136
Committed: https://crrev.com/46ce5e6bc615a0f23207130ea2a06bff589ad2c0
Cr-Commit-Position: refs/heads/master@{#350264}

[tapted, 2015-09-21 06:26:43]

tapted@chromium.org changed reviewers:
+ rsesek@chromium.org

[tapted, 2015-09-21 12:24:03]

Hi Robert, please take a look

https://codereview.chromium.org/1356023002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (left):

https://codereview.chromium.org/1356023002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:227: -
(BOOL)setBookmarkButtonVisibility;
(this isn't defined, and became an error when the category name was removed, but
I just noticed that change looks out of place now that the category adding
viewDidLoad got moved to sdk_forward_declarations :/. Happy to revert these bits
and put them in a follow-up, or leave them in this CL [forgot to send this CL
out before leaving work])

[Robert Sesek, 2015-09-21 22:04:01]

Do you expect we'll have more of these to fix one-off? I was just contemplating
whether we should do that, or maybe have a helper class that swizzles
-awakeFromNib and -viewDidLoad and calls like -chromeViewDidLoad appropriately.
But that could be uglier than just doing this.

https://codereview.chromium.org/1356023002/diff/20001/base/mac/sdk_forward_de...
File base/mac/sdk_forward_declarations.h (right):

https://codereview.chromium.org/1356023002/diff/20001/base/mac/sdk_forward_de...
base/mac/sdk_forward_declarations.h:494: @interface NSViewController
(YosemiteSDK)
We build with the 10.10 SDK now, so is this necessary?

[tapted, 2015-09-21 23:55:16]

On 2015/09/21 22:04:01, Robert Sesek wrote:
> Do you expect we'll have more of these to fix one-off? I was just
contemplating
> whether we should do that, or maybe have a helper class that swizzles
> -awakeFromNib and -viewDidLoad and calls like -chromeViewDidLoad
appropriately.
> But that could be uglier than just doing this.

I'm still hopeful of getting more insight into the crashes. This one is
sufficiently different that I'm not sure the crashes will go away :/. For
ToolbarController the crashes suggested a data member that never got
initialized, for BookmarkBarController the crashes suggest it's getting
instantiated with a vanilla NSView rather BookmarkBarView. However, both of
these are consistent with an incorrect initialization path. But another
difference is that BookmarkBarView calls [self view] from its initializer --
that might be tripping something up.

I looked closely at -[BrowserWindowController initWithBrowser:..]. The next
NSViewController in the queue would be InfoBarContainerController. However, it
doesn't override awakeFromNib, or even have a nib - it initializes the base with
[super initWithNibName:nil bundle:nil], which got updated treatment in 10.10 to
autoload a NIB file with the same name as the controller. But the documentation
isn't clear about how NSViewController should be initialized when there is no
nib. It says:

"""
Prior to OS X v10.10, a typical usage pattern for loading a nib file was to
subclass NSViewController and override its loadView method to call [super
loadView]. But in OS X v10.10 and later, the loadView method automatically looks
for a nib file with the same name as the view controller. To take advantage of
this behavior, name a nib file after its corresponding view controller and pass
nil to both parameters of the initWithNibName:bundle: method.
...
Prior to OS X v10.10, the loadView method did not provide well-defined behavior
if the nibName property’s value was nil. In OS X v10.10 and later, however, you
get correct behavior without specifying a nib name as long as the nib file’s
name is the same as that of the view controller. For example, if you have a view
controller subclass called MyViewController and a nib file with the same name,
you can employ the convenient initialization pattern [[MyViewController alloc]
init].
"""

(so is it [super initWithNibName:nil bundle:nil] or [super init]?)

Then, if the crashes are isolated to -[BrowserWindowController
initWithBrowser:..] this might *actually* be the last one - I don't see later
NIB loads, but a "child" NIB load is still a possibility.

But then it doesn't really make sense why it would be isolated to
BrowserWindowController - so I'm hoping there might be clarity on that if the
crashes move elsewhere. Or if they disappear (i.e .why is
BrowserWindowController special?).

But the prior fix for ToolbarController seems solid -- That is, I don't think
there's any way for the fix to have not done its job and not still crash (i.e.
I'm convinced "no crash" \implies{} "initialized correctly")


So.. all of that is a long-winded way of saying "maybe this is all it needs but
I'm not sure".

https://codereview.chromium.org/1356023002/diff/20001/base/mac/sdk_forward_de...
File base/mac/sdk_forward_declarations.h (right):

https://codereview.chromium.org/1356023002/diff/20001/base/mac/sdk_forward_de...
base/mac/sdk_forward_declarations.h:494: @interface NSViewController
(YosemiteSDK)
On 2015/09/21 22:04:01, Robert Sesek wrote:
> We build with the 10.10 SDK now, so is this necessary?

Yup. It was silent on my checkout still linking to 10.9, but it will get a
partial availability warning when linking against 10.10 or later - 

../../chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:257:9: error:
'viewDidLoad' is partial: introduced in OS X 10.10
[-Werror,-Wpartial-availability]
  [self viewDidLoad];
        ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSViewController.h:103:1:
note: 'viewDidLoad' has been explicitly marked partial here
- (void)viewDidLoad NS_AVAILABLE_MAC(10_10);
^
../../chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm:257:9: note:
explicitly redeclare 'viewDidLoad' to silence this warning
  [self viewDidLoad];

[Robert Sesek, 2015-09-22 18:25:44]

Yikes, this seems like a really poorly handled transition by Apple. Hopefully
this does it. Good luck && LGTM

[tapted, 2015-09-22 22:55:21]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2015-09-22 22:55:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1356023002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1356023002/20001

[commit-bot: I haz the power, 2015-09-22 23:45:07]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-22 23:45:44]

Patchset 2 (id:??) landed as
https://crrev.com/46ce5e6bc615a0f23207130ea2a06bff589ad2c0
Cr-Commit-Position: refs/heads/master@{#350264}