Close the bubble when entering full screen

Fix not to close the bubble when entering full screen.

Listen to the full screen event, then if so, close the bubble.

BUG=326081
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278385

[Robert Sesek, 2014-06-11 15:03:54]

Thanks for the patch!

Have you signed the CLA, as per these instructions?
http://dev.chromium.org/developers/contributing-code#TOC-Get-your-code-ready I
can't find your email in the spreadsheet.

https://codereview.chromium.org/320393005/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/320393005/diff/1/AUTHORS#newcode224
AUTHORS:224: Malcolm Wang <malcolm.2.wang@gmail.com>
nit: alphabetize. Mal > Mah

https://codereview.chromium.org/320393005/diff/1/chrome/browser/ui/cocoa/base...
File chrome/browser/ui/cocoa/base_bubble_controller.mm (right):

https://codereview.chromium.org/320393005/diff/1/chrome/browser/ui/cocoa/base...
chrome/browser/ui/cocoa/base_bubble_controller.mm:126:
name:NSWindowWillEnterFullScreenNotification
This notification is only available on 10.7 and later, but Chrome needs to
support 10.6. 

Move the definition of this notification here:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

... into base/mac/sdk_forward_declarations.h. You'll need to create a new .mm
file to place the string value, though. So in the .h:

extern NSString* const NSWindowDidExitFullScreenNotification;

In the new .mm file:

NSString* const NSWindowDidExitFullScreenNotification =
@"NSWindowDidExitFullScreenNotification";

[Malcolm, 2014-06-12 13:07:05]

On 2014/06/11 15:03:54, rsesek wrote:
> Thanks for the patch!
> 
> Have you signed the CLA, as per these instructions?
> http://dev.chromium.org/developers/contributing-code#TOC-Get-your-code-ready I
> can't find your email in the spreadsheet.
> 
> https://codereview.chromium.org/320393005/diff/1/AUTHORS
> File AUTHORS (right):
> 
> https://codereview.chromium.org/320393005/diff/1/AUTHORS#newcode224
> AUTHORS:224: Malcolm Wang <mailto:malcolm.2.wang@gmail.com>
> nit: alphabetize. Mal > Mah
> 
>
https://codereview.chromium.org/320393005/diff/1/chrome/browser/ui/cocoa/base...
> File chrome/browser/ui/cocoa/base_bubble_controller.mm (right):
> 
>
https://codereview.chromium.org/320393005/diff/1/chrome/browser/ui/cocoa/base...
> chrome/browser/ui/cocoa/base_bubble_controller.mm:126:
> name:NSWindowWillEnterFullScreenNotification
> This notification is only available on 10.7 and later, but Chrome needs to
> support 10.6. 
> 
> Move the definition of this notification here:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> ... into base/mac/sdk_forward_declarations.h. You'll need to create a new .mm
> file to place the string value, though. So in the .h:
> 
> extern NSString* const NSWindowDidExitFullScreenNotification;
> 
> In the new .mm file:
> 
> NSString* const NSWindowDidExitFullScreenNotification =
> @"NSWindowDidExitFullScreenNotification";

Thank you for your instruction, I've signed the CLA and will update the patch
later.

[Malcolm, 2014-06-13 07:44:37]

Patch is updated

[Robert Sesek, 2014-06-13 14:07:46]

https://codereview.chromium.org/320393005/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/window_event.mm (right):

https://codereview.chromium.org/320393005/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/window_event.mm:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
nit: Don't add (c) to copyright lines in new files.

https://codereview.chromium.org/320393005/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/window_event.mm:5: #include
"base/mac/sdk_forward_declarations.h"
Move this file base/mac/sdk_forward_declarations.mm.

[Malcolm, 2014-06-16 03:02:52]

On 2014/06/13 14:07:46, rsesek wrote:
>
https://codereview.chromium.org/320393005/diff/20001/chrome/browser/ui/cocoa/...
> File chrome/browser/ui/cocoa/window_event.mm (right):
> 
>
https://codereview.chromium.org/320393005/diff/20001/chrome/browser/ui/cocoa/...
> chrome/browser/ui/cocoa/window_event.mm:1: // Copyright (c) 2014 The Chromium
> Authors. All rights reserved.
> nit: Don't add (c) to copyright lines in new files.
> 
>
https://codereview.chromium.org/320393005/diff/20001/chrome/browser/ui/cocoa/...
> chrome/browser/ui/cocoa/window_event.mm:5: #include
> "base/mac/sdk_forward_declarations.h"
> Move this file base/mac/sdk_forward_declarations.mm.

Hi Rsesek, thanks for your patience and pls have a look at the latest patch.

[Robert Sesek, 2014-06-16 15:02:06]

LGTM. Now you'll need someone in base/mac/OWNERS to approve the CL, and then you
can click the 'Commit' checkbox.

[Malcolm, 2014-06-17 01:44:06]

Hi Mark and Thakis,
I'm going to add a new file under base/mac/, as the owner , would you please
review this CL, thanks!

[Nico, 2014-06-17 04:40:56]

base/mac lgtm

[Malcolm, 2014-06-17 06:05:26]

The CQ bit was checked by malcolm.2.wang@gmail.com

[commit-bot: I haz the power, 2014-06-17 06:08:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/malcolm.2.wang@gmail.com/320393005/40001

[Malcolm, 2014-06-17 10:43:24]

mac_chromium_compile_dbg failed, because there is link error.
I forgot to add a dependency in chrome_browser_ui.gypi, which depends on
base.gypi. Sorry for the inconvenience.

[Mark Mentovai, 2014-06-17 13:30:41]

https://codereview.chromium.org/320393005/diff/60001/base/mac/sdk_forward_dec...
File base/mac/sdk_forward_declarations.h (right):

https://codereview.chromium.org/320393005/diff/60001/base/mac/sdk_forward_dec...
base/mac/sdk_forward_declarations.h:183: extern NSString* const
NSWindowWillEnterFullScreenNotification;
This should be extern "C". Look at where the SDK declares this:

APPKIT_EXTERN NSString * const NSWindowWillEnterFullScreenNotification
NS_AVAILABLE_MAC(10_7);

and then look at how APPKIT_EXTERN is defined (in <AppKit/AppKitDefines.h>).

https://codereview.chromium.org/320393005/diff/60001/chrome/chrome_browser_ui...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/320393005/diff/60001/chrome/chrome_browser_ui...
chrome/chrome_browser_ui.gypi:2513: 'dependencies': [
You may remove this, it wasn’t the problem. It would be amazing if none of the
existing code in here depended on base, but it all seems to work.

The problem is specific to the component=shared_library build. You need to add
BASE_EXPORT to NSWindowWillEnterFullScreenNotification.

[Malcolm, 2014-06-18 09:43:37]

On 2014/06/17 13:30:41, Mark Mentovai wrote:
>
https://codereview.chromium.org/320393005/diff/60001/base/mac/sdk_forward_dec...
> File base/mac/sdk_forward_declarations.h (right):
> 
>
https://codereview.chromium.org/320393005/diff/60001/base/mac/sdk_forward_dec...
> base/mac/sdk_forward_declarations.h:183: extern NSString* const
> NSWindowWillEnterFullScreenNotification;
> This should be extern "C". Look at where the SDK declares this:
> 
> APPKIT_EXTERN NSString * const NSWindowWillEnterFullScreenNotification
> NS_AVAILABLE_MAC(10_7);
> 
> and then look at how APPKIT_EXTERN is defined (in <AppKit/AppKitDefines.h>).
> 
>
https://codereview.chromium.org/320393005/diff/60001/chrome/chrome_browser_ui...
> File chrome/chrome_browser_ui.gypi (right):
> 
>
https://codereview.chromium.org/320393005/diff/60001/chrome/chrome_browser_ui...
> chrome/chrome_browser_ui.gypi:2513: 'dependencies': [
> You may remove this, it wasn’t the problem. It would be amazing if none of the
> existing code in here depended on base, but it all seems to work.
> 
> The problem is specific to the component=shared_library build. You need to add
> BASE_EXPORT to NSWindowWillEnterFullScreenNotification.

Thanks, Mark. I've updated.

[Mark Mentovai, 2014-06-18 12:49:55]

LGTM in base.

[Malcolm, 2014-06-19 01:23:17]

The CQ bit was checked by malcolm.2.wang@gmail.com

[commit-bot: I haz the power, 2014-06-19 01:24:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/malcolm.2.wang@gmail.com/320393005/80001

[commit-bot: I haz the power, 2014-06-19 07:34:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-19 07:34:01]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/18838)

[Malcolm, 2014-06-19 08:13:35]

The CQ bit was checked by malcolm.2.wang@gmail.com

[commit-bot: I haz the power, 2014-06-19 08:15:49]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/malcolm.2.wang@gmail.com/320393005/100001

[commit-bot: I haz the power, 2014-06-19 15:28:43]

Change committed as 278385