[SR-4906] Separate cookieStorage by application #1009
Conversation
| @@ -227,18 +227,22 @@ class TestNSHTTPCookieStorage: XCTestCase { | |||
| storage.setCookie(testCookie) | |||
| XCTAssertEqual(storage.cookies!.count, 3) | |||
| var destPath: String | |||
| //let bundleName = Bundle.main.object(forInfoDictionaryKey: "CFBundleDisplayName") as! String | |||
| //TODO: Above api returns nil on Linux when this code is introduced. Hence, using the below workaround. | |||
There was a problem hiding this comment.
Sounds like an expected failure. A macOS command line tool won't necessarily have an Info.plist with the bundle display name either. I don't think you should check in this commented-out code.
There was a problem hiding this comment.
On linux, NSBundle.infoDictionary:
infoDictionary: [String : Any]? {
let cfDict: CFDictionary? = CFBundleGetInfoDictionary(_bundle)
I printed the cfDict and saw the below values:
Optional<Dictionary<AnyHashable, Any>>
Key: CFBundleShortVersionString Value: 1.0
Key: CFBundleDevelopmentRegion Value: en
Key: NSHumanReadableCopyright Value: Copyright (c) 2014 - 2015 Apple Inc. and the Swift project authors
Key: CFBundleInfoDictionaryVersion Value: 6.0
Key: CFBundleName Value: TestFoundation
Key: CFBundleIdentifier Value: org.swift.TestFoundation
Key: CFBundleVersion Value: 1
Key: CFBundleNumericVersion Value: 16809984
Key: NSPrincipalClass Value:
Key: CFBundleExecutable Value: TestFoundation
Key: CFBundlePackageType Value: APPL
Key: CFBundleSignature Value: ????
It does have the Bundle name and other details, whereas the cast
return dict as! [String : Any]
fails. Thats why it looks like a bridging issue to me on Linux.
I’ve removed the commented code.
| //Needs replacement once fixed. | ||
| let bundlePath = Bundle.main.bundlePath | ||
| var bundlePathComponents = bundlePath.components(separatedBy: "/") | ||
| let bundleName: String = "/" + bundlePathComponents[bundlePathComponents.count - 1] |
There was a problem hiding this comment.
This won't do the right thing for executables ("command line tools" created in Xcode). The last component will just be the parent directory.
The : String is not necessary as it will be inferred.
There was a problem hiding this comment.
You can also simplify bundlePathComponents[bundlePathComponents.count - 1] to bundlePathComponents.last and safely unwrap the optional.
There was a problem hiding this comment.
Searched around to check if there is any other way I can get the Product name details of the command line, but in vain. Any ideas please?
Yep, removing the explicit String.
bundlePathComponents.last it is. Thanks!
| } else { | ||
| destPath = NSHomeDirectory() + "/.local/share/.cookies.shared" | ||
| destPath = NSHomeDirectory() + "/.local/share" + bundleName + "/.cookies.shared" |
There was a problem hiding this comment.
Shouldn't there be an / after /.local/share?
There was a problem hiding this comment.
"/" is appended to the bundleName during initialisation
var bundleName = "/" + bundlePath.components(separatedBy: "/").last!
| return FileManager.default.currentDirectoryPath + fileName | ||
| //if we were unable to create the desired directory, create the cookie file | ||
| //in a subFolder (named after the bundle) of the `pwd` | ||
| return FileManager.default.currentDirectoryPath + "/" + bundleName + fileName |
There was a problem hiding this comment.
If it's a subfolder, shouldn't there be a / between the bundleName and fileName?
There was a problem hiding this comment.
filename already has "/" prepended.
Foundation/NSHTTPCookieStorage.swift
Outdated
| @@ -38,7 +38,7 @@ open class HTTPCookieStorage: NSObject { | |||
|
|
|||
| private static var sharedStorage: HTTPCookieStorage? | |||
| private static var sharedCookieStorages: [String: HTTPCookieStorage] = [:] //for group storage containers | |||
|
|
|||
| private var bundleName: String! | |||
There was a problem hiding this comment.
Why store this value? Seems like a good candidate for a computed property (and you get to avoid the implicitly-unwrapped optional if you do that).
There was a problem hiding this comment.
Agreed. I have modified it. I compute the ‘bundleName’ now and pass it to the ‘filePath’ private function for reuse.
Foundation/NSHTTPCookieStorage.swift
Outdated
| var bundlePathComponents = bundlePath.components(separatedBy: "/") | ||
| let bundleDesc: String = bundlePathComponents[bundlePathComponents.count - 1] | ||
| bundleName = bundleDesc | ||
| if let range = bundleDesc.range(of: ".", options: String.CompareOptions.backwards, range: nil, locale: nil) { |
There was a problem hiding this comment.
There's a deletingPathExtension property on NSString you can use if that's what you're trying to do here.
https://developer.apple.com/reference/foundation/nsstring/1418214-deletingpathextension
You don't do this in the code below that also retrieves the bundle name though.
There was a problem hiding this comment.
NSString.deletingPathExtension is marked unavailable
now.
mamabusi
force-pushed
the
appSpecificSharedCookies
branch
from
8c437e6 to
678ac50
Compare
mamabusi
force-pushed
the
appSpecificSharedCookies
branch
from
678ac50 to
a660ad4
Compare
|
@mattrajca Thanks for your time on this. I have addressed the review comments. Please take a look. |
| @@ -47,7 +46,13 @@ open class HTTPCookieStorage: NSObject { | |||
| allCookies = [:] | |||
| cookieAcceptPolicy = .always | |||
| super.init() | |||
| cookieFilePath = filePath(path: _CFXDGCreateDataHomePath()._swiftObject, fileName: "/.cookies." + cookieStorageName) | |||
| let bundlePath = Bundle.main.bundlePath | |||
| var bundleName = bundlePath.components(separatedBy: "/").last! | |||
There was a problem hiding this comment.
@mamabusi Are we sure this will never have a nil?
There was a problem hiding this comment.
@pushkarnk Yes, 'bundlePath' is always a non-nil String that will ensure that the value returned by 'bundlePath.components(separatedBy: "/")' can be safely unwrapped.
|
Looks good to me. |
|
@swift-ci please test and merge |
No description provided.