Lorien Hu (:lsocks) Reporter
	Description • 9 years ago

      No description provided.

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

Comment on attachment 8636203 [details] [diff] [review]
check if we're in content or parent to create root vs docs

"meh" I guess in principal it works, but it seems like there's a nicer way to do what this function does.

>   bool isRootDoc = nsCoreUtils::IsRootDocument(aDocument);
>+  // Content process should not have root accessibles.

seems like that would be better where this var is used.

>+  bool isTopContentDoc = isRootDoc ? XRE_IsContentProcess() : false;

I think you could just check when creating the object.

> 
>   DocAccessible* parentDocAcc = nullptr;
>-  if (!isRootDoc) {
>+  if (!isRootDoc && !isTopContentDoc) {

isRootDoc being false implies isTopContentDoc is false.

>@@ -434,32 +436,32 @@ DocManager::CreateDocOrRootAccessible(ns
>   // Cache the document accessible into document cache.
>   mDocAccessibleCache.Put(aDocument, docAcc);

and you don't actually effect the type of the object created???

> 
>   // Initialize the document accessible.
>   docAcc->Init();
>   docAcc->SetRoleMapEntry(aria::GetRoleMap(aDocument));
> 
>   // Bind the document to the tree.
>-  if (isRootDoc) {
>+  if (isRootDoc || isTopContentDoc) {

same thing about !isRootDoc implies !isTopCOntentDoc

>     if (!ApplicationAcc()->AppendChild(docAcc)) {
>       docAcc->Shutdown();
>       return nullptr;
>     }
> 
>     // Fire reorder event to notify new accessible document has been attached to
>     // the tree. The reorder event is delivered after the document tree is
>     // constructed because event processing and tree construction are done by
>     // the same document.
>     // Note: don't use AccReorderEvent to avoid coalsecense and special reorder
>     // events processing.
>     docAcc->FireDelayedEvent(nsIAccessibleEvent::EVENT_REORDER,
>                              ApplicationAcc());

we don't need to do that in the child process, and not doing it there would allow us to remove some stuff from HandleAccEvent

> 
>-    if (IPCAccessibilityActive()) {
>+    if (!isTopContentDoc && IPCAccessibilityActive()) {

seems inverted

	Lorien Hu (:lsocks) Reporter
	Comment 4 • 9 years ago

I changed isRootDoc to only be true if it's not in the child process.

There's a function call AddListeners(aDocument, isRootDoc) at the end that I'm not sure about. Are DOMContentLoaded listeners relevant to top level docaccessibles?

	Lorien Hu (:lsocks) Reporter
	Comment 6 • 9 years ago

Comment on attachment 8636705 [details] [diff] [review]
refactor DocManager::CreateDocOrRootAccessible to never create root accessibles in content processes

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

just nitpicking

::: accessible/base/DocManager.cpp
@@ +411,5 @@
>    if (!presShell || presShell->IsDestroying())
>      return nullptr;
>  
> +  nsIContent *rootElm = nsCoreUtils::GetRoleContent(aDocument);
> +  nsIDocument* parentDoc = aDocument->GetParentDocument();

the placement of the * is not the same between these two lines

@@ +416,3 @@
>    DocAccessible* parentDocAcc = nullptr;
> +  nsRefPtr<DocAccessible> docAcc;
> +  bool needIPCMsg = false, isRootAcc = false;

Could do without needIPCMsg, see comments below

@@ +416,4 @@
>    DocAccessible* parentDocAcc = nullptr;
> +  nsRefPtr<DocAccessible> docAcc;
> +  bool needIPCMsg = false, isRootAcc = false;
> +  if (parentDocAcc) {

should be if (parentDoc)

@@ +427,5 @@
>  
> +    docAcc = new DocAccessibleWrap(aDocument, rootElm, presShell);
> +  } else if (XRE_IsContentProcess()) {
> +    docAcc = new DocAccessibleWrap(aDocument, rootElm, presShell);
> +    needIPCMsg= true;

missing space between needIPCMsg and =

@@ +443,5 @@
>    docAcc->Init();
>    docAcc->SetRoleMapEntry(aria::GetRoleMap(aDocument));
>  
>    // Bind the document to the tree.
> +  if ((isRootAcc || needIPCMsg) && !ApplicationAcc()->AppendChild(docAcc)) {

can check for !parentDoc here instead of isRootAcc || needIPCMsg

@@ +459,4 @@
>      docAcc->FireDelayedEvent(nsIAccessibleEvent::EVENT_REORDER,
>                               ApplicationAcc());
>  
> +  if (needIPCMsg && IPCAccessibilityActive()) {

could check for !parentDoc && IPCAccessibilityActive() since the IPCAccessibilityActive() only is true for content process

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

It actually ended up not being too different from what we used to have, but it seems somewhat better

	Lorien Hu (:lsocks) Reporter
	Comment 8 • 9 years ago

I believe nsAccessibilityService::PresShellActivated also needs a change or we'll get an assertion failure there when it tries to find a root doc.

	Lorien Hu (:lsocks) Reporter
	Comment 9 • 9 years ago

Just found this, needs to be changed as well: DocAccessible.cpp:1452 in DoInitialUpdate

if (!IsRoot()) {
    nsRefPtr<AccReorderEvent> reorderEvent = new AccReorderEvent(Parent());
    ParentDocument()->FireDelayedEvent(reorderEvent);

	Lorien Hu (:lsocks) Reporter
	Comment 10 • 9 years ago

Nevermind, ignore the last comment. The other one still holds, though.

	Lorien Hu (:lsocks) Reporter
	Comment 11 • 9 years ago

I retract my retraction, both of those are still issues. I just ran with that patch.

	Lorien Hu (:lsocks) Reporter
	Comment 13 • 9 years ago

Comment on attachment 8643698 [details] [diff] [review]
don't try and fire platform events in the child process

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

nit: I think we're supposed to be moving to always brace

	Pulsebot
	Comment 14 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/92470c43a4cc

	Carsten Book [:Tomcat]
	Comment 15 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/92470c43a4cc