Fix multi-window bookmark bubble problem....

Fix multi-window bookmark bubble problem.

Also fix a "flash" where the bookmark bubble opened in the wrong spot
before jumping to the correct spot (hard to see).

BookmarkBubble.xib change: NOT initially visible.

BUG=http://crbug.com/27545

TEST=see bug (it's clear).


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=32030

[viettrungluu, 2009-11-13 22:01:00]

Generally LG with some clarifications. I haven't read the unit test very
carefully, but your changes are minimal and seem right.

http://codereview.chromium.org/391057/diff/1/4
File chrome/browser/cocoa/bookmark_bubble_controller.h (right):

http://codereview.chromium.org/391057/diff/1/4#newcode13
Line 13: // Protocol for a BookmarkBubbleController's (BBC's) delegate.
You know, the BBC is also the BookmarkBarController (in addition to the British
Broadcasting Corporation).

http://codereview.chromium.org/391057/diff/1/5
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/391057/diff/1/5#newcode65
Line 65: // showWindow:] will also bring on-screen (but will cause unexpected
s/bring on-screen/bring it on-screen/

More importantly, the relation between -addChildWindow:'s action and
-showWindow:'s is unclear from your comment. Are they both going to get called,
and is that the problem?

http://codereview.chromium.org/391057/diff/1/5#newcode148
Line 148: // performSelector: call below triggers a close after a desire to
I don't understand "after a desire".

http://codereview.chromium.org/391057/diff/1/6
File chrome/browser/cocoa/bookmark_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/391057/diff/1/6#newcode39
Line 39: - (void)setWindowController:(NSWindowController *)controller {
Could you comment on this a bit (given that it doesn't seem to do anything like
what the method name implies)?

http://codereview.chromium.org/391057/diff/1/3
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/391057/diff/1/3#newcode1164
Line 1164: if (window == [bookmarkBubbleController_ window])
Is there any case where this would be false? Should there be a DCHECK()?

[John Grabowski, 2009-11-13 22:58:22]

Feedback applied (mostly comment changes).
Final diffs up.
Thx Trung.

http://codereview.chromium.org/391057/diff/1/3
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/391057/diff/1/3#newcode1164
Line 1164: if (window == [bookmarkBubbleController_ window])
On 2009/11/13 22:01:01, viettrungluu wrote:
> Is there any case where this would be false? Should there be a DCHECK()?

2 windows.  1 has bubble open.  Switch focus by clicking on the star of the
other window.

windowDidResignKey of the bubble is used to hide it, but since it's a window
delegate it can't close itself.  The close is thus from a
performSelector:afterDelay:0.  But that gets processed AFTER the new bubble is
created.  To deal, it calls this routine before the performSelector:.  but to
handle the non-wierd case (e.g. user clicks OK), a "normal" call to
bubbleWindowWillClose: is made inside the windowWillClose.

The browser window controller only needs this ptr to close the bubble if the
browser window loses focus, so if the bubble is going away anyway (e.g. the
2-window case) it's fine for the pointer to get clobbered.

[viettrungluu, 2009-11-13 23:04:42]

Thanks, LGTM other than a punctuation nit (just to annoy you).

http://codereview.chromium.org/391057/diff/5001/5005
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/391057/diff/5001/5005#newcode68
Line 68: // showWindow: Thus, we have our own version.
You're missing a period (or something) on this line.

[John Grabowski, 2009-11-13 23:07:09]

It works; I'm annoyed.

http://codereview.chromium.org/391057/diff/5001/5005
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/391057/diff/5001/5005#newcode68
Line 68: // showWindow: Thus, we have our own version.
On 2009/11/13 23:04:42, viettrungluu wrote:
> You're missing a period (or something) on this line.

fixed

[mrossetti, 2009-11-13 23:46:55]

LGTM