1843207 - security/manager/ssl/OSReauthenticatorDarwin.mm:9:10: fatal error: 'nsCocoaUtils.h' file not found

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P3)

Description

[Mike Hommey [:glandium]]

STR: (on a mac)

• Clone https://hg.mozilla.org/projects/cedar
• Revert the change from changeset 95f85676b7425b5d428a4a74bb5eb69a935432ca (https://hg.mozilla.org/projects/cedar/rev/95f85676b7425b5d428a4a74bb5eb69a935432ca)
• Create a mozconfig with:

ac_add_options --enable-application=mobile/ios
ac_add_options --disable-tests

• Build

The build fails with:

 0:10.53 /tmp/gecko/security/manager/ssl/OSReauthenticatorDarwin.mm:9:10: fatal error: 'nsCocoaUtils.h' file not found
 0:10.53 #include "nsCocoaUtils.h"
 0:10.53          ^~~~~~~~~~~~~~~~
 0:10.67 1 error generated.

I was going to file this as a PSM bug, but looking a little further, the only reason OSReauthenticatorDarwin.mm needs that header is to use ToNSString, which converts a nsAString to a NSString. If I copy the function in the source file, it builds just fine. Such utility function that apply to both mac and iOS should probably be moved to a more common place.

Maybe even out of widget. Xpcom would seem suitable in this specific case. There are other functions that convert geometry stuff (to/from NSRect/NSPoint to equivalent gecko types) that could live in gfx, but some other stuff that are more widgetty.

Comment 1

[Mike Hommey [:glandium]]

In the specific case of ToNSString, there is a xpcom/base/MacStringHelpers.mm where it would seem to be a good fit.

Comment 2

[Mike Hommey [:glandium]]

In fact, nsCocoaUtils has a GetStringForNSString that does the same thing as CopyCocoaStringToXPCOMString, albeit with no error checking.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

I agree that moving these non-widget-specific utilities out of widget and into xpcom makes sense, and MacStringHelpers does seem like a perfect existing header to do it with (though we should probably remove the Cocoa from the name given that it's also applicable to uikit - perhaps AppleString or CoreFoundationString?)

Comment 4

[Mike Hommey [:glandium]]

NSString would seem like the natural name. Merging GetStringForNSString and GetStringForNSString into one function would seem natural too.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

(In reply to Mike Hommey [:glandium] from comment #4)

NSString would seem like the natural name. Merging GetStringForNSString and GetStringForNSString into one function would seem natural too.

Yeah, the main downside of the name NSString is just that it's very similar to nsString - but I suppose that's hard to be helped. I believe that NSString is "toll free bridged" to the core-foundation CFStringRef type (https://developer.apple.com/documentation/foundation/nsstring?language=objc, https://developer.apple.com/library/archive/documentation/General/Conceptual/CocoaEncyclopedia/Toll-FreeBridgin/Toll-FreeBridgin.html), meaning that it wouldn't be unreasonable to use that name in ambiguous situations (though it may be confusing).

Comment 6

[Stephen A Pohl [:spohl]]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #5)

(In reply to Mike Hommey [:glandium] from comment #4)

NSString would seem like the natural name. Merging GetStringForNSString and GetStringForNSString into one function would seem natural too.

Yeah, the main downside of the name NSString is just that it's very similar to nsString

This is an existing, unfortunate situation in Cocoa land, but also an accepted one at this point. My vote is for NSString as well.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:spohl, could you have a look please?

For more information, please visit BugBot documentation.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Bug 1843207 - Rename CopyCocoaStringToXPCOMString to CopyNSStringToXPCOMString.

And make nsCocoaUtils::GetStringForNSString rely on it.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Bug 1843207 - Add a XPCOMStringToNSString function that similar to nsCocoaUtils::ToNSString.

And make the latter rely on the former.

Comment 10

[Mike Hommey [:glandium]]

Attachment: Bug 1843207 - Stop using nsCocoaUtils::ToNSString in OSReauthenticatorDarwin.mm.

and enable it on iOS.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59bb60ab89dd
Rename CopyCocoaStringToXPCOMString to CopyNSStringToXPCOMString. r=nika
https://hg.mozilla.org/integration/autoland/rev/bd6d6b3f26c7
Add a XPCOMStringToNSString function that similar to nsCocoaUtils::ToNSString. r=nika
https://hg.mozilla.org/integration/autoland/rev/5aecee71c900
Stop using nsCocoaUtils::ToNSString in OSReauthenticatorDarwin.mm. r=nika

Comment 12

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/59bb60ab89dd
https://hg.mozilla.org/mozilla-central/rev/bd6d6b3f26c7
https://hg.mozilla.org/mozilla-central/rev/5aecee71c900