Closed Bug 678477 Opened 9 years ago Closed 7 years ago

change ownership of nsCaretAccessible

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 3 obsolete files)

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.
Assignee: nobody → michaljev
Status: NEW → ASSIGNED
Trev, please comment your thinking about fixing this bug.
Michał, do you work on this? Should I put the bug back into a pool?
Hi, sorry for being absent :( I'll send patch this week if its ok :)
Sure. What naming did you choose?

(Let's get feedback from Trev on the approach)
Flags: needinfo?(trev.saunders)
Sure. What naming did you choose?

(Let's get feedback from Trev on the approach)
caretHandler :) if its fine
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.
Flags: needinfo?(trev.saunders)
sounds good for me
It doesn't manage a caret, so CaretHandler might be preciser.
hmm then what name fits best ?
(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.
Then I'll change it to CaretManager
Im not really sure what I suppose to do, how to do "unique instance nsCaretAccessible is enough. The instance should be created by AccessibilityService." ?
class CaretManager {};
class nsAccessibilityService : public CaretManager, public ... {};
I changed some records in makefile.in :) added public mozilla::a11y::CaretManager and now changing it to be "unique instance" :)
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 ?
(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.
Surkov, should I model on FocusManager ?
(In reply to Michał Frontczak :fxa90id from comment #18)
> Surkov, should I model on FocusManager ?

I'm not sure I follow the question.
Could you check it ?
Attachment #695730 - Flags: review?(surkov.alexander)
Comment on attachment 695730 [details] [diff] [review]
change ownership of nsCaretAccessible

made few mistakes sorry.
Attachment #695730 - Flags: review?(surkov.alexander) → review-
Attachment #695730 - Attachment is obsolete: true
Attachment #695732 - Flags: review?(surkov.alexander)
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
Attachment #695732 - Flags: review?(surkov.alexander) → review-
I addded CaretMgr friend member function :).
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.
Yes, sure I was playing with it too long and couldnt figure out why its doesnt work :(
Attached patch patch (obsolete) — Splinter Review
Assignee: michaljev → surkov.alexander
Attachment #695732 - Attachment is obsolete: true
Attachment #720590 - Flags: review?(trev.saunders)
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
Attachment #720590 - Flags: review?(trev.saunders) → review+
(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.
> > >-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.
Ok, I'll change nsAccessibilityService to be nsISupports implementation for SelectionManager
Attachment #720590 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/24d44bc51473
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Blocks: 735663
You need to log in before you can comment on or make changes to this bug.