The changeset has been generated automatically through:
./mach static-analysis check --output ./clang-tidy-bugprone.json --format json --fix
followed by manual adjustment when needed or when the auto-fixer bailed out.
This paves the way for Bug 2023527
Differential Revision: https://phabricator.services.mozilla.com/D287941
The changeset has been generated automatically through:
./mach static-analysis check --output ./clang-tidy-bugprone.json --format json --fix
followed by manual adjustment when needed or when the auto-fixer bailed out.
This paves the way for Bug 2023527
Differential Revision: https://phabricator.services.mozilla.com/D287941
It is replaced by a standard-conforming cast to (void) which is
understood the same by the compiler.
In some cases this could be replaced by `[[maybe_unused]]` but for
expression whose value is ignored, this requires storing to a temporary
variable first, so not going that way.
One notable change is that marking an object of type already_AddRefed as
unused used to implicitly leak memory. This is now made explicit.
Most of the heavy lifting was done through:
$ git grep -l '\<Unused\> <<' | xargs sed -i -e 's/Unused << /(void)/g'
$ git grep -l mozilla/Unused.h | xargs sed -i -e '/mozilla\/Unused.h/ d'
Differential Revision: https://phabricator.services.mozilla.com/D268706
It is replaced by a standard-conforming cast to (void) which is
understood the same by the compiler.
In some cases this could be replaced by `[[maybe_unused]]` but for
expression whose value is ignored, this requires storing to a temporary
variable first, so not going that way.
One notable change is that marking an object of type already_AddRefed as
unused used to implicitly leak memory. This is now made explicit.
Most of the heavy lifting was done through:
$ git grep -l '\<Unused\> <<' | xargs sed -i -e 's/Unused << /(void)/g'
$ git grep -l mozilla/Unused.h | xargs sed -i -e '/mozilla\/Unused.h/ d'
Differential Revision: https://phabricator.services.mozilla.com/D268706
It is replaced by a standard-conforming cast to (void) which is
understood the same by the compiler.
In some cases this could be replaced by `[[maybe_unused]]` but for
expression whose value is ignored, this requires storing to a temporary
variable first, so not going that way.
One notable change is that marking an object of type already_AddRefed as
unused used to implicitly leak memory. This is now made explicit.
Most of the heavy lifting was done through:
$ git grep -l '\<Unused\> <<' | xargs sed -i -e 's/Unused << /(void)/g'
$ git grep -l mozilla/Unused.h | xargs sed -i -e '/mozilla\/Unused.h/ d'
Differential Revision: https://phabricator.services.mozilla.com/D268706
It is replaced by a standard-conforming cast to (void) which is
understood the same by the compiler.
In some cases this could be replaced by `[[maybe_unused]]` but for
expression whose value is ignored, this requires storing to a temporary
variable first, so not going that way.
One notable change is that marking an object of type already_AddRefed as
unused used to implicitly leak memory. This is now made explicit.
Most of the heavy lifting was done through:
$ git grep -l '\<Unused\> <<' | xargs sed -i -e 's/Unused << /(void)/g'
$ git grep -l mozilla/Unused.h | xargs sed -i -e '/mozilla\/Unused.h/ d'
Differential Revision: https://phabricator.services.mozilla.com/D268706
Previously, if there was an anchor jump to a DOM node that wasn't yet rendered in the accessibility tree, we could fail in one of two ways:
1. If none of the target's ancestors (other than the document itself) were in the accessibility tree, we wouldn't fire a scrolling start event until the document was next re-focused.
2. If the target did have a container already in the accessibility tree, we would fire a scrolling start event on the container, which might effectively be a long way from the target.
To fix this, if there is a pending tree update, process the anchor jump only after the tree is up to date (in WillRefresh).
As part of this, anchor jump processing has been refactored into a single, unified function.
Aside from avoiding additional code duplication, this ensures that anchor jump handling is consistent going forward, as there are quite a few subtle details to get right which have bitten us in the past; e..g bug 1926198.
Differential Revision: https://phabricator.services.mozilla.com/D255274
This revision removes unnecessary include directives from cpp files in the
accessible/base directory. These suggestions came from the Include What You Use
tool.
Differential Revision: https://phabricator.services.mozilla.com/D182288
When the Accessible is recreated for the focused node, DocAccessible::CreateSubtree calls FocusManager::DispatchFocusEvent directly.
If FocusManager::mActiveItem was set, DispatchFocusEvent previously didn't clear it, even though DispatchFocusEvent was given a new target overriding mActiveItem.
This meant that the old mActiveItem would remain until it was next set or cleared, causing assertions if it died and potentially other problems.
To fix this, DispatchFocusEvent clears mActiveItem if it is different to the target, since the target should override.
Differential Revision: https://phabricator.services.mozilla.com/D181605
This revision changes our focus event and anchor scroll handling to fire
scrolling start events for non-interactive targets, where "non-interactive" is
defined as "any generic, landmark, or region Accessible." This revision also
adds a test to verify that the scrolling start event is fired.
Differential Revision: https://phabricator.services.mozilla.com/D181329
Move most the event handling stuff to the DOM. I've left nsMenuBarFrame
for now, but I will be removing that in the future.
The basic set up is:
* nsMenuParent becomes XULMenuParentElement (menubar or popup, manages
the current active menu item)
* nsMenuFrame -> XULButtonElements that return true for IsMenu().
Can't use XULMenuElement because of <button type=menu>, which
behaves like a, well, menu.
This makes the a11y events for menus (DOMMenuItem{Active,Inactive}) make
sense (before that we were firing duplicate Inactive events etc, and the
event order was rather suspicious).
Differential Revision: https://phabricator.services.mozilla.com/D164210
Move most the event handling stuff to the DOM. I've left nsMenuBarFrame
for now, but I will be removing that in the future.
The basic set up is:
* nsMenuParent becomes XULMenuParentElement (menubar or popup, manages
the current active menu item)
* nsMenuFrame -> XULButtonElements that return true for IsMenu().
Can't use XULMenuElement because of <button type=menu>, which
behaves like a, well, menu.
This makes the a11y events for menus (DOMMenuItem{Active,Inactive}) make
sense (before that we were firing duplicate Inactive events etc, and the
event order was rather suspicious).
Differential Revision: https://phabricator.services.mozilla.com/D164210
Previously, IsFocused had a separate implementation instead of just using FocusedAccessible because of cases years ago where it would unintentionally cause us to create a DocAccessible for an initial about:blank document.
As far as I can tell, we no longer prevent creation of DocAccessibles for initial documents because they aren't necessarily temporary documents.
Therefore, we can just compare against FocusedAccessible now.
Since FocusedAccessible is already unified, this makes unifying IsFocused very simple.
This paves the way for unifying correct calculation of the focused state.
This necessitated a change to FocusedLocalAccessible to prevent it from trying to get/create a DocAccessible when the accessibility service is shutting down.
Shutting down the service shuts down all documents, and shutting down a document now calls IsFocused, which causes us to get/create a DocAccessible.
That isn't safe while the service is shutting down and was causing a crash.
Differential Revision: https://phabricator.services.mozilla.com/D161875
IN subsequent patches, IsFocused will be called when querying the state of a RemoteAccessible and IsFocused calls FocusedAccessible.
A RemoteAccessible can be queried from the Android UI thread (which is different to Gecko's main thread), but it's not safe to deal with LocalAccessibles off the main thread.
Therefore, don't try to call FocusedLocalAccessible in this case.
Differential Revision: https://phabricator.services.mozilla.com/D162177
Previously, IsFocused had a separate implementation instead of just using FocusedAccessible because of cases years ago where it would unintentionally cause us to create a DocAccessible for an initial about:blank document.
As far as I can tell, we no longer prevent creation of DocAccessibles for initial documents because they aren't necessarily temporary documents.
Therefore, we can just compare against FocusedAccessible now.
Since FocusedAccessible is already unified, this makes unifying IsFocused very simple.
This paves the way for unifying correct calculation of the focused state.
Differential Revision: https://phabricator.services.mozilla.com/D161875
IN subsequent patches, IsFocused will be called when querying the state of a RemoteAccessible and IsFocused calls FocusedAccessible.
A RemoteAccessible can be queried from the Android UI thread (which is different to Gecko's main thread), but it's not safe to deal with LocalAccessibles off the main thread.
Therefore, don't try to call FocusedLocalAccessible in this case.
Differential Revision: https://phabricator.services.mozilla.com/D162177
Previously, IsFocused had a separate implementation instead of just using FocusedAccessible because of cases years ago where it would unintentionally cause us to create a DocAccessible for an initial about:blank document.
As far as I can tell, we no longer prevent creation of DocAccessibles for initial documents because they aren't necessarily temporary documents.
Therefore, we can just compare against FocusedAccessible now.
Since FocusedAccessible is already unified, this makes unifying IsFocused very simple.
This paves the way for unifying correct calculation of the focused state.
Differential Revision: https://phabricator.services.mozilla.com/D161875
This moves the unified focus logic from DocAccessible::FocusedChild to FocusManager.
This is needed in order to support a unified IsFocusWithin method.
Differential Revision: https://phabricator.services.mozilla.com/D160879
Added an assert in focus manager. Hopefully fuzzers will help us find
cases where the active item is defunct, if that is indeed what is
happening.
Differential Revision: https://phabricator.services.mozilla.com/D109403
This is a collaboration between clang-tidy and clang-format.
clang-tidy uses compiled code paths, so this patch is linux specific.
Differential Revision: https://phabricator.services.mozilla.com/D105669
Print preview documents don't get DocAccessibles because this currently causes crashes and doesn't provide much value.
However, we still want to tell a11y clients something useful when a print preview document gets focus, rather than a11y focus just going nowhere.
Therefore, we allow a11y focus to land on the OuterDocAccessible (browser element) in this case.
Differential Revision: https://phabricator.services.mozilla.com/D87997
Before Fission, FocusManager::FocusedDOMNode rejected a11y focus on remote XUL browsers by checking EventStateManager::IsRemoteTarget.
In bug 1594623, code was added to FocusManager::ProcessDOMFocus to prevent a11y focus on OuterDocAccessibles in order to reject focus on OOP iframes.
In bug 1635784, EventStateManager::IsRemoteTarget was renamed to EventStateManager::IsTopLevelRemoteTarget, and EventStateManager::IsRemoteTarget now checks for OOP iframes as well.
This allows us to unify rejection of a11y focus on remote OuterDocAccessibles in FocusManager::FocusedDOMNode.
Differential Revision: https://phabricator.services.mozilla.com/D87996
Print preview documents don't get DocAccessibles because this currently causes crashes and doesn't provide much value.
However, we still want to tell a11y clients something useful when a print preview document gets focus, rather than a11y focus just going nowhere.
Therefore, we allow a11y focus to land on the OuterDocAccessible (browser element) in this case.
Differential Revision: https://phabricator.services.mozilla.com/D87997
Before Fission, FocusManager::FocusedDOMNode rejected a11y focus on remote XUL browsers by checking EventStateManager::IsRemoteTarget.
In bug 1594623, code was added to FocusManager::ProcessDOMFocus to prevent a11y focus on OuterDocAccessibles in order to reject focus on OOP iframes.
In bug 1635784, EventStateManager::IsRemoteTarget was renamed to EventStateManager::IsTopLevelRemoteTarget, and EventStateManager::IsRemoteTarget now checks for OOP iframes as well.
This allows us to unify rejection of a11y focus on remote OuterDocAccessibles in FocusManager::FocusedDOMNode.
Differential Revision: https://phabricator.services.mozilla.com/D87996
nsFocusManager::GetFocusedWindow may return a window even if focus is actually inside an embedded OOP iframe.
Instead, use nsFocusManager::GetFocusedBrowsingContext, which always knows about the current focus across all processes.
If the focused context is in this process, we get its document and return it.
Differential Revision: https://phabricator.services.mozilla.com/D64514
Previously, the following scenario was possible:
1. aria-activedescendant is used on a menupopup to set a11y focus. When the event is processed, FocusManager::mLastFocus gets set accordingly.
2. aria-activedescendant on the menupopup is changed, so a focus event gets queued.
3. Before that focus event can be processed, the Accessible in (1) gets unbound from the a11y tree.
4. DocAccessible::UnbindFromDocument checks whether the Accessible is the last focus. As per (1), it is.
5. It then tells FocusManager to reset the active item, causing it to throw away the active item set in (2) and fire focus on the DOM focus (which is not the menupopup).
This breaks the new select dropdown implementation.
To fix this, we now set mLastFocus in DispatchFocusEvent (when the event is queued) instead of ProcessFocusEvent.
This way, we don't override a pending new focus change unintentionally.
Differential Revision: https://phabricator.services.mozilla.com/D65706
Previously, we only restored focus within the top level remote browser.
This is fine for remote documents without iframes or only in-process iframes, as was the case with e10s.
For Fission, if an OOP iframe has focus, we need to get the appropriate nested remote browser.
Fortunately, BrowserParent::GetFocused was introduced to do exactly this.
Differential Revision: https://phabricator.services.mozilla.com/D52803
An OuterDoc shouldn't get accessibility focus itself.
Focus should always go to something inside it.
However, OOP iframes will get DOM focus because their content isn't in this process.
We now suppress the OuterDoc focus in this case.
The OOP browser will fire focus for the correct Accessible inside the embedded document.
Without suppression of the OuterDoc focus, the two focus events will race and the OuterDoc focus may override the correct embedded focus for accessibility clients.
Even if they fired in the correct order, clients may report extraneous focus information to the user before reporting the correct focus.
Differential Revision: https://phabricator.services.mozilla.com/D52126
In FocusManager::ProcessFocusEvent, after firing the event, we get the anchor jump from the target Accessible's document.
However, it seems the Accessible can be shut down during the call to nsEventShell::FireEvent, resulting in a crash when we try to get the anchor jump.
Protect against this by checking whether the target is defunct after firing the event.
Differential Revision: https://phabricator.services.mozilla.com/D39450
If the DOM focus is removed before something else is focused, the document gets DOM focus, but no blur event is fired (bug 559561).
This means that no a11y focus event is fired, so clients aren't notified.
This is particularly problematic for screen readers when dismissing some ARIA dialogs, as the screen reader doesn't know that focus has returned to the top level document.
Differential Revision: https://phabricator.services.mozilla.com/D31024
Summary: Really sorry for the size of the patch. It's mostly automatic
s/nsIDocument/Document/ but I had to fix up in a bunch of places manually to
add the right namespacing and such.
Overall it's not a very interesting patch I think.
nsDocument.cpp turns into Document.cpp, nsIDocument.h into Document.h and
nsIDocumentInlines.h into DocumentInlines.h.
I also changed a bunch of nsCOMPtr usage to RefPtr, but not all of it.
While fixing up some of the bits I also removed some unneeded OwnerDoc() null
checks and such, but I didn't do anything riskier than that.
Eitan Isaacson