1743749 - Clean up use of atoms for cache keys

Status: RESOLVED FIXED

(Core :: Disability Access APIs, task)

Description

[James Teh [:Jamie] (away returning 3 Oct)]

We use AccAttributes for the CTW cache and AccAttributes is keyed by nsAtom. This has some advantages as compared to an enum: we don't need separate data structures for object/text attributes and the CTW cache and we get readable logging for the keys for free. However, a major disadvantage is that because we try to avoid polluting the nsGkAtoms namespace, we end up with some unclear/unintuitive keys; e.g. nsGkAtoms::line for text line start boundaries, nsGkAtoms::style for text attributes, etc.

We need to find a way to make this more obvious and readable. We can either alias the atoms in our own namespace or switch to using an enum after all.

I looked into various approaches of aliasing. We can't use an enum because enums have to be convertable to int. We can't use a using declaration because they only work for types, not values. What does work is this:

class CacheKey {
 public:
  static constexpr nsStaticAtom* TextLineStarts = nsGkAtoms::line;
};

Comment 1

[James Teh [:Jamie] (away returning 3 Oct)]

Attachment: Bug 1743749 part 1: Create aliases for RemoteAccessible cache keys.

Comment 2

[James Teh [:Jamie] (away returning 3 Oct)]

Attachment: Bug 1743749 part 2: Replace usage of atoms as cache keys with the new CacheKey aliases.

This was done with the following Python script:

import re

cacheConsts = open("accessible/base/CacheConstants.h", "rt").read()
aliases = {
  alias: atom
  for atom, alias in
  re.findall(
    r'static constexpr nsStaticAtom\*\s+(.*?)\s+=\s+(nsGkAtoms::.*?);',
    cacheConsts
  )
}

RE_ATOM = re.compile(r'(fields->SetAttribute|(?:mCachedFields|aFields)->(?:GetAttribute|GetAttributeRefPtr|GetMutableAttribute|HasAttribute|Remove|SetAttribute)(?:<.+>)?)(\(\s*)(nsGkAtoms::[a-zA-Z_]+)')

def repl(m):
  # Group 3 is the atom.
  alias = aliases.get(m.group(3))
  if not alias:
    # No alias for this atom. Return input unaltered.
    return m.group(0)
  alias = "CacheKey::" + alias
  # Groups 1 and 2 should be returned unaltered. Group 3 (the atom) is replaced
  # with the alias.
  return m.group(1) + m.group(2) + alias

for fn in (
  # Found with: git grep -l 'ields->'
  "accessible/base/CachedTableAccessible.cpp",
  "accessible/base/nsAccessibilityService.cpp",
  "accessible/base/TextLeafRange.cpp",
  "accessible/generic/LocalAccessible.cpp",
  "accessible/ipc/DocAccessibleParent.cpp",
  "accessible/ipc/RemoteAccessible.cpp",
  "accessible/ipc/RemoteAccessible.h",
  "accessible/windows/sdn/sdnAccessible.cpp",
):
  input = open(fn, "rt").read()
  output = RE_ATOM.sub(repl, input)
  open(fn, "wt").write(output)

Comment 3

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

The following patches are waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D184667	Bug 1743749 part 1: Create aliases for RemoteAccessible cache keys.	Jamie	morgan: Back Aug 7, 2023
D184791	Bug 1743749 part 2: Replace usage of atoms as cache keys with the new CacheKey aliases.	Jamie	morgan: Back Aug 7, 2023

:Jamie, could you please find another reviewer?

For more information, please visit BugBot documentation.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/922edbc3a604
part 1: Create aliases for RemoteAccessible cache keys. r=nlapre
https://hg.mozilla.org/integration/autoland/rev/662d10a1b3a3
part 2: Replace usage of atoms as cache keys with the new CacheKey aliases. r=nlapre

Comment 5

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/922edbc3a604
https://hg.mozilla.org/mozilla-central/rev/662d10a1b3a3