Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1135333002:
Suppress -Wpartial-availability warnings that arise from NSWindowDelegate methods only available on… (Closed)
DescriptionSuppress -Wpartial-availability warnings that arise from NSWindowDelegate.
Some methods in the protocol NSWindowDelegate are only declared in an OSX 10.7+
SDK. There is no good way to suppress -Wpartial-availability warnings for
protocols. In BrowserWindowController, redeclare the methods in the private
header. For bridge_native_widget_unittest, use performSelector: to invoke the
methods.
BUG=471823
Committed: https://crrev.com/905a291f9595753d0ed663761eff707d5579c48c
Cr-Commit-Position: refs/heads/master@{#330472}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Comments from thakis. #
Total comments: 2
Patch Set 3 : Comments from thakis. #
Total comments: 3
Patch Set 4 : Remove two unnecessary redeclarations from browser_window_controller_private.h. #
Messages
Total messages: 20 (2 generated)
erikchen@chromium.org changed reviewers: + thakis@chromium.org
thakis: Please review.
https://codereview.chromium.org/1135333002/diff/1/chrome/browser/ui/cocoa/bro... File chrome/browser/ui/cocoa/browser_window_controller_private.h (right): https://codereview.chromium.org/1135333002/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_controller_private.h:168: // -Wpartial-availability warnings. Should this have some "// Available in 10.7, remove once that's the minimum deployment target" comment too? https://codereview.chromium.org/1135333002/diff/1/ui/views/cocoa/bridged_nati... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1135333002/diff/1/ui/views/cocoa/bridged_nati... ui/views/cocoa/bridged_native_widget_unittest.mm:494: withObject:window]; why this change?
thakis: PTAL https://codereview.chromium.org/1135333002/diff/1/chrome/browser/ui/cocoa/bro... File chrome/browser/ui/cocoa/browser_window_controller_private.h (right): https://codereview.chromium.org/1135333002/diff/1/chrome/browser/ui/cocoa/bro... chrome/browser/ui/cocoa/browser_window_controller_private.h:168: // -Wpartial-availability warnings. On 2015/05/12 22:29:38, Nico wrote: > Should this have some "// Available in 10.7, remove once that's the minimum > deployment target" comment too? I wrapped this in the preprocessor conditional that mark and I have been using to indicate that the contained code should be removed once we stop supporting 10.6. https://codereview.chromium.org/1135333002/diff/1/ui/views/cocoa/bridged_nati... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1135333002/diff/1/ui/views/cocoa/bridged_nati... ui/views/cocoa/bridged_native_widget_unittest.mm:494: withObject:window]; On 2015/05/12 22:29:38, Nico wrote: > why this change? The method windowDidFailToEnterFullScreen: is only available on 10.7+ and is defined in a protocol. Switching to a perform-selector suppresses the warning. If you prefer, I could use clang preprocessor directives.
https://codereview.chromium.org/1135333002/diff/1/ui/views/cocoa/bridged_nati... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1135333002/diff/1/ui/views/cocoa/bridged_nati... ui/views/cocoa/bridged_native_widget_unittest.mm:494: withObject:window]; On 2015/05/12 22:41:39, erikchen wrote: > On 2015/05/12 22:29:38, Nico wrote: > > why this change? > > The method windowDidFailToEnterFullScreen: is only available on 10.7+ and is > defined in a protocol. Switching to a perform-selector suppresses the warning. > If you prefer, I could use clang preprocessor directives. Huh, if you declare it it should be possible to call it, no?
https://codereview.chromium.org/1135333002/diff/1/ui/views/cocoa/bridged_nati... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1135333002/diff/1/ui/views/cocoa/bridged_nati... ui/views/cocoa/bridged_native_widget_unittest.mm:494: withObject:window]; On 2015/05/12 22:45:00, Nico wrote: > On 2015/05/12 22:41:39, erikchen wrote: > > On 2015/05/12 22:29:38, Nico wrote: > > > why this change? > > > > The method windowDidFailToEnterFullScreen: is only available on 10.7+ and is > > defined in a protocol. Switching to a perform-selector suppresses the warning. > > If you prefer, I could use clang preprocessor directives. > > Huh, if you declare it it should be possible to call it, no? Just to confirm: You would prefer it if I cast window_delegate to ViewsNSWindowDelegate, add redeclarations of the relevant methods to ViewsNSWindowDelegate, and add VIEWS_EXPORT to ViewsNSWindowDelegate?
https://codereview.chromium.org/1135333002/diff/1/ui/views/cocoa/bridged_nati... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1135333002/diff/1/ui/views/cocoa/bridged_nati... ui/views/cocoa/bridged_native_widget_unittest.mm:494: withObject:window]; On 2015/05/12 23:49:19, erikchen wrote: > On 2015/05/12 22:45:00, Nico wrote: > > On 2015/05/12 22:41:39, erikchen wrote: > > > On 2015/05/12 22:29:38, Nico wrote: > > > > why this change? > > > > > > The method windowDidFailToEnterFullScreen: is only available on 10.7+ and is > > > defined in a protocol. Switching to a perform-selector suppresses the > warning. > > > If you prefer, I could use clang preprocessor directives. > > > > Huh, if you declare it it should be possible to call it, no? > > Just to confirm: You would prefer it if I cast window_delegate to > ViewsNSWindowDelegate, add redeclarations of the relevant methods to > ViewsNSWindowDelegate, and add VIEWS_EXPORT to ViewsNSWindowDelegate? Are you sure that you need the cast and the export? Does just adding the declaration there suffice?
https://codereview.chromium.org/1135333002/diff/1/ui/views/cocoa/bridged_nati... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1135333002/diff/1/ui/views/cocoa/bridged_nati... ui/views/cocoa/bridged_native_widget_unittest.mm:494: withObject:window]; On 2015/05/13 00:18:30, Nico wrote: > On 2015/05/12 23:49:19, erikchen wrote: > > On 2015/05/12 22:45:00, Nico wrote: > > > On 2015/05/12 22:41:39, erikchen wrote: > > > > On 2015/05/12 22:29:38, Nico wrote: > > > > > why this change? > > > > > > > > The method windowDidFailToEnterFullScreen: is only available on 10.7+ and > is > > > > defined in a protocol. Switching to a perform-selector suppresses the > > warning. > > > > If you prefer, I could use clang preprocessor directives. > > > > > > Huh, if you declare it it should be possible to call it, no? > > > > Just to confirm: You would prefer it if I cast window_delegate to > > ViewsNSWindowDelegate, add redeclarations of the relevant methods to > > ViewsNSWindowDelegate, and add VIEWS_EXPORT to ViewsNSWindowDelegate? > > Are you sure that you need the cast and the export? Does just adding the > declaration there suffice? Ah, they are not necessary. One caveat: This class currently doesn't include "views_nswindow_delegate.h", which is where the redeclaration would happen. I can add an include, but there will be no visibility as to why the include is necessary. I guess I could wrap the include the ifdefs? It's not very clear that we're using the redeclarations in views_nswindow_delegate.h to suppress a warning on this "id window_delegate".
thakis: PTAL
(for reference, the compile error against 10.6 SDK is:
../../ui/views/cocoa/bridged_native_widget_unittest.mm:494:20: error: instance
method '-windowDidFailToEnterFullScreen:' not found (return type defaults to
'id') [-Werror,-Wobjc-method-access]
[window_delegate windowDidFailToEnterFullScreen:window];
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
)
https://codereview.chromium.org/1135333002/diff/1/ui/views/cocoa/bridged_nati...
File ui/views/cocoa/bridged_native_widget_unittest.mm (right):
https://codereview.chromium.org/1135333002/diff/1/ui/views/cocoa/bridged_nati...
ui/views/cocoa/bridged_native_widget_unittest.mm:494: withObject:window];
On 2015/05/13 00:26:21, erikchen wrote:
> On 2015/05/13 00:18:30, Nico wrote:
> > On 2015/05/12 23:49:19, erikchen wrote:
> > > On 2015/05/12 22:45:00, Nico wrote:
> > > > On 2015/05/12 22:41:39, erikchen wrote:
> > > > > On 2015/05/12 22:29:38, Nico wrote:
> > > > > > why this change?
> > > > >
> > > > > The method windowDidFailToEnterFullScreen: is only available on 10.7+
> and
> > is
> > > > > defined in a protocol. Switching to a perform-selector suppresses the
> > > warning.
> > > > > If you prefer, I could use clang preprocessor directives.
> > > >
> > > > Huh, if you declare it it should be possible to call it, no?
> > >
> > > Just to confirm: You would prefer it if I cast window_delegate to
> > > ViewsNSWindowDelegate, add redeclarations of the relevant methods to
> > > ViewsNSWindowDelegate, and add VIEWS_EXPORT to ViewsNSWindowDelegate?
> >
> > Are you sure that you need the cast and the export? Does just adding the
> > declaration there suffice?
>
> Ah, they are not necessary.
>
> One caveat: This class currently doesn't include "views_nswindow_delegate.h",
> which is where the redeclaration would happen. I can add an include, but there
> will be no visibility as to why the include is necessary. I guess I could wrap
> the include the ifdefs? It's not very clear that we're using the
redeclarations
> in views_nswindow_delegate.h to suppress a warning on this "id
window_delegate".
Hm. Removing NSWindowDelegateFullScreenAdditions from sdk_forward_declarations.h
prevents this code from compiling against 10.6 SDK. That...doesn't make sense to
me. I don't understand why defining a protocol that is never used removes the
compile failure. The code was introduced here:
https://codereview.chromium.org/665823002/
The name of the protocol itself doesn't matter - I changed it to "sdfasdf" and
its existence still removed the compile failure.
I think that switching to performSelector: and removing this "magic" protocol
from sdk_forward_declarations.h is the best solution. wdyt?
On 2015/05/13 16:56:43, erikchen wrote: > thakis: PTAL > > (for reference, the compile error against 10.6 SDK is: > > ../../ui/views/cocoa/bridged_native_widget_unittest.mm:494:20: error: instance > method '-windowDidFailToEnterFullScreen:' not found (return type defaults to > 'id') [-Werror,-Wobjc-method-access] > [window_delegate windowDidFailToEnterFullScreen:window]; > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ) > > https://codereview.chromium.org/1135333002/diff/1/ui/views/cocoa/bridged_nati... > File ui/views/cocoa/bridged_native_widget_unittest.mm (right): > > https://codereview.chromium.org/1135333002/diff/1/ui/views/cocoa/bridged_nati... > ui/views/cocoa/bridged_native_widget_unittest.mm:494: withObject:window]; > On 2015/05/13 00:26:21, erikchen wrote: > > On 2015/05/13 00:18:30, Nico wrote: > > > On 2015/05/12 23:49:19, erikchen wrote: > > > > On 2015/05/12 22:45:00, Nico wrote: > > > > > On 2015/05/12 22:41:39, erikchen wrote: > > > > > > On 2015/05/12 22:29:38, Nico wrote: > > > > > > > why this change? > > > > > > > > > > > > The method windowDidFailToEnterFullScreen: is only available on 10.7+ > > and > > > is > > > > > > defined in a protocol. Switching to a perform-selector suppresses the > > > > warning. > > > > > > If you prefer, I could use clang preprocessor directives. > > > > > > > > > > Huh, if you declare it it should be possible to call it, no? > > > > > > > > Just to confirm: You would prefer it if I cast window_delegate to > > > > ViewsNSWindowDelegate, add redeclarations of the relevant methods to > > > > ViewsNSWindowDelegate, and add VIEWS_EXPORT to ViewsNSWindowDelegate? > > > > > > Are you sure that you need the cast and the export? Does just adding the > > > declaration there suffice? > > > > Ah, they are not necessary. > > > > One caveat: This class currently doesn't include "views_nswindow_delegate.h", > > which is where the redeclaration would happen. I can add an include, but there > > will be no visibility as to why the include is necessary. I guess I could wrap > > the include the ifdefs? It's not very clear that we're using the > redeclarations > > in views_nswindow_delegate.h to suppress a warning on this "id > window_delegate". > > Hm. Removing NSWindowDelegateFullScreenAdditions from sdk_forward_declarations.h > prevents this code from compiling against 10.6 SDK. That...doesn't make sense to > me. I don't understand why defining a protocol that is never used removes the > compile failure. The code was introduced here: > https://codereview.chromium.org/665823002/ > > The name of the protocol itself doesn't matter - I changed it to "sdfasdf" and > its existence still removed the compile failure. > > I think that switching to performSelector: and removing this "magic" protocol > from sdk_forward_declarations.h is the best solution. wdyt? thakis: ping?
(+tapted in case he disagrees with me) https://codereview.chromium.org/1135333002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1135333002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:491: // compiles on 10.6. So I think this is the problem. The Right Fix I think is to: 1. Declare these methods in ui/views/cocoa/views_nswindow_delegate.h 2. Assert that [window delegate] is a ViewsNSWindowDelegate (if this wasn't true this test wouldn't pass on 10.6) 3. Cast the pointer to ViewsNSWindowDelegate, and call the method there directly
thakis: PTAL https://codereview.chromium.org/1135333002/diff/20001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1135333002/diff/20001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:491: // compiles on 10.6. On 2015/05/18 23:45:09, Nico wrote: > So I think this is the problem. The Right Fix I think is to: > > 1. Declare these methods in ui/views/cocoa/views_nswindow_delegate.h > 2. Assert that [window delegate] is a ViewsNSWindowDelegate (if this wasn't true > this test wouldn't pass on 10.6) > 3. Cast the pointer to ViewsNSWindowDelegate, and call the method there directly 1. I declared the relevant methods in ui/views/cocoa/views_nswindow_delegate.h. 2. This test doesn't run on OSX 10.6, so I left out the assert. 3. I used base::mac::ObjCCast. https://codereview.chromium.org/1135333002/diff/40001/ui/views/cocoa/views_ns... File ui/views/cocoa/views_nswindow_delegate.h (right): https://codereview.chromium.org/1135333002/diff/40001/ui/views/cocoa/views_ns... ui/views/cocoa/views_nswindow_delegate.h:20: VIEWS_EXPORT This is necessary when I do a component build.
On 2015/05/18 23:45:09, Nico wrote: > (+tapted in case he disagrees with me) > > https://codereview.chromium.org/1135333002/diff/20001/ui/views/cocoa/bridged_... > File ui/views/cocoa/bridged_native_widget_unittest.mm (right): > > https://codereview.chromium.org/1135333002/diff/20001/ui/views/cocoa/bridged_... > ui/views/cocoa/bridged_native_widget_unittest.mm:491: // compiles on 10.6. > So I think this is the problem. The Right Fix I think is to: > > 1. Declare these methods in ui/views/cocoa/views_nswindow_delegate.h > 2. Assert that [window delegate] is a ViewsNSWindowDelegate (if this wasn't true > this test wouldn't pass on 10.6) > 3. Cast the pointer to ViewsNSWindowDelegate, and call the method there directly sgtm. Downside that "every" NSWindowDelegate wanting these would have to redeclare is not so terrible :)
https://codereview.chromium.org/1135333002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_private.h (right): https://codereview.chromium.org/1135333002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.h:170: // -Wpartial-availability warnings. is this still needed? all of it? (i think the only hit for windowDidFailToEnterFullScreen i found earlier was in that views file?)
https://codereview.chromium.org/1135333002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_private.h (right): https://codereview.chromium.org/1135333002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.h:170: // -Wpartial-availability warnings. On 2015/05/19 00:38:02, Nico wrote: > is this still needed? all of it? (i think the only hit for > windowDidFailToEnterFullScreen i found earlier was in that views file?) I removed the two unnecessary redeclarations.
lgtm
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135333002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/905a291f9595753d0ed663761eff707d5579c48c Cr-Commit-Position: refs/heads/master@{#330472} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
