only use the pres shell to get existing doc accessibles

RESOLVED FIXED

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
it should be faster than the hash table.
(Assignee)

Comment 1

6 years ago
Created attachment 693396 [details] [diff] [review]
patch
Attachment #693396 - Flags: review?(surkov.alexander)
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
(Assignee)

Comment 3

6 years ago
> ::: 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.
(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).
Attachment #693396 - Flags: review?(surkov.alexander) → review+
I thought we were agreed on DocAccessible -> Document replacement.
(Assignee)

Comment 7

6 years ago
(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/
(Assignee)

Comment 8

6 years ago
and https://hg.mozilla.org/integration/mozilla-inbound/rev/f301d0d6f540 because aparently I don't know how to use grep
https://hg.mozilla.org/mozilla-central/rev/29ce28a490c9
https://hg.mozilla.org/mozilla-central/rev/f301d0d6f540
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(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.
Target Milestone: mozilla20 → ---
(Assignee)

Comment 11

6 years ago
(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.
You need to log in before you can comment on or make changes to this bug.