678477 - change ownership of nsCaretAccessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

1) nsCaretAccessible has nothing common with nsRootAccessible, unique instance nsCaretAccessible is enough. The instance should be created by AccessibilityService.
2) rename nsCaretAccessible to something more reasonable. This class is used to fire caret move events when normal selection is changed, calculate caret rect and fire text attribute change when spellcheck selection is changed. It might be hard to choose a name reflecting the propose of the class, so maybe the name should answer how rather than what question, for example, DOMSelectionHandler.

Comment 1

[alexander :surkov (:asurkov)]

Trev, please comment your thinking about fixing this bug.

Comment 2

[alexander :surkov (:asurkov)]

Michał, do you work on this? Should I put the bug back into a pool?

Comment 3

[Michał Frontczak [:fxa90id] ♥]

Hi, sorry for being absent :( I'll send patch this week if its ok :)

Comment 4

[alexander :surkov (:asurkov)]

Sure. What naming did you choose?

(Let's get feedback from Trev on the approach)

Comment 5

[alexander :surkov (:asurkov)]

Sure. What naming did you choose?

(Let's get feedback from Trev on the approach)

Comment 6

[Michał Frontczak [:fxa90id] ♥]

caretHandler :) if its fine

Comment 7

[Trevor Saunders (:tbsaunde)]

So, actually it occurs to me instead of having the service create an object we could just add more stuff to nsAccessibilityService.  At which point I guess we could either get rid of the class, or have a class like CaretManager that nsAccessibilityServices inherits from like FocusManager just for the purpose of keeping code seperate.

Comment 8

[Michał Frontczak [:fxa90id] ♥]

sounds good for me

Comment 9

[alexander :surkov (:asurkov)]

It doesn't manage a caret, so CaretHandler might be preciser.

Comment 10

[Michał Frontczak [:fxa90id] ♥]

hmm then what name fits best ?

Comment 11

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #9)
> It doesn't manage a caret, so CaretHandler might be preciser.

I think I don't mind actually. It seems to be closer to FocusManager and DocManager enough so CaretManager should be ok.

Comment 12

[Michał Frontczak [:fxa90id] ♥]

Then I'll change it to CaretManager

Comment 13

[Michał Frontczak [:fxa90id] ♥]

Im not really sure what I suppose to do, how to do "unique instance nsCaretAccessible is enough. The instance should be created by AccessibilityService." ?

Comment 14

[alexander :surkov (:asurkov)]

class CaretManager {};
class nsAccessibilityService : public CaretManager, public ... {};

Comment 15

[Michał Frontczak [:fxa90id] ♥]

I changed some records in makefile.in :) added public mozilla::a11y::CaretManager and now changing it to be "unique instance" :)

Comment 16

[Michał Frontczak [:fxa90id] ♥]

Surkov, I cant catch you on irc ;(, anyway Im trying to create CaretManager by modeling on FocusManager but Im not sure If Im doing it right way. Could you help me a bit ?

Comment 17

[alexander :surkov (:asurkov)]

(In reply to Michał Frontczak :fxa90id from comment #16)
> Surkov, I cant catch you on irc ;(, anyway Im trying to create CaretManager
> by modeling on FocusManager but Im not sure If Im doing it right way. Could
> you help me a bit ?

Sure, just catch me. Otherwise if our works hours aren't intersected much then put your questions and wip patch here. I will answer and provide a feedback.

Comment 18

[Michał Frontczak [:fxa90id] ♥]

Surkov, should I model on FocusManager ?

Comment 19

[alexander :surkov (:asurkov)]

(In reply to Michał Frontczak :fxa90id from comment #18)
> Surkov, should I model on FocusManager ?

I'm not sure I follow the question.

Comment 20

[Michał Frontczak [:fxa90id] ♥]

Attachment: change ownership of nsCaretAccessible

Could you check it ?

Comment 21

[Michał Frontczak [:fxa90id] ♥]

Comment on attachment 695730 [details] [diff] [review]
change ownership of nsCaretAccessible

made few mistakes sorry.

Comment 22

[Michał Frontczak [:fxa90id] ♥]

Attachment: change ownership of nsCaretAccessible

Comment 23

[alexander :surkov (:asurkov)]

Comment on attachment 695732 [details] [diff] [review]
change ownership of nsCaretAccessible

Review of attachment 695732 [details] [diff] [review]:
-----------------------------------------------------------------

you need to hg rename nsCaretAccessible files to CaretManager.

::: accessible/src/generic/RootAccessible.cpp
@@ +198,5 @@
>      }
>    }
>  
> +  if (!mCaretManager) {
> +    mCaretManager = getAccService()->CaretMgr();

It doesn't seem RootAccessible has mCaretManager member. There's no getAccService() function ('g' is used in lowercase). nsAccessibilityService doesn't have CaretMgr() method. Please build the patch before filing it for review.

::: accessible/src/generic/RootAccessible.h
@@ +39,5 @@
>    virtual mozilla::a11y::role NativeRole();
>    virtual uint64_t NativeState();
>  
>    // RootAccessible
> +  CaretManager* GetCaretManager();

nsAccessibilityService.h version should be enough

::: accessible/src/msaa/AccessibleWrap.cpp
@@ +1807,5 @@
>    if (!rootAccessible) {
>      return;
>    }
>  
> +  nsRefPtr<CaretManager> caretManager = rootAccessible->GetCaretManager();

you don't need to addref, just deal with raw pointers

Comment 24

[Michał Frontczak [:fxa90id] ♥]

I addded CaretMgr friend member function :).

Comment 25

[alexander :surkov (:asurkov)]

Michał, I think to steal this bug from you. I suspect this class might be leaking so I need to sort out things asap. I hope you're ok with this. Let me know if you need me to find another bug for you.

Comment 26

[Michał Frontczak [:fxa90id] ♥]

Yes, sure I was playing with it too long and couldnt figure out why its doesnt work :(

Comment 27

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 28

[Trevor Saunders (:tbsaunde)]

Comment on attachment 720590 [details] [diff] [review]
patch

>diff --git a/accessible/src/base/nsCaretAccessible.cpp b/accessible/src/base/SelectionManager.cpp
>rename from accessible/src/base/nsCaretAccessible.cpp
>rename to accessible/src/base/SelectionManager.cpp
>--- a/accessible/src/base/nsCaretAccessible.cpp
>+++ b/accessible/src/base/SelectionManager.cpp
>@@ -1,223 +1,190 @@
> /* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* 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 "nsCaretAccessible.h"
>+#include "SelectionManager.h"

it's been claimed to me that its better to always include the same way so mozilla/a11y/SelectionManager.h because its faster for the compiler to see its the same thing.

>+SelectionManager::SelectionManager() { }
>+SelectionManager::~SelectionManager() { }

you can't inline these because that would make you include HyperTextAccessible.h into SelectionManager.h which has to be included into nsAccessibilityService.h

but I don't see why you can't remove both and let the compiler autogenerate them for you.  (or does it try to do that when its sees the class, not when it needs them?)

>+SelectionManager::Shutdown()
> {
>   // The caret accessible isn't shut down until the RootAccessible owning it is shut down

bogus now right?

>   // Don't fire events until document is loaded.
>   if (document && document->IsContentLoaded()) {
>     // The caret accessible has the same lifetime as the root accessible, and
>     // this outlives all its descendant document accessibles, so that we are
>     // guaranteed that the notification is processed before the caret accessible
>     // is destroyed.

update

>+SelectionManager::ProcessSelectionChanged(nsISelection* aSelection)
> {
>   nsCOMPtr<nsISelectionPrivate> privSel(do_QueryInterface(aSelection));

fwiw nsISelection / nsISelectionPrivate are builtin class, and there's already code in the tree that just static casts them to Selection* so we should get rid of this some day.

>-NS_IMPL_ISUPPORTS_INHERITED3(nsAccessibilityService,
>-                             DocManager,
>-                             nsIAccessibilityService,
>-                             nsIAccessibleRetrieval,
>-                             nsIObserver)
>+NS_INTERFACE_TABLE_HEAD(nsAccessibilityService)
>+NS_INTERFACE_TABLE_INHERITED3(nsAccessibilityService,
>+                              nsIAccessibilityService,
>+                              nsIAccessibleRetrieval,
>+                              nsIObserver)
>+if (NS_SUCCEEDED(rv)) \
>+  return rv; \

are you sure the out pointer is AddRefed at this point?

>+rv = SelectionManager::QueryInterface(aIID, aInstancePtr); \

why are \ needed I don't see any reason its important these are on the same line for the compiler.

>+NS_INTERFACE_TABLE_TAIL_INHERITING(DocManager)
>+
>+NS_IMPL_ADDREF_INHERITED(nsAccessibilityService, DocManager)
>+NS_IMPL_RELEASE_INHERITED(nsAccessibilityService, DocManager)

it seems it would be much simpler to just use NS_IMPL_ISUPPORTS4 and add nsISelectionListener

Comment 29

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #28)

> >-#include "nsCaretAccessible.h"
> >+#include "SelectionManager.h"
> 
> it's been claimed to me that its better to always include the same way so
> mozilla/a11y/SelectionManager.h because its faster for the compiler to see
> its the same thing.

yep, I recall that

> >+SelectionManager::SelectionManager() { }
> >+SelectionManager::~SelectionManager() { }
> 
> you can't inline these because that would make you include
> HyperTextAccessible.h into SelectionManager.h which has to be included into
> nsAccessibilityService.h
> 
> but I don't see why you can't remove both and let the compiler autogenerate
> them for you.  (or does it try to do that when its sees the class, not when
> it needs them?)

seems working

> >+SelectionManager::Shutdown()
> > {
> >   // The caret accessible isn't shut down until the RootAccessible owning it is shut down
> 
> bogus now right?
> 
> >   // Don't fire events until document is loaded.
> >   if (document && document->IsContentLoaded()) {
> >     // The caret accessible has the same lifetime as the root accessible, and
> >     // this outlives all its descendant document accessibles, so that we are
> >     // guaranteed that the notification is processed before the caret accessible
> >     // is destroyed.
> 
> update

done

> >+SelectionManager::ProcessSelectionChanged(nsISelection* aSelection)
> > {
> >   nsCOMPtr<nsISelectionPrivate> privSel(do_QueryInterface(aSelection));
> 
> fwiw nsISelection / nsISelectionPrivate are builtin class, and there's
> already code in the tree that just static casts them to Selection* so we
> should get rid of this some day.

yeah, I saw your work on this. Maybe I'll tweak this code soon.

> >-NS_IMPL_ISUPPORTS_INHERITED3(nsAccessibilityService,
> >-                             DocManager,
> >-                             nsIAccessibilityService,
> >-                             nsIAccessibleRetrieval,
> >-                             nsIObserver)
> >+NS_INTERFACE_TABLE_HEAD(nsAccessibilityService)
> >+NS_INTERFACE_TABLE_INHERITED3(nsAccessibilityService,
> >+                              nsIAccessibilityService,
> >+                              nsIAccessibleRetrieval,
> >+                              nsIObserver)
> >+if (NS_SUCCEEDED(rv)) \
> >+  return rv; \
> 
> are you sure the out pointer is AddRefed at this point?

it was a copy of NS_INTERFACE_TABLE_TAIL_INHERITING(DocManager) so we should be safe

> >+rv = SelectionManager::QueryInterface(aIID, aInstancePtr); \
> 
> why are \ needed I don't see any reason its important these are on the same
> line for the compiler.

dunno, I tend to consider it as code style

> >+NS_INTERFACE_TABLE_TAIL_INHERITING(DocManager)
> >+
> >+NS_IMPL_ADDREF_INHERITED(nsAccessibilityService, DocManager)
> >+NS_IMPL_RELEASE_INHERITED(nsAccessibilityService, DocManager)
> 
> it seems it would be much simpler to just use NS_IMPL_ISUPPORTS4 and add
> nsISelectionListener

that should work but it isn't extensible. I can't say I like approach I implemented this.

Comment 30

[Trevor Saunders (:tbsaunde)]

> > >-NS_IMPL_ISUPPORTS_INHERITED3(nsAccessibilityService,
> > >-                             DocManager,
> > >-                             nsIAccessibilityService,
> > >-                             nsIAccessibleRetrieval,
> > >-                             nsIObserver)
> > >+NS_INTERFACE_TABLE_HEAD(nsAccessibilityService)
> > >+NS_INTERFACE_TABLE_INHERITED3(nsAccessibilityService,
> > >+                              nsIAccessibilityService,
> > >+                              nsIAccessibleRetrieval,
> > >+                              nsIObserver)
> > >+if (NS_SUCCEEDED(rv)) \
> > >+  return rv; \
> > 
> > are you sure the out pointer is AddRefed at this point?
> 
> it was a copy of NS_INTERFACE_TABLE_TAIL_INHERITING(DocManager) so we should
> be safe

probably best to check how macros actually work.

> > >+rv = SelectionManager::QueryInterface(aIID, aInstancePtr); \
> > 
> > why are \ needed I don't see any reason its important these are on the same
> > line for the compiler.
> 
> dunno, I tend to consider it as code style

that is... odd style

> > >+NS_INTERFACE_TABLE_TAIL_INHERITING(DocManager)
> > >+
> > >+NS_IMPL_ADDREF_INHERITED(nsAccessibilityService, DocManager)
> > >+NS_IMPL_RELEASE_INHERITED(nsAccessibilityService, DocManager)
> > 
> > it seems it would be much simpler to just use NS_IMPL_ISUPPORTS4 and add
> > nsISelectionListener
> 
> that should work but it isn't extensible. I can't say I like approach I
> implemented this.

who cares if it won't scale to a zillion interfaces? its very rare that we add interfaces to this singleton.  and if some day we find ourselves in the insane world where nsAccessibilityService has a zillion interfaces we can worry about the problem then, and won't have lost anything by keeping it simple until then.

Comment 31

[alexander :surkov (:asurkov)]

Ok, I'll change nsAccessibilityService to be nsISupports implementation for SelectionManager

Comment 32

[alexander :surkov (:asurkov)]

Attachment: patch2 (for landing)

Comment 33

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/24d44bc51473

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/24d44bc51473