1338925 - Enable warnings-as-errors in widget/cocoa/

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[(Away)]

As far as I can tell, this is the last remaining non-third-party use of ALLOW_COMPILER_WARNINGS in our tree (if you count NSS and NSPR as third-party). Let's clean it up!

Comment 1

[(Away)]

Attachment: Fix the exception macro in nsDragService.mm

nsDragService.mm:223:31: error: expression result unused [-Werror,-Wunused-value]

From a look at nearby code, here's my guess at what was intended.

Comment 2

[(Away)]

Attachment: Debug-only variables in nsDeviceContentSpecX.mm

nsDeviceContextSpecX.mm:202:19: error: unused variable 'destPath' [-Werror,-Wunused-variable]

Comment 3

[(Away)]

Attachment: Declare viewDidChangeBackingProperties in nsChildView.h

nsChildView.mm:3642:12: error: 'NSView' may not respond to 'viewDidChangeBackingProperties' [-Werror]

This is the patch I'm least sure about. Is it safe to just declare it among the undocumented methods? https://developer.apple.com/reference/appkit/nsview/1483742-viewdidchangebackingproperties says it's available since 10.7.

Comment 4

[(Away)]

Attachment: Remove ALLOW_COMPILER_WARNINGS in moz.build

Comment 5

[Markus Stange [:mstange]]

Thank you so much for doing this! I've been meaning to do it for a long time but never got around to it.

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8836513 [details] [diff] [review]
Fix the exception macro in nsDragService.mm

Review of attachment 8836513 [details] [diff] [review]:
-----------------------------------------------------------------

Oops, yes.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8836515 [details] [diff] [review]
Declare viewDidChangeBackingProperties in nsChildView.h

Review of attachment 8836515 [details] [diff] [review]:
-----------------------------------------------------------------

Where did you see this warning? Locally, or on try? If locally, are you compiling with the 10.6 SDK or with something more recent? IIRC our build machines compile with the 10.9 SDK.
But I suppose there could be more people out there building Gecko with the 10.6 SDK, -Werror would be breaking their builds without this patch, so we should land it anyway.

Declaring it in this place is fine. Redeclarations of an existing method don't cause errors or other breakage. The only way this could go wrong is if other code called [someView viewDidChangeBackingProperties] on a pre-10.7 platform without checking whether the method exists (using respondsToSelector). Doing so would throw an Objective C exception at runtime and wouldn't give you any warnings at compile time. But we only call viewDidChangeBackingProperties if Cocoa already called this method so it must exist. And we only run on 10.9+ anyway.

Comment 8

[(Away)]

(In reply to Markus Stange [:mstange] from comment #7)
> Comment on attachment 8836515 [details] [diff] [review]
> Declare viewDidChangeBackingProperties in nsChildView.h
> 
> Review of attachment 8836515 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Where did you see this warning? Locally, or on try? If locally, are you
> compiling with the 10.6 SDK or with something more recent? IIRC our build
> machines compile with the 10.9 SDK.

On try, which makes that build error particularly confusing.

Comment 9

[Stefan [:stefanh]]

NSView.h in the 10.7sdk has this line:
- (void)viewDidChangeBackingProperties NS_AVAILABLE_MAC(10_7); // available in 10.7.4

So perhaps the try builders are using an older 10.7 version.

Comment 10

[Stefan [:stefanh]]

> So perhaps the try builders are using an older 10.7 version.

which was suggested in bug 1296967, comment #4.

Comment 11

[Markus Stange [:mstange]]

That would explain it. Thanks for checking!

Comment 12

[(Away)]

Indeed: https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/browser/config/tooltool-manifests/macosx64/cross-releng.manifest#24

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e06b22dfa60
Use the correct exception macro in nsDragService::IsValidType. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/81a31c193a1b
Move debug-only variables to ifdef DEBUG. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fe222eed019
Declare viewDidChangeBackingProperties on NSView. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2400aa8116
Enable warnings-as-errors in widget/cocoa/. r=mstange

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0e06b22dfa60
https://hg.mozilla.org/mozilla-central/rev/81a31c193a1b
https://hg.mozilla.org/mozilla-central/rev/8fe222eed019
https://hg.mozilla.org/mozilla-central/rev/fb2400aa8116