822664 - only use the pres shell to get existing doc accessibles

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

it should be faster than the hash table.

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

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

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

::: accessible/src/base/DocManager.h
@@ +147,5 @@
>  };
>  
> +  /**
> +   * Return The existing document accessible for the document if any.
> +   */

nit: wrong indent
nit: The -> the
perhaps you need to clarify (i.e. the function doesn't try to create a document accessible).

btw, the comment should warn that document accessible of primary presshell is returned

@@ +149,5 @@
> +  /**
> +   * Return The existing document accessible for the document if any.
> +   */
> +inline DocAccessible*
> +GetExistingDocAccessible(const nsIDocument* aDocument)

nit: maybe name it 

GetExistingDocument() since we tend to not use DocAccessible internally. I think it's not supposed to be used outside?

alternatives:
GetCachedDocument() or GetCreatedDocument()

perhaps I like more GetCachedDocument().

@@ +152,5 @@
> +inline DocAccessible*
> +GetExistingDocAccessible(const nsIDocument* aDocument)
> +{
> +  if (!aDocument)
> +    return nullptr;

do you really need null check? If that's an internal method and it seems we don't operate on null document internally.

::: accessible/src/base/nsAccessibilityService.cpp
@@ +201,3 @@
>        }
>  
> +      return aCanCreate ?  GetDocAccessible(ps) : ps->GetDocAccessible();

nit: extra space after ?

::: accessible/src/generic/DocAccessible.cpp
@@ +642,1 @@
>    mPresShell = nullptr;  // Avoid reentrancy

ok, dependents shutdowns shoudln't rely on doc from cache

Comment 3

[Trevor Saunders (:tbsaunde)]

> ::: accessible/src/base/DocManager.h
> @@ +147,5 @@
> >  };
> >  
> > +  /**
> > +   * Return The existing document accessible for the document if any.
> > +   */
> 
> nit: wrong indent
> nit: The -> the
> perhaps you need to clarify (i.e. the function doesn't try to create a
> document accessible).

I'd hope the name would make that obvious, but ok

> @@ +149,5 @@
> > +  /**
> > +   * Return The existing document accessible for the document if any.
> > +   */
> > +inline DocAccessible*
> > +GetExistingDocAccessible(const nsIDocument* aDocument)
> 
> nit: maybe name it 
> 
> GetExistingDocument() since we tend to not use DocAccessible internally. I
> think it's not supposed to be used outside?

I'm not sure if there's a case outside where the caller doesn't want to create a document and only operate on existing ones, but if there is I don't see why it would be bad to allow it to be used outside.

> alternatives:
> GetCachedDocument() or GetCreatedDocument()
> 
> perhaps I like more GetCachedDocument().

I think I like GetExisting or GetCreated more since I don't really think of it as a cache.

> @@ +152,5 @@
> > +inline DocAccessible*
> > +GetExistingDocAccessible(const nsIDocument* aDocument)
> > +{
> > +  if (!aDocument)
> > +    return nullptr;
> 
> do you really need null check? If that's an internal method and it seems we
> don't operate on null document internally.

probably, I was just to lazy to audit all the callers.

> ::: accessible/src/generic/DocAccessible.cpp
> @@ +642,1 @@
> >    mPresShell = nullptr;  // Avoid reentrancy
> 
> ok, dependents shutdowns shoudln't rely on doc from cache

not sure I  understand this.

Comment 4

[alexander :surkov (:asurkov)]

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

> > perhaps I like more GetCachedDocument().
> 
> I think I like GetExisting or GetCreated more since I don't really think of
> it as a cache.

yeah, if you think about cached from implementation point of view, I thought about it as stored somewhere. But up to you.

> > ok, dependents shutdowns shoudln't rely on doc from cache
> 
> not sure I  understand this.

I meant we need to remember that Shutdown() can operate on mDocument only if needed (like MSAA or ATK document specific shutdowns).

Comment 5

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29ce28a490c9

Comment 6

[alexander :surkov (:asurkov)]

I thought we were agreed on DocAccessible -> Document replacement.

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> I thought we were agreed on DocAccessible -> Document replacement.

well, I thought you were fine with DocAccessible since it can be used outside accessibles/

Comment 8

[Trevor Saunders (:tbsaunde)]

and https://hg.mozilla.org/integration/mozilla-inbound/rev/f301d0d6f540 because aparently I don't know how to use grep

Comment 9

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/29ce28a490c9
https://hg.mozilla.org/mozilla-central/rev/f301d0d6f540

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> (In reply to alexander :surkov from comment #6)
> > I thought we were agreed on DocAccessible -> Document replacement.
> 
> well, I thought you were fine with DocAccessible since it can be used
> outside accessibles/

ok, maybe it doesn't really matter. It doesn't think anybody is going to use it though.

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > (In reply to alexander :surkov from comment #6)
> > > I thought we were agreed on DocAccessible -> Document replacement.
> > 
> > well, I thought you were fine with DocAccessible since it can be used
> > outside accessibles/
> 
> ok, maybe it doesn't really matter. It doesn't think anybody is going to use
> it though.

well, who will use it sort of depends on bug 777603.

Note we also have plenty of functions named GetDocAccessible() around so its sort of more inline with there names too.