it should be faster than the hash table.

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

> ::: 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).

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

(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/

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

(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.

(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.