Use Markable instead of std::optional when storing UUIDs in structs #9433
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
achristensen07
requested review from
b-weinstein,
xeenon and
cdumez
as code owners
achristensen07
added
the
WebCore Misc.
Constellation
approved these changes
Feb 1, 2023
| @@ -117,7 +117,7 @@ std::optional<NetworkSessionCreationParameters> NetworkSessionCreationParameters | |||
| if (!sessionID) | |||
| return std::nullopt; | |||
|
|
|||
| std::optional<std::optional<UUID>> dataStoreIdentifier; | |||
| std::optional<Markable<UUID>> dataStoreIdentifier; | |||
There was a problem hiding this comment.
We can just decode it into std::optional, and then we can assign it to markable.
There was a problem hiding this comment.
This is more correct. I really ought to just make it NetworkSessionCreationParameters.serialization.in which would generate this, but that should probably be a separate PR.
| @@ -50,7 +51,7 @@ class WebExtensionControllerConfiguration : public API::ObjectImpl<API::Object:: | |||
| explicit WebExtensionControllerConfiguration(IsPersistent); | |||
| explicit WebExtensionControllerConfiguration(const UUID&); | |||
|
|
|||
| std::optional<UUID> identifier() const { return m_identifier; } | |||
| Markable<UUID> identifier() const { return m_identifier; } | |||
There was a problem hiding this comment.
You can continue returning std::optional if it is better since Markable <-> std::optional conversion can happen implicitly. So, I would recommend using std::optional usually, and using Markable only when we store it to the member.
| @@ -59,7 +60,7 @@ class WebsiteDataStoreConfiguration : public API::ObjectImpl<API::Object::Type:: | |||
| Ref<WebsiteDataStoreConfiguration> copy() const; | |||
|
|
|||
| bool isPersistent() const { return m_isPersistent == IsPersistent::Yes; } | |||
| std::optional<UUID> identifier() const { return m_identifier; } | |||
| Markable<UUID> identifier() const { return m_identifier; } | |||
| @@ -66,7 +66,7 @@ class ClientConnection : public RefCounted<ClientConnection>, public CanMakeWeak | |||
| bool hostAppHasPushInjectEntitlement(); | |||
|
|
|||
| const String& pushPartitionString() const { return m_pushPartitionString; } | |||
| std::optional<UUID> dataStoreIdentifier() const { return m_dataStoreIdentifier; } | |||
| Markable<UUID> dataStoreIdentifier() const { return m_dataStoreIdentifier; } | |||
| @@ -529,7 +529,7 @@ async function disableShowNotifications() | |||
| TestWebKitAPI::Util::run(&ready); | |||
| } | |||
|
|
|||
| const std::optional<UUID>& dataStoreIdentifier() { return m_dataStoreIdentifier; } | |||
| const Markable<UUID>& dataStoreIdentifier() { return m_dataStoreIdentifier; } | |||
|
EWS run on previous version of this PR (hash 12c32e8) |
achristensen07
force-pushed
the
eng/Use-Markable-instead-of-stdoptional-when-storing-UUIDs-in-structs
branch
from
12c32e8
to
b2b2f97
Compare
|
EWS run on current version of this PR (hash b2b2f97) |
achristensen07
added
the
merge-queue
https://bugs.webkit.org/show_bug.cgi?id=251485 rdar://104902597 Reviewed by Yusuke Suzuki. This removes unnecessary and unused padding bytes, decreasing memory use. * Source/WTF/wtf/UUID.h: (WTF::UUID::MarkableTraits::isEmptyValue): (WTF::UUID::MarkableTraits::emptyValue): * Source/WebCore/Modules/push-api/PushSubscriptionIdentifier.h: * Source/WebKit/NetworkProcess/NetworkSession.h: * Source/WebKit/NetworkProcess/NetworkSessionCreationParameters.cpp: (WebKit::NetworkSessionCreationParameters::decode): * Source/WebKit/NetworkProcess/NetworkSessionCreationParameters.h: * Source/WebKit/Platform/IPC/DaemonCoders.h: * Source/WebKit/Shared/WebPushDaemonConnectionConfiguration.h: * Source/WebKit/Shared/WebPushDaemonConnectionConfiguration.serialization.in: * Source/WebKit/UIProcess/Extensions/WebExtensionControllerConfiguration.h: (WebKit::WebExtensionControllerConfiguration::identifier const): * Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreConfiguration.h: (WebKit::WebsiteDataStoreConfiguration::identifier const): * Source/WebKit/webpushd/PushClientConnection.h: (WebPushD::ClientConnection::dataStoreIdentifier const): * Tools/TestWebKitAPI/Tests/WebKitCocoa/WebPushDaemon.mm: Canonical link: https://commits.webkit.org/259665@main
webkit-commit-queue
force-pushed
the
eng/Use-Markable-instead-of-stdoptional-when-storing-UUIDs-in-structs
branch
from
b2b2f97
to
4c52b06
Compare
|
Committed 259665@main (4c52b06): https://commits.webkit.org/259665@main Reviewed commits have been landed. Closing PR #9433 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
4c52b06
b2b2f97
🛠 🧪 win🛠 gtk🛠 wincairo🧪 ios-wk2🧪 api-mac🧪 gtk-wk2🧪 api-ios🧪 api-gtk🛠 🧪 jsc🧪 mac-wk2🛠 tv-sim🧪 mac-AS-debug-wk2🛠 watch-sim