I've been thinking about how we can improve our attribute querying in light of how we plan to split up cache domains. There are a few classes of bugs I'd like to make difficult to write:
- Requesting a value for a cache key associated with an inactive domain
- Forgetting to properly request the relevant domains for the function at hand
- Passing the wrong template type for the given cache key to GetAttribute
There are a few things I'd like to accomplish to address these issues:
1. Make it easy to get domains associated with cache keys
a. Do this in a constexpr way so we don't incur any runtime penalty (hash lookups or memory) and don't have to worry about static initialization
b. Do this while also using the cache key static atom aliases, for readability
2. Make it easy to request domains and return early if any of the domains associated with cache keys are not active
3. Make it easy to get the data type associated with each cache key
4. Enforce the rule that any requested cache key must be domain-checked before querying it. That is, don't let ourselves request a cache key without ensuring that the associated domain is already active.
I've been toying around and proving out ideas to address these hopes.
### Getting and Requesting Cache Domains
To get the domain associated with each cache key, I'd like to introduce a function of this form:
```
constexpr uint64_t GetCacheDomainForKey(const nsStaticAtom* aCacheKey) {
if (aCacheKey == CacheKey::Name) {
return CacheDomain::NameAndDescription;
} else if (aCacheKey == CacheKey::State) {
return CacheDomain::State;
} else if (/* and so on... */) {}
}
```
Because static atoms are comparable at compile time, we can write
```
constexpr uint64_t domainForCacheKey = GetCacheDomainForKey(CacheKey::Name);
```
and resolve this domain at compile time, without the need for a hash table or other runtime lookup.
For any number of cache keys, we could use something like:
```
constexpr uint64_t GetRequiredCacheDomains(
std::initializer_list<nsStaticAtom*> aAttributes) {
uint64_t requiredCacheDomains = 0;
for (nsStaticAtom* attribute : aAttributes) {
requiredCacheDomains |= GetCacheDomainForKey(attribute);
}
return requiredCacheDomains;
}
```
Then,
```
constexpr uint64_t requiredCacheDomains = GetRequiredCacheDomains(CacheKey::Name, CacheKey::State);
```
also resolves at compile time.
Next, assume we have a function `RequestDomainsIfMissing` which takes the required cache domains and compares them to the active cache domains, sending off requests to all content processes and returning true if any domains were missing. We can then write a macro like so (name WIP) which returns the "domain not cached" result and fires off requests to content if any associated domain is missing:
```
#define CHECK_ATTRIBUTE_DOMAINS_CACHED(aAttributesNotCachedResult, ...) \
{ \
constexpr uint64_t requiredCacheDomains = \
GetRequiredCacheDomains({__VA_ARGS__}); \
if (RequestDomainsIfMissing(requiredCacheDomains)) { \
return aAttributesNotCachedResult; \
} \
}
```
This macro can be invoked at the top of every RemoteAccessible function that requests something from the cache. For instance:
```
CHECK_ATTRIBUTE_DOMAINS_CACHED(false, CacheKey::Name, CacheKey::State)
```
I think marshaling all attributes into a domain, even if that domain is an "Always" domain or similar, would help us here. This solves (1) and (2) above.
### Getting the Data Type Associated with each Cache Key
(3) Is not a directly pressing problem, but I think it's worth solving. Crucially, we _know_ the data types associated with each cache key at compile time. We open ourselves up to really annoying-to-hunt-down programming mistakes that we could solve entirely at compile time. The actual address of cache keys is not known at compile time, but their offset from the start of the static atoms array (mozilla::detail::gGkAtoms.mAtoms) _is_. We can use this information to build pointer-to-type associations. Imagine this utility function:
```
constexpr size_t AtomOffset(const nsStaticAtom* aAtom) {
return aAtom - mozilla::detail::gGkAtoms.mAtoms;
}
```
And this template struct + template alias combo:
```
namespace detail {
template <size_t AtomOffset>
struct GetAttrType {};
template <>
struct GetAttrType<AtomOffset(CacheKey::Name)> {
using type = nsString;
};
template <>
struct GetAttrType<AtomOffset(CacheKey::Name)> {
using type = uint64_t;
};
// ... and so on for each cache key ...
} // namespace detail
// utility type alias, akin to standard library type aliasing
template <size_t AtomOffset>
using GetAttrTypeT = typename detail::GetAttrType<AtomOffset>::type;
```
We could then get the attribute type like so:
```
using NameAttrType = GetAttrTypeT<AtomOffset(CacheKey::Name)>;
static_assert(std::is_same_v<NameAttrType, nsString>, "types are the same!");
```
This helps us build a member function in AccAttributes:
```
template <size_t AtomOffset>
decltype(auto) GetAttributeSafe() const {
return GetAttribute<GetAttrTypeT<AtomOffset>>(nsGkAtoms::GetAtomByIndex(AtomOffset));
}
```
which we can use like so in RemoteAccessible:
```
mCachedFields->GetAttributeSafe<AtomOffset(CacheKey::Name)>();
```
With that, we have the ability to safely (read: checked at compile time) get an attribute value for a cache key without having to specify the type at the call site, addressing (3) above. We can collect cache keys and their associated types in one place, centralizing how we add new cache keys. If the semantics of this are be a little grating, but we can improve things with a macro, which I'll detail in the next section.
### Ensuring All Requested Cache Keys are Domain-Checked before Querying
It would be nice to assert that the cache keys we're looking to query in a function have been domain-checked, without having to think too hard about it. I want to eliminate the class of error where someone adds a new GetAttribute call but forgets to add the attribute to the CHECK_ATTRIBUTE_DOMAINS_CACHED macro invocation, since that might mean they're looking for info in an uncached domain.
To solve this, we can expand the CHECK_ATTRIBUTE_DOMAINS_CACHED macro, adding:
```
const auto _CHECKER_FN = GetCheckerFunction({__VA_ARGS__});
```
where `GetCheckerFunction` is defined as such:
```
std::function<bool(const nsStaticAtom*)> GetCheckerFunction(
std::initializer_list<nsStaticAtom*> atoms) {
return [atoms](const nsStaticAtom* aAtom) {
for (auto atom : atoms) {
if (aAtom == atom) {
return true;
}
}
return false;
};
}
```
This puts a function at function scope that we can call to verify that the requested cache key has been domain-associated. I think this should probably only happen in debug builds, to avoid the std::function overhead. Consider this new RemoteAccessible function:
```
template <typename T>
Maybe<const T&> GetAttributeSafeInternal(
nsStaticAtom* aCacheKey,
const std::function<bool(nsStaticAtom*)> aCheckerFn) const {
MOZ_ASSERT(mCachedFields);
MOZ_ASSERT(
aCheckerFn(aCacheKey),
"Requested cache key is not domain-checked at the top of the calling "
"function. Please add it to the CHECK_ATTRIBUTES_DOMAIN_CACHED macro.");
return mCachedFields->GetAttribute<T>(aCacheKey);
}
```
This enables us to write the following macro for getting cache values:
```
#define GetAttributeSafe(cache_key) \
GetAttributeSafeInternal<GetAttrTypeT<AtomOffset(cache_key)>>(cache_key, _CHECKER_FN)
```
meaning we can write, simply:
```
GetAttributeSafe(CacheKey::Name);
```
and get all of the safety checks that we're hoping for.
### Putting it Together
Take the current implementation of RemoteAccessible::IsClipped:
```
bool RemoteAccessible::IsClipped() const {
MOZ_ASSERT(mCachedFields);
if (mCachedFields->GetAttribute<bool>(nsGkAtoms::clip_rule)) {
return true;
}
return false;
}
```
With this above utilities, we could write:
```
bool RemoteAccessible::IsClipped() const {
CHECK_ATTRIBUTE_DOMAINS_CACHED(false, CacheKey::IsClipped);
if (GetAttributeSafe(CacheKey::IsClipped)) {
return true;
}
return false;
}
```
I'm not sold on any of these ideas, but I think some subset of them might be useful and would like peoples' thoughts on them.
Bug 1794974 Comment 7 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
I've been thinking about how we can improve our attribute querying in light of how we plan to split up cache domains. There are a few classes of bugs I'd like to make difficult to write:
- Requesting a value for a cache key associated with an inactive domain
- Forgetting to properly request the relevant domains for the function at hand
- Passing the wrong template type for the given cache key to GetAttribute
There are a few things I'd like to accomplish to address these issues:
1. Make it easy to get domains associated with cache keys
a. Do this in a constexpr way so we don't incur any runtime penalty (hash lookups or memory) and don't have to worry about static initialization
b. Do this while also using the cache key static atom aliases, for readability
2. Make it easy to request domains and return early if any of the domains associated with cache keys are not active
3. Make it easy to get the data type associated with each cache key
4. Enforce the rule that any requested cache key must be domain-checked before querying it. That is, don't let ourselves request a cache key without ensuring that the associated domain is already active.
I've been toying around and proving out ideas to address these hopes.
### Getting and Requesting Cache Domains
To get the domain associated with each cache key, I'd like to introduce a function of this form:
```
constexpr uint64_t GetCacheDomainForKey(const nsStaticAtom* aCacheKey) {
if (aCacheKey == CacheKey::Name) {
return CacheDomain::NameAndDescription;
} else if (aCacheKey == CacheKey::State) {
return CacheDomain::State;
} else if (/* and so on... */) {}
}
```
Because static atoms are comparable at compile time, we can write
```
constexpr uint64_t domainForCacheKey = GetCacheDomainForKey(CacheKey::Name);
```
and resolve this domain at compile time, without the need for a hash table or other runtime lookup.
For any number of cache keys, we could use something like:
```
constexpr uint64_t GetRequiredCacheDomains(
std::initializer_list<nsStaticAtom*> aAttributes) {
uint64_t requiredCacheDomains = 0;
for (nsStaticAtom* attribute : aAttributes) {
requiredCacheDomains |= GetCacheDomainForKey(attribute);
}
return requiredCacheDomains;
}
```
Then,
```
constexpr uint64_t requiredCacheDomains = GetRequiredCacheDomains(CacheKey::Name, CacheKey::State);
```
also resolves at compile time.
Next, assume we have a function `RequestDomainsIfMissing` which takes the required cache domains and compares them to the active cache domains, sending off requests to all content processes and returning true if any domains were missing. We can then write a macro like so (name WIP) which returns the "domain not cached" result and fires off requests to content if any associated domain is missing:
```
#define CHECK_ATTRIBUTE_DOMAINS_CACHED(aAttributesNotCachedResult, ...) \
{ \
constexpr uint64_t requiredCacheDomains = \
GetRequiredCacheDomains({__VA_ARGS__}); \
if (RequestDomainsIfMissing(requiredCacheDomains)) { \
return aAttributesNotCachedResult; \
} \
}
```
This macro can be invoked at the top of every RemoteAccessible function that requests something from the cache. For instance:
```
CHECK_ATTRIBUTE_DOMAINS_CACHED(false, CacheKey::Name, CacheKey::State)
```
I think marshaling all attributes into a domain, even if that domain is an "Always" domain or similar, would help us here. This solves (1) and (2) above.
### Getting the Data Type Associated with each Cache Key
(3) Is not a directly pressing problem, but I think it's worth solving. Crucially, we _know_ the data types associated with each cache key at compile time. We open ourselves up to really annoying-to-hunt-down programming mistakes that we could solve entirely at compile time. The actual address of cache keys is not known at compile time, but their offset from the start of the static atoms array (mozilla::detail::gGkAtoms.mAtoms) _is_. We can use this information to build pointer-to-type associations. Imagine this utility function:
```
constexpr size_t AtomOffset(const nsStaticAtom* aAtom) {
return aAtom - mozilla::detail::gGkAtoms.mAtoms;
}
```
And this template struct + template alias combo:
```
namespace detail {
template <size_t AtomOffset>
struct GetAttrType {};
template <>
struct GetAttrType<AtomOffset(CacheKey::Name)> {
using type = nsString;
};
template <>
struct GetAttrType<AtomOffset(CacheKey::State)> {
using type = uint64_t;
};
// ... and so on for each cache key ...
} // namespace detail
// utility type alias, akin to standard library type aliasing
template <size_t AtomOffset>
using GetAttrTypeT = typename detail::GetAttrType<AtomOffset>::type;
```
We could then get the attribute type like so:
```
using NameAttrType = GetAttrTypeT<AtomOffset(CacheKey::Name)>;
static_assert(std::is_same_v<NameAttrType, nsString>, "types are the same!");
```
This helps us build a member function in AccAttributes:
```
template <size_t AtomOffset>
decltype(auto) GetAttributeSafe() const {
return GetAttribute<GetAttrTypeT<AtomOffset>>(nsGkAtoms::GetAtomByIndex(AtomOffset));
}
```
which we can use like so in RemoteAccessible:
```
mCachedFields->GetAttributeSafe<AtomOffset(CacheKey::Name)>();
```
With that, we have the ability to safely (read: checked at compile time) get an attribute value for a cache key without having to specify the type at the call site, addressing (3) above. We can collect cache keys and their associated types in one place, centralizing how we add new cache keys. If the semantics of this are be a little grating, but we can improve things with a macro, which I'll detail in the next section.
### Ensuring All Requested Cache Keys are Domain-Checked before Querying
It would be nice to assert that the cache keys we're looking to query in a function have been domain-checked, without having to think too hard about it. I want to eliminate the class of error where someone adds a new GetAttribute call but forgets to add the attribute to the CHECK_ATTRIBUTE_DOMAINS_CACHED macro invocation, since that might mean they're looking for info in an uncached domain.
To solve this, we can expand the CHECK_ATTRIBUTE_DOMAINS_CACHED macro, adding:
```
const auto _CHECKER_FN = GetCheckerFunction({__VA_ARGS__});
```
where `GetCheckerFunction` is defined as such:
```
std::function<bool(const nsStaticAtom*)> GetCheckerFunction(
std::initializer_list<nsStaticAtom*> atoms) {
return [atoms](const nsStaticAtom* aAtom) {
for (auto atom : atoms) {
if (aAtom == atom) {
return true;
}
}
return false;
};
}
```
This puts a function at function scope that we can call to verify that the requested cache key has been domain-associated. I think this should probably only happen in debug builds, to avoid the std::function overhead. Consider this new RemoteAccessible function:
```
template <typename T>
Maybe<const T&> GetAttributeSafeInternal(
nsStaticAtom* aCacheKey,
const std::function<bool(nsStaticAtom*)> aCheckerFn) const {
MOZ_ASSERT(mCachedFields);
MOZ_ASSERT(
aCheckerFn(aCacheKey),
"Requested cache key is not domain-checked at the top of the calling "
"function. Please add it to the CHECK_ATTRIBUTES_DOMAIN_CACHED macro.");
return mCachedFields->GetAttribute<T>(aCacheKey);
}
```
This enables us to write the following macro for getting cache values:
```
#define GetAttributeSafe(cache_key) \
GetAttributeSafeInternal<GetAttrTypeT<AtomOffset(cache_key)>>(cache_key, _CHECKER_FN)
```
meaning we can write, simply:
```
GetAttributeSafe(CacheKey::Name);
```
and get all of the safety checks that we're hoping for.
### Putting it Together
Take the current implementation of RemoteAccessible::IsClipped:
```
bool RemoteAccessible::IsClipped() const {
MOZ_ASSERT(mCachedFields);
if (mCachedFields->GetAttribute<bool>(nsGkAtoms::clip_rule)) {
return true;
}
return false;
}
```
With this above utilities, we could write:
```
bool RemoteAccessible::IsClipped() const {
CHECK_ATTRIBUTE_DOMAINS_CACHED(false, CacheKey::IsClipped);
if (GetAttributeSafe(CacheKey::IsClipped)) {
return true;
}
return false;
}
```
I'm not sold on any of these ideas, but I think some subset of them might be useful and would like peoples' thoughts on them.
I've been thinking about how we can improve our attribute querying in light of how we plan to split up cache domains. There are a few classes of bugs I'd like to make difficult to write:
- Requesting a value for a cache key associated with an inactive domain
- Forgetting to properly request the relevant domains for the function at hand
- Passing the wrong template type for the given cache key to GetAttribute
There are a few things I'd like to accomplish to address these issues:
1. Make it easy to get domains associated with cache keys
a. Do this in a constexpr way so we don't incur any runtime penalty (hash lookups or memory) and don't have to worry about static initialization
b. Do this while also using the cache key static atom aliases, for readability
2. Make it easy to request domains and return early if any of the domains associated with cache keys are not active
3. Make it easy to get the data type associated with each cache key
4. Enforce the rule that any requested cache key must be domain-checked before querying it. That is, don't let ourselves request a cache key without ensuring that the associated domain is already active.
I've been toying around and proving out ideas to address these hopes.
### Getting and Requesting Cache Domains
To get the domain associated with each cache key, I'd like to introduce a function of this form:
```
constexpr uint64_t GetCacheDomainForKey(const nsStaticAtom* aCacheKey) {
if (aCacheKey == CacheKey::Name) {
return CacheDomain::NameAndDescription;
} else if (aCacheKey == CacheKey::State) {
return CacheDomain::State;
} else if (/* and so on... */) {}
}
```
Because static atoms are comparable at compile time, we can write
```
constexpr uint64_t domainForCacheKey = GetCacheDomainForKey(CacheKey::Name);
```
and resolve this domain at compile time, without the need for a hash table or other runtime lookup.
For any number of cache keys, we could use something like:
```
constexpr uint64_t GetRequiredCacheDomains(
std::initializer_list<nsStaticAtom*> aCacheKeys) {
uint64_t requiredCacheDomains = 0;
for (nsStaticAtom* cacheKey : aCacheKeys) {
requiredCacheDomains |= GetCacheDomainForKey(cacheKey);
}
return requiredCacheDomains;
}
```
Then,
```
constexpr uint64_t requiredCacheDomains = GetRequiredCacheDomains(CacheKey::Name, CacheKey::State);
```
also resolves at compile time.
Next, assume we have a function `RequestDomainsIfMissing` which takes the required cache domains and compares them to the active cache domains, sending off requests to all content processes and returning true if any domains were missing. We can then write a macro like so (name WIP) which returns the "domain not cached" result and fires off requests to content if any associated domain is missing:
```
#define CHECK_ATTRIBUTE_DOMAINS_CACHED(aAttributesNotCachedResult, ...) \
{ \
constexpr uint64_t requiredCacheDomains = \
GetRequiredCacheDomains({__VA_ARGS__}); \
if (RequestDomainsIfMissing(requiredCacheDomains)) { \
return aAttributesNotCachedResult; \
} \
}
```
This macro can be invoked at the top of every RemoteAccessible function that requests something from the cache. For instance:
```
CHECK_ATTRIBUTE_DOMAINS_CACHED(false, CacheKey::Name, CacheKey::State)
```
I think marshaling all attributes into a domain, even if that domain is an "Always" domain or similar, would help us here. This solves (1) and (2) above.
### Getting the Data Type Associated with each Cache Key
(3) Is not a directly pressing problem, but I think it's worth solving. Crucially, we _know_ the data types associated with each cache key at compile time. We open ourselves up to really annoying-to-hunt-down programming mistakes that we could solve entirely at compile time. The actual address of cache keys is not known at compile time, but their offset from the start of the static atoms array (mozilla::detail::gGkAtoms.mAtoms) _is_. We can use this information to build pointer-to-type associations. Imagine this utility function:
```
constexpr size_t AtomOffset(const nsStaticAtom* aAtom) {
return aAtom - mozilla::detail::gGkAtoms.mAtoms;
}
```
And this template struct + template alias combo:
```
namespace detail {
template <size_t AtomOffset>
struct GetAttrType {};
template <>
struct GetAttrType<AtomOffset(CacheKey::Name)> {
using type = nsString;
};
template <>
struct GetAttrType<AtomOffset(CacheKey::State)> {
using type = uint64_t;
};
// ... and so on for each cache key ...
} // namespace detail
// utility type alias, akin to standard library type aliasing
template <size_t AtomOffset>
using GetAttrTypeT = typename detail::GetAttrType<AtomOffset>::type;
```
We could then get the attribute type like so:
```
using NameAttrType = GetAttrTypeT<AtomOffset(CacheKey::Name)>;
static_assert(std::is_same_v<NameAttrType, nsString>, "types are the same!");
```
This helps us build a member function in AccAttributes:
```
template <size_t AtomOffset>
decltype(auto) GetAttributeSafe() const {
return GetAttribute<GetAttrTypeT<AtomOffset>>(nsGkAtoms::GetAtomByIndex(AtomOffset));
}
```
which we can use like so in RemoteAccessible:
```
mCachedFields->GetAttributeSafe<AtomOffset(CacheKey::Name)>();
```
With that, we have the ability to safely (read: checked at compile time) get an attribute value for a cache key without having to specify the type at the call site, addressing (3) above. We can collect cache keys and their associated types in one place, centralizing how we add new cache keys. The semantics of this might be a little grating, but we can improve things with a macro, which I'll detail in the next section.
### Ensuring All Requested Cache Keys are Domain-Checked before Querying
It would be nice to assert that the cache keys we're looking to query in a function have been domain-checked, without having to think too hard about it. I want to eliminate the class of error where someone adds a new GetAttribute call but forgets to add the attribute to the CHECK_ATTRIBUTE_DOMAINS_CACHED macro invocation, since that might mean they're looking for info in an uncached domain.
To solve this, we can expand the CHECK_ATTRIBUTE_DOMAINS_CACHED macro, adding:
```
const auto _CHECKER_FN = GetCheckerFunction({__VA_ARGS__});
```
where `GetCheckerFunction` is defined as such:
```
std::function<bool(const nsStaticAtom*)> GetCheckerFunction(
std::initializer_list<nsStaticAtom*> atoms) {
return [atoms](const nsStaticAtom* aAtom) {
for (auto atom : atoms) {
if (aAtom == atom) {
return true;
}
}
return false;
};
}
```
This puts a function at function scope that we can call to verify that the requested cache key has been domain-associated. I think this should probably only happen in debug builds, to avoid the std::function overhead. Consider this new RemoteAccessible function:
```
template <typename T>
Maybe<const T&> GetAttributeSafeInternal(
nsStaticAtom* aCacheKey,
const std::function<bool(nsStaticAtom*)> aCheckerFn) const {
MOZ_ASSERT(mCachedFields);
MOZ_ASSERT(
aCheckerFn(aCacheKey),
"Requested cache key is not domain-checked at the top of the calling "
"function. Please add it to the CHECK_ATTRIBUTES_DOMAIN_CACHED macro.");
return mCachedFields->GetAttribute<T>(aCacheKey);
}
```
This enables us to write the following macro for getting cache values:
```
#define GetAttributeSafe(cache_key) \
GetAttributeSafeInternal<GetAttrTypeT<AtomOffset(cache_key)>>(cache_key, _CHECKER_FN)
```
meaning we can write, simply:
```
GetAttributeSafe(CacheKey::Name);
```
and get all of the safety checks that we're hoping for.
### Putting it Together
Take the current implementation of RemoteAccessible::IsClipped:
```
bool RemoteAccessible::IsClipped() const {
MOZ_ASSERT(mCachedFields);
if (mCachedFields->GetAttribute<bool>(nsGkAtoms::clip_rule)) {
return true;
}
return false;
}
```
With this above utilities, we could write:
```
bool RemoteAccessible::IsClipped() const {
CHECK_ATTRIBUTE_DOMAINS_CACHED(false, CacheKey::IsClipped);
if (GetAttributeSafe(CacheKey::IsClipped)) {
return true;
}
return false;
}
```
I'm not sold on any of these ideas, but I think some subset of them might be useful and would like peoples' thoughts on them.
I've been thinking about how we can improve our attribute querying in light of how we plan to split up cache domains. There are a few classes of bugs I'd like to make difficult to write:
- Requesting a value for a cache key associated with an inactive domain
- Forgetting to properly request the relevant domains for the function at hand
- Passing the wrong template type for the given cache key to GetAttribute
There are a few things I'd like to accomplish to address these issues:
1. Make it easy to get domains associated with cache keys
a. Do this in a constexpr way so we don't incur any runtime penalty (hash lookups or memory) and don't have to worry about static initialization
b. Do this while also using the cache key static atom aliases, for readability
2. Make it easy to request domains and return early if any of the domains associated with cache keys are not active
3. Make it easy to get the data type associated with each cache key
4. Enforce the rule that any requested cache key must be domain-checked before querying it. That is, don't let ourselves request a cache key without ensuring that the associated domain is already active.
I've been toying around and proving out ideas to address these hopes.
### Getting and Requesting Cache Domains
To get the domain associated with each cache key, I'd like to introduce a function of this form:
```
constexpr uint64_t GetCacheDomainForKey(const nsStaticAtom* aCacheKey) {
if (aCacheKey == CacheKey::Name) {
return CacheDomain::NameAndDescription;
} else if (aCacheKey == CacheKey::State) {
return CacheDomain::State;
} else if (/* and so on... */) {}
}
```
Because static atoms are comparable at compile time, we can write
```
constexpr uint64_t domainForCacheKey = GetCacheDomainForKey(CacheKey::Name);
```
and resolve this domain at compile time, without the need for a hash table or other runtime lookup.
For any number of cache keys, we could use something like:
```
constexpr uint64_t GetRequiredCacheDomains(
std::initializer_list<nsStaticAtom*> aCacheKeys) {
uint64_t requiredCacheDomains = 0;
for (nsStaticAtom* cacheKey : aCacheKeys) {
requiredCacheDomains |= GetCacheDomainForKey(cacheKey);
}
return requiredCacheDomains;
}
```
Then,
```
constexpr uint64_t requiredCacheDomains = GetRequiredCacheDomains(CacheKey::Name, CacheKey::State);
```
also resolves at compile time.
Next, assume we have a function `RequestDomainsIfMissing` which takes the required cache domains and compares them to the active cache domains, sending off requests to all content processes and returning true if any domains were missing. We can then write a macro like so (name WIP) which returns the "domain not cached" result and fires off requests to content if any associated domain is missing:
```
#define CHECK_ATTRIBUTE_DOMAINS_CACHED(aAttributesNotCachedResult, ...) \
{ \
constexpr uint64_t requiredCacheDomains = \
GetRequiredCacheDomains({__VA_ARGS__}); \
if (RequestDomainsIfMissing(requiredCacheDomains)) { \
return aAttributesNotCachedResult; \
} \
}
```
This macro can be invoked at the top of every RemoteAccessible function that requests something from the cache. For instance:
```
CHECK_ATTRIBUTE_DOMAINS_CACHED(false, CacheKey::Name, CacheKey::State)
```
I think marshaling all attributes into a domain, even if that domain is an "Always" domain or similar, would help us here. This solves (1) and (2) above.
### Getting the Data Type Associated with each Cache Key
(3) Is not a directly pressing problem, but I think it's worth solving. Crucially, we _know_ the data types associated with each cache key at compile time. We open ourselves up to really annoying-to-hunt-down programming mistakes that we could solve entirely at compile time. The actual address of cache keys is not known at compile time, but their offset from the start of the static atoms array (mozilla::detail::gGkAtoms.mAtoms) _is_. We can use this information to build pointer-to-type associations. Imagine this utility function:
```
constexpr size_t AtomOffset(const nsStaticAtom* aAtom) {
return aAtom - mozilla::detail::gGkAtoms.mAtoms;
}
```
And this template struct + template alias combo:
```
namespace detail {
template <size_t AtomOffset>
struct GetAttrType {};
template <>
struct GetAttrType<AtomOffset(CacheKey::Name)> {
using type = nsString;
};
template <>
struct GetAttrType<AtomOffset(CacheKey::State)> {
using type = uint64_t;
};
// ... and so on for each cache key ...
} // namespace detail
// utility type alias, akin to standard library type aliasing
template <size_t AtomOffset>
using GetAttrTypeT = typename detail::GetAttrType<AtomOffset>::type;
```
We could then get the attribute type like so:
```
using NameAttrType = GetAttrTypeT<AtomOffset(CacheKey::Name)>;
static_assert(std::is_same_v<NameAttrType, nsString>, "types are the same!");
```
This helps us build a member function in AccAttributes:
```
template <size_t AtomOffset>
decltype(auto) GetAttributeSafe() const {
return GetAttribute<GetAttrTypeT<AtomOffset>>(nsGkAtoms::GetAtomByIndex(AtomOffset));
}
```
which we can use like so in RemoteAccessible:
```
mCachedFields->GetAttributeSafe<AtomOffset(CacheKey::Name)>();
```
With that, we have the ability to safely (read: checked at compile time) get an attribute value for a cache key without having to specify the type at the call site, addressing (3) above. We can collect cache keys and their associated types in one place, centralizing how we add new cache keys. The semantics of this might be a little grating, but we can improve things with a macro, which I'll detail in the next section.
### Ensuring All Requested Cache Keys are Domain-Checked before Querying
It would be nice to assert that the cache keys we're looking to query in a function have been domain-checked, without having to think too hard about it. I want to eliminate the class of error where someone adds a new GetAttribute call but forgets to add the attribute to the CHECK_ATTRIBUTE_DOMAINS_CACHED macro invocation, since that might mean they're looking for info in an uncached domain.
To solve this, we can expand the CHECK_ATTRIBUTE_DOMAINS_CACHED macro, adding:
```
const auto _CHECKER_FN = GetCheckerFunction({__VA_ARGS__});
```
where `GetCheckerFunction` is defined as such:
```
std::function<bool(const nsStaticAtom*)> GetCheckerFunction(
std::initializer_list<nsStaticAtom*> atoms) {
return [atoms](const nsStaticAtom* aAtom) {
for (auto atom : atoms) {
if (aAtom == atom) {
return true;
}
}
return false;
};
}
```
This puts a function at function scope that we can call to verify that the requested cache key has been domain-associated. I think this should probably only happen in debug builds, to avoid the std::function overhead. Consider this new RemoteAccessible function:
```
template <typename T>
Maybe<const T&> GetAttributeSafeInternal(
nsStaticAtom* aCacheKey,
const std::function<bool(nsStaticAtom*)> aCheckerFn) const {
MOZ_ASSERT(mCachedFields);
MOZ_ASSERT(
aCheckerFn(aCacheKey),
"Requested cache key is not domain-checked at the top of the calling "
"function. Please add it to the CHECK_ATTRIBUTES_DOMAIN_CACHED macro.");
return mCachedFields->GetAttribute<T>(aCacheKey);
}
```
This enables us to write the following macro for getting cache values:
```
#define GetAttributeSafe(cache_key) \
GetAttributeSafeInternal<GetAttrTypeT<AtomOffset(cache_key)>>(cache_key, _CHECKER_FN)
```
meaning we can write, simply:
```
GetAttributeSafe(CacheKey::Name);
```
and get all of the safety checks that we're hoping for.
### Putting it Together
Take the current implementation of RemoteAccessible::IsClipped:
```
bool RemoteAccessible::IsClipped() const {
MOZ_ASSERT(mCachedFields);
if (mCachedFields->GetAttribute<bool>(nsGkAtoms::clip_rule)) {
return true;
}
return false;
}
```
With this above utilities, we could write:
```
bool RemoteAccessible::IsClipped() const {
CHECK_ATTRIBUTE_DOMAINS_CACHED(false, CacheKey::IsClipped);
if (GetAttributeSafe(CacheKey::IsClipped)) {
return true;
}
return false;
}
```
I'm not sold on any of these ideas, but I think some subset of them might be useful and would like peoples' thoughts on them.