-
-
Notifications
You must be signed in to change notification settings - Fork 15.5k
qt511.qtwebengine: fix on darwin #53848
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This is needed to facilitate build of qt5.qtwebengine on darwin
8fbe7b6
to
886c6de
Compare
Forgot to commit couple important patches, now ok. |
886c6de
to
8c78894
Compare
Co-Authored-By: Josef Kemetmüller <josef.kemetmueller@gmail.com>
8c78894
to
45ab55e
Compare
|
||
namespace { | ||
|
||
-// TODO(davidben): Remove this when we switch to building to the 10.13 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch looks a bit scary. What is it doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In short, this disables signing using "Certificate, Key, and Trust Services API" and makes it use CSSM API instead. If you look below, this branch, that I removed, is hidden behind a version check:
if (base::mac::IsAtLeastOS10_12()) {
return /* bla using SecKey */;
}
return /* bla using CSSM */;
So I only drop an optional execution path.
There is a nice overview of the API's in http://www.arp.com/medias/13902065.pdf . From their explanation, I understood that CSDA (and CSSM) are more sophisticated API's produced by some open source standard group while "Certificate, Key, and Trust Services API" is what Apple wants us to use. They seem to allude that the latter API is just a wrapper around the former:
Where possible, you should use one of the following instead of using CDSA directly:
- The Certificate, Key, and Trust Services API for general encryption, key management, and other tasks.
I suppose, the reason why CSSM is not the preferred API in chromium is because they want to maintain it in a future proof state. My best understanding is that this patch is harmless and we can drop it once 10.12 SDK lands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok.
# error: pointer to non-const type 'id' with no explicit ownership | ||
# id** _kvcPropertyAccessors; | ||
# | ||
# TODO remove when new Apple SDK is in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't whether ARC is used a property of a codebase, rather than of an SDK version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says that at very least some of the headers in our SDK don't compile when ARC is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I can also confirm that this builds on my mac (boy that took some time to finish though)
Motivation for this change
QtWebEngine is broken on darwin.
Resolves: #36932
Things done
cc @matthewbauer @knedlsepp