Closed
Bug 1479034
Opened 6 years ago
Closed 6 years ago
Make GeckoView's SessionAccessibility a JNIObject associated with a nsWindow.
Categories
(Core :: Disability Access APIs, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: eeejay, Assigned: eeejay)
References
Details
Attachments
(1 file, 1 obsolete file)
26.25 KB,
patch
|
jchen
:
review+
|
Details | Diff | Splinter Review |
Each SessionAccessibility instance should be associated with an nsWindow that references the session's root accessible.
Assignee | ||
Comment 1•6 years ago
|
||
We could remove the attach functions if SessionAccessibility wasn't lazily instantiated.
Attachment #9009288 -
Flags: review?(nchen)
Comment 2•6 years ago
|
||
Comment on attachment 9009288 [details] [diff] [review] Make GeckoView's SessionAccessibility a JNIObject associated with a nsWindow. r?jchen Review of attachment 9009288 [details] [diff] [review]: ----------------------------------------------------------------- Looks good with only a few nits. ::: accessible/android/SessionAccessibility.cpp @@ +2,5 @@ > + * This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include <stdio.h> Not used? ::: accessible/android/SessionAccessibility.h @@ +7,5 @@ > +#define mozilla_a11y_SessionAccessibility_h_ > + > +#include "AndroidUiThread.h" > +#include "GeneratedJNINatives.h" > +#include "nsAppShell.h" Not used? @@ +12,5 @@ > +#include "nsThreadUtils.h" > +#include "nsWindow.h" > + > +#include <android/log.h> > +#define AALOG(args...) \ `#if DEBUG`. Also please define this in a .cpp file rather than in a header. @@ +33,5 @@ > + { > + if (RefPtr<nsThread> uiThread = GetAndroidUiThread()) { > + java::SessionAccessibility::GlobalRef sa(mSessionAccessibility); > + uiThread->Dispatch(NS_NewRunnableFunction( > + "SessionAccessibility::Attach", [sa] { sa->SetAttached(true); })); `[sa = java::SessionAccessibility::GlobalRef(mSessionAccessibility)]` and no need for separate `sa` declaration above @@ +42,5 @@ > + { > + if (RefPtr<nsThread> uiThread = GetAndroidUiThread()) { > + java::SessionAccessibility::GlobalRef sa(mSessionAccessibility); > + uiThread->Dispatch(NS_NewRunnableFunction( > + "SessionAccessibility::OnDetach", [sa] { sa->SetAttached(false); })); Same here. It's probably worth combining this function and the constructor since they share much of the code. @@ +56,5 @@ > +private: > + ~SessionAccessibility() {} > + > + nsWindow::WindowPtr<SessionAccessibility> mWindow; // Parent only > + java::SessionAccessibility::GlobalRef mSessionAccessibility; Probably need to switch to a weak pointer later, with the work :snorp is doing (bug 1486596). ::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java @@ +792,5 @@ > public native void attachEditable(IGeckoEditableParent parent, > GeckoEditableChild child); > > + @WrapForJNI(dispatchTo = "proxy") > + private native void attachAccessibility(SessionAccessibility SessionAccessibility); `public`. Also parameter name should be lowercase (sessionAccessibility) @@ +1070,5 @@ > * > * @return SessionAccessibility instance. > */ > public @NonNull SessionAccessibility getAccessibility() { > if (mAccessibility == null) { Early return here, i.e. `if (mAccessibility != null) { return mAccessibility; }` Should also add a `ThreadUtils.assertOnUiThread();` above. ::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/SessionAccessibility.java @@ +32,5 @@ > import android.view.accessibility.AccessibilityManager; > import android.view.accessibility.AccessibilityNodeInfo; > import android.view.accessibility.AccessibilityNodeProvider; > > +public class SessionAccessibility extends JNIObject { Because `SessionAccessibility` is public API, instead of having it directly extend JNIObject, we normally prefer having a private inner class that extends JNIObject. ::: widget/android/nsWindow.cpp @@ +296,5 @@ > // Close and destroy the nsWindow. > void Close(); > > + void BindAccessibility(const GeckoSession::Window::LocalRef& inst, > + jni::Object::Param aSessionAccessibility); Not used? Seems like this would do the same thing as `AttachAccessibility` @@ +1145,5 @@ > window.mLayerViewSupport.Detach(); > } > + > + if (window.mSessionAccessibility) { > + window.mSessionAccessibility.Detach(); Four space indent @@ +1265,5 @@ > MOZ_ASSERT(window.mAndroidView); > window.mAndroidView->mEventDispatcher->Attach( > java::EventDispatcher::Ref::From(aDispatcher), mDOMWindow); > > + if (aSessionAccessibility) { We should call `window.mSessionAccessibility.Detach()` if needed, even if `aSessionAccessibility` is null here. @@ +1302,5 @@ > window.mEditableParent = aEditableParent; > } > > +void > +nsWindow::GeckoViewSupport::AttachAccessibility(const GeckoSession::Window::LocalRef& inst, I would just incorporate this into `GeckoViewSupport::Transfer` itself ::: widget/android/nsWindow.h @@ +189,5 @@ > // Strong referenced by the Java instance. > NativePtr<mozilla::widget::GeckoEditableSupport> mEditableSupport; > mozilla::jni::Object::GlobalRef mEditableParent; > > + NativePtr<mozilla::a11y::SessionAccessibility> mSessionAccessibility; Please add a comment about ownership (similar to the other members above)
Attachment #9009288 -
Flags: review?(nchen) → feedback+
Assignee | ||
Comment 3•6 years ago
|
||
(In reply to Jim Chen [:jchen] [:darchons] from comment #2) > @@ +1302,5 @@ > > window.mEditableParent = aEditableParent; > > } > > > > +void > > +nsWindow::GeckoViewSupport::AttachAccessibility(const GeckoSession::Window::LocalRef& inst, > > I would just incorporate this into `GeckoViewSupport::Transfer` itself I'm not sure what you mean. Get rid of AttachAccessibility? We need it for when the window is opened before a SessionAccessibility is created.
Assignee | ||
Comment 4•6 years ago
|
||
Here is another attempt!
Attachment #9010794 -
Flags: review?(nchen)
Assignee | ||
Updated•6 years ago
|
Attachment #9009288 -
Attachment is obsolete: true
Comment 5•6 years ago
|
||
Comment on attachment 9010794 [details] [diff] [review] Make GeckoView's SessionAccessibility a JNIObject associated with a nsWindow. r?jchen Review of attachment 9010794 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Thanks! ::: accessible/android/SessionAccessibility.h @@ +12,5 @@ > +namespace mozilla { > +namespace a11y { > + > +class SessionAccessibility final > + : public java::SessionAccessibility::NativeProvider::Natives< Usually you would put these in one line; same below. ::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java @@ +1092,5 @@ > + mWindow.attachAccessibility(mAccessibility.getNativeProvider()); > + } else { > + GeckoThread.queueNativeCallUntil(GeckoThread.State.PROFILE_READY, > + mWindow, "attachAccessibility", > + SessionAccessibility.class, mAccessibility.getNativeProvider()); `SessionAccessibility.NativeProvider.class` ::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/SessionAccessibility.java @@ +234,5 @@ > /* package */ final GeckoSession mSession; > // This is the view that delegates accessibility to us. We also sends event through it. > private View mView; > + // The native portion of the node provider. > + final private NativeProvider mNativeProvider = new NativeProvider(); `/* package */ final NativeProvider nativeProvider` would also be fine if you want to get rid of `getNativeProvider()`. ::: widget/android/nsWindow.cpp @@ +1265,5 @@ > window.mAndroidView->mEventDispatcher->Attach( > java::EventDispatcher::Ref::From(aDispatcher), mDOMWindow); > > + if (window.mSessionAccessibility) { > + window.mSessionAccessibility.Detach(); Four space indent @@ +1312,5 @@ > + inst.Env()); > + sessionAccessibility = java::SessionAccessibility::NativeProvider::Ref::From( > + aSessionAccessibility); > + > + if (window.mSessionAccessibility) { I think you can `MOZ_ASSERT(!window.mSessionAccessibility)` here?
Attachment #9010794 -
Flags: review?(nchen) → review+
Pushed by eisaacson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6c784e692474 Make GeckoView's SessionAccessibility a JNIObject associated with a nsWindow. r=jchen
Comment 7•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6c784e692474
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Updated•6 years ago
|
Assignee: nobody → eitan
You need to log in
before you can comment on or make changes to this bug.
Description
•