Trevor Saunders (:tbsaunde) Assignee
	Description • 8 years ago

This is kind of just an RFC.  I gave up on finishing this when I realized that
actually getting rid of InvalidateChildren was going to be tricky without
dealing with the underlying issue in bug 1198974 where direct kids could
actually get removed by InvalidateChildren().  However some of the patches I
wrote before realizing that may still be useful on there own.


  # Please enter the summary
(first line) and description (other lines). Lines

	Trevor Saunders (:tbsaunde) Assignee
	Comment 1 • 8 years ago

If we are shutting the object down we can leave mOffsets for the destructor to
clean up.  That means the only time we need to explicitly clear mOffsets is
in CacheChildren() which means we can remove
HyperTextAccessible::InvalidateChildren().

	Trevor Saunders (:tbsaunde) Assignee
	Comment 2 • 8 years ago

If we are shutting the object down we can leave mOffsets for the destructor to
clean up.  That means the only time we need to explicitly clear mOffsets is
in CacheChildren() which means we can remove
HyperTextAccessible::InvalidateChildren().

	Trevor Saunders (:tbsaunde) Assignee
	Comment 3 • 8 years ago

InvalidateChildren() is called in two places, either by Accessible::Shutdown()
  or by DocAccessible::UpdateTreeOnInsertion().  In the case
  Accessible::Shutdown() is calling InvalidateChildren() there is no reason to
  run mozAccessible::invalidateChildren() because the object is about to be
  destroyed which will do that clean up anyway.  In the second case
  CacheChildren() is called immediately after InvalidateChildren() so we can
  clear the mac specific state there.

	Trevor Saunders (:tbsaunde) Assignee
	Comment 6 • 8 years ago

After collecting the existing child windows the list of children of the
Application accessible is kept up to date with AppendChildren and
RemoveChildren without using CacheChildren or InvalidateChildren.  So instead
of calling EnsureChildren() every time we need to get a document accessible we
should be able to collect the list of windows when a11y starts and then just
keep it up to date.

	Trevor Saunders (:tbsaunde) Assignee
	Comment 7 • 8 years ago

There are only a few specific cases where AreChildrenCached() will return
  false, and mozAccessible::children() shouldn't be called during any of them.

	Trevor Saunders (:tbsaunde) Assignee
	Comment 8 • 8 years ago

There are a couple places that only call EnsureChildren(), but the accessible
  shouldn't already have children in those cases, so calling
  InvalidateChildren() first should be a no op.  Similarly
  HTMLComboboxAccessible::CacheChildren() can call InvalidateChildren() on
  mListAccessible without calling EnsureChildren(), but in that case
  mListAccessible should probably have no children, and in that case it is
  removed from the children of the HTMLComboboxAccessible, and won't be added
  back without updating its children again.
  mListAccessible won't

	Trevor Saunders (:tbsaunde) Assignee
	Comment 9 • 8 years ago

This means that ApplicationAccessible can ignore mChildrenFlags.

	Trevor Saunders (:tbsaunde) Assignee
	Comment 11 • 8 years ago

We always call InvalidateChildren() before checking mChildrenFlags which means
it will always be eChildrenUninitialized so we can remove that state.  Which
means we can convert mChildrenFlags to a simple bool if there is only embedded
children or if there are mixed children.

	alexander :surkov (:asurkov)
	Comment 18 • 8 years ago

Comment on attachment 8711697 [details] [diff] [review]
Clear HyperTextAccessible::mOffsets when caching children instead of when clearing the cache

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

this one doesn't really make us closer to get rid of InvalidateChildren, because you should keep in mind then you have to remove mOffsets.Clear(); from CacheChildren. Having no this patch you would just remove the method and that's it.

	alexander :surkov (:asurkov)
	Comment 19 • 8 years ago

Comment on attachment 8711700 [details] [diff] [review]
have mac override Accessible::CacheChildren instead of Accessible::InvalidateChildren

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

Same about this patch, no benefits, besides that InvalidateChildren inside CacheChildren looks weird, in the end you will either remove this code or remove that code.

	alexander :surkov (:asurkov)
	Comment 20 • 8 years ago

We might get rid of CacheChildren for ApplicationAccessible as you suggest, because anyway the method is called on it always explicitly, but I'm not sure about other patches, whether they are different from the first two, and make things easier/nicer.

	Trevor Saunders (:tbsaunde) Assignee
	Comment 21 • 8 years ago

> this one doesn't really make us closer to get rid of InvalidateChildren,
> because you should keep in mind then you have to remove mOffsets.Clear();
> from CacheChildren. Having no this patch you would just remove the method
> and that's it.

uh why would you remove clearing mOffsets fromCacheChildren()? That doesn't make sense to me.  YOu could maybe do better and only invalidate the offsets that might have changed, but that has nothing to do with InvalidateChildren() accept that its easier after this patch and other stuff.

	Trevor Saunders (:tbsaunde) Assignee
	Comment 22 • 8 years ago

> Same about this patch, no benefits, besides that InvalidateChildren inside
> CacheChildren looks weird, in the end you will either remove this code or
> remove that code.
I don't get this either, you could do something else to this mac cache invalidation stuff, but after this patch that has nothing to do with Accessible::InvalidateChildren()

I guess the name invalidateChildren isn't great, but if it was invalidateChildCache or something that would seem totally reasonable that caching things can invalidte an old cache.

	alexander :surkov (:asurkov)
	Comment 23 • 8 years ago

(In reply to Trevor Saunders (:tbsaunde) from comment #21)
> > this one doesn't really make us closer to get rid of InvalidateChildren,
> > because you should keep in mind then you have to remove mOffsets.Clear();
> > from CacheChildren. Having no this patch you would just remove the method
> > and that's it.
> 
> uh why would you remove clearing mOffsets fromCacheChildren()? That doesn't
> make sense to me.

If we didn't have InvalidateChildren then CacheChildren would be called only once (at creation time, if we had this method at all), so it doesn't make sense to clear mOffset because there's nothing there yet.

>  YOu could maybe do better and only invalidate the offsets
> that might have changed,

right, like we do for RemoveChild for example

> but that has nothing to do with
> InvalidateChildren() accept that its easier after this patch and other stuff.

If we didn't have InvalidateChildren() then we would have InsertChildAt() which would invalidate offsets.

	Trevor Saunders (:tbsaunde) Assignee
	Comment 24 • 8 years ago

(In reply to alexander :surkov from comment #23)
> (In reply to Trevor Saunders (:tbsaunde) from comment #21)
> > > this one doesn't really make us closer to get rid of InvalidateChildren,
> > > because you should keep in mind then you have to remove mOffsets.Clear();
> > > from CacheChildren. Having no this patch you would just remove the method
> > > and that's it.
> > 
> > uh why would you remove clearing mOffsets fromCacheChildren()? That doesn't
> > make sense to me.
> 
> If we didn't have InvalidateChildren then CacheChildren would be called only
> once (at creation time, if we had this method at all), so it doesn't make
> sense to clear mOffset because there's nothing there yet.

maybe, that's one possible future world, but I'm not convinced it is the only one,  and I think its a mistake to worry about that if we just want to get rid of InvalidateChildren()

> > but that has nothing to do with
> > InvalidateChildren() accept that its easier after this patch and other stuff.
> 
> If we didn't have InvalidateChildren() then we would have InsertChildAt()
> which would invalidate offsets.

maybe, but not necessarily.

	alexander :surkov (:asurkov)
	Comment 25 • 8 years ago

Comment on attachment 8711701 [details] [diff] [review]
remove HTMLComboboxAccessible::InvalidateChildren()

InvalidateChildren has been removed. These patches shouldn't be needed any longer.

	alexander :surkov (:asurkov)
	Comment 26 • 8 years ago

We probably don't need this bug anymore, since CacheChildren / InvalidateChildren have been gone. Trevor, please reopen it, if there are worth-to-land bits in the patches.