Closed Bug 1419362 Opened 2 years ago Closed 2 years ago

[e10s a11y] Handler: Fetch all text info in one call

Categories

(Core :: Disability Access APIs, enhancement)

All
Windows
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: Jamie, Assigned: Jamie)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

If a client requests all text (via IAccessibleText::text with IA2_TEXT_OFFSET_LENGTH), it's quite likely they will want all other information about the text as well; i.e. embedded objects and attributes. Separate calls for text, hyperlinks and attributes means at least three cross-process calls. However, there may well be multiple text attribute runs, in which case there is one additional call per additional attribute run. We can instead fetch all of these in one single call.

This is somewhat different to the current handler caching in that we don't cache this in the initial payload, since the client might not be interested in text at all (or not interested for specific nodes). Instead, we fetch this information when the client requests text, caching the hyperlinks and attributes so the client can retrieve them with subsequent calls.
Blocks: 1365655
(In reply to James Teh [:Jamie] from comment #4)
> Created attachment 8931225 [details]
> Bug 1419362 part 3: Accessible HandlerProvider: Implement a method to
> retrieve all text, hyperlinks and attributes at once.

Aaron, I'm primarily requesting your review on this regarding the HandlerProvider::ToWrappedObject method. Of course, any other comments are welcome, but the rest is all fairly accessibility specific and I think Marco can take care of it. Thanks!
Comment on attachment 8931223 [details]
Bug 1419362 part 1: Make IAccessibleHypertext2::hyperlinks return null and S_FALSE when there are no hyperlinks.

https://reviewboard.mozilla.org/r/202336/#review207698
Attachment #8931223 - Flags: review?(mzehe) → review+
Comment on attachment 8931224 [details]
Bug 1419362 part 2: Move AccessibleTextTearoff into AccessibleHandler.

https://reviewboard.mozilla.org/r/202338/#review207700
Attachment #8931224 - Flags: review?(mzehe) → review+
Comment on attachment 8931225 [details]
Bug 1419362 part 3: Accessible HandlerProvider: Implement a method to retrieve all text, hyperlinks and attributes at once.

https://reviewboard.mozilla.org/r/202340/#review207704

::: accessible/ipc/win/handler/HandlerData.idl:159
(Diff revision 1)
>    HRESULT Refresh([out] DynamicIA2Data* aOutData);
> +  [propget] HRESULT AllTextInfo([out] BSTR* aText,
> +    [out, size_is(,*aNHyperlinks)] IAccessibleHyperlink*** aHyperlinks,
> +    [out] long* aNHyperlinks,
> +    [out, size_is(,*aNAttribRuns)] IA2TextSegment** aAttribRuns,
> +    [out] long* aNAttribRuns);

Please make sure to check whether this needs a clobber when landing.
Attachment #8931225 - Flags: review?(mzehe) → review+
Comment on attachment 8931226 [details]
Bug 1419362 part 4: AccessibleHandler: When a caller asks for all text, cache hyperlinks and text attributes in the same call.

https://reviewboard.mozilla.org/r/202342/#review207706
Attachment #8931226 - Flags: review?(mzehe) → review+
Comment on attachment 8931292 [details]
Bug 1419362 part 5: Clobber required for accessibility IDL changes.

https://reviewboard.mozilla.org/r/202442/#review207796

r=me. Note that this may need rebasing if other bugs require clobbering in the meantime.
Attachment #8931292 - Flags: review?(mzehe) → review+
Comment on attachment 8931225 [details]
Bug 1419362 part 3: Accessible HandlerProvider: Implement a method to retrieve all text, hyperlinks and attributes at once.

https://reviewboard.mozilla.org/r/202340/#review207954

::: accessible/ipc/win/HandlerProvider.cpp:578
(Diff revision 1)
> +HRESULT
> +HandlerProvider::ToWrappedObject(Interface** aObj)
> +{
> +  mscom::STAUniquePtr<Interface> inObj;
> +  // Adopt the reference to the original object.
> +  *mscom::getter_AddRefs(inObj) = *aObj;

I'm not a fan of this. Can you just pass *aObj to inObj's constructor? That's what we do in MainThreadHandoff::OnWalkInterface.

::: accessible/ipc/win/HandlerProvider.cpp:582
(Diff revision 1)
> +  // Adopt the reference to the original object.
> +  *mscom::getter_AddRefs(inObj) = *aObj;
> +  RefPtr<HandlerProvider> hprov = new HandlerProvider(__uuidof(Interface),
> +    mscom::ToInterceptorTargetPtr(inObj));
> +  HRESULT hr = mscom::MainThreadHandoff::WrapInterface(Move(inObj), hprov,
> +    (Interface**)aObj);

Nit: I think this cast is redundant.

::: accessible/ipc/win/HandlerProvider.cpp:605
(Diff revision 1)
> +  MOZ_ASSERT(aNAttribRuns);
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(mTargetUnk);
> +
> +  RefPtr<IAccessibleHypertext2> ht;
> +  HRESULT hr = mTargetUnk.get()->QueryInterface(IID_IAccessibleHypertext2,

Nit: Do you need to call .get() here? I think operator -> should be overloaded such that you can just call QI directly.
Attachment #8931225 - Flags: review?(aklotz) → review+
(In reply to Aaron Klotz [:aklotz] from comment #13)
> > +  mscom::STAUniquePtr<Interface> inObj;
> > +  *mscom::getter_AddRefs(inObj) = *aObj;
> I'm not a fan of this. Can you just pass *aObj to inObj's constructor?
> That's what we do in MainThreadHandoff::OnWalkInterface.

I'm a bit confused about the ref counting situation here. Won't the smart pointer AddRef the raw pointer in this case? That's my understanding of RefPtr based stuff. Or is this different because it's a UniquePtr? I want the original reference to get adopted by the interceptor.

> > +  HRESULT hr = mscom::MainThreadHandoff::WrapInterface(Move(inObj), hprov,
> > +    (Interface**)aObj);
> Nit: I think this cast is redundant.

Oh. So it is. I copied it from CreateHolderFromAccessible, but yeah, WrapInterface is a template, so it shouldn't be necessary. Is CreateHolderFromAccessible a special exception for some reason or did you just manage the same nit I did? :)
(In reply to James Teh [:Jamie] from comment #14)
> (In reply to Aaron Klotz [:aklotz] from comment #13)
> > > +  mscom::STAUniquePtr<Interface> inObj;
> > > +  *mscom::getter_AddRefs(inObj) = *aObj;
> > I'm not a fan of this. Can you just pass *aObj to inObj's constructor?
> > That's what we do in MainThreadHandoff::OnWalkInterface.
> 
> I'm a bit confused about the ref counting situation here. Won't the smart
> pointer AddRef the raw pointer in this case? That's my understanding of
> RefPtr based stuff. Or is this different because it's a UniquePtr? I want
> the original reference to get adopted by the interceptor.

STAUniquePtr, being UniquePtr-based, doesn't AddRef.

> 
> > > +  HRESULT hr = mscom::MainThreadHandoff::WrapInterface(Move(inObj), hprov,
> > > +    (Interface**)aObj);
> > Nit: I think this cast is redundant.
> 
> Oh. So it is. I copied it from CreateHolderFromAccessible, but yeah,
> WrapInterface is a template, so it shouldn't be necessary. Is
> CreateHolderFromAccessible a special exception for some reason or did you
> just manage the same nit I did? :)

Yeah I just nit my own code, apparently ;-)
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/9eb730bc1ab0
part 1: Make IAccessibleHypertext2::hyperlinks return null and S_FALSE when there are no hyperlinks. r=MarcoZ
https://hg.mozilla.org/integration/autoland/rev/2e0a46d7290d
part 2: Move AccessibleTextTearoff into AccessibleHandler. r=MarcoZ
https://hg.mozilla.org/integration/autoland/rev/ec30f83fa43e
part 3: Accessible HandlerProvider: Implement a method to retrieve all text, hyperlinks and attributes at once. r=aklotz,MarcoZ
https://hg.mozilla.org/integration/autoland/rev/9ca69991432e
part 4: AccessibleHandler: When a caller asks for all text, cache hyperlinks and text attributes in the same call. r=MarcoZ
https://hg.mozilla.org/integration/autoland/rev/172e6c7b3ada
part 5: Clobber required for accessibility IDL changes. r=MarcoZ
Keywords: checkin-needed
Depends on: 1421209
Depends on: 1421873
You need to log in before you can comment on or make changes to this bug.