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

      No description provided.

	Lorien Hu (:lsocks) Assignee
	Comment 1 • 9 years ago

The way this is set up isn't very clean, but I'm not sure if there's a better way to do it.

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

Comment on attachment 8639029 [details] [diff] [review]
proxies in role, subrole

>+DocAccessibleChild::RecvLandmarkRole(const uint64_t& aID, nsString* aLandmark)
>+{
>+  Accessible* acc = IdToAccessible(aID);
>+  if (!acc)
>+    return true;
>+
>+  if (nsIAtom* roleAtom = (acc->LandmarkRole()))

isn't there some extra parens there?

>+{
>+  Accessible* acc = IdToAccessible(aID);
>+  if (!acc)
>+    return true;
>+
>+  if (nsIAtom* roleAtom = (*(acc->ARIARoleMap()->roleAtom)))

I think RoleMap() can return null

>+void
>+ProxyAccessible::LandmarkRole(nsIAtom* aLandmark) const
>+{
>+  nsString landmark;
>+  unused << mDoc->SendLandmarkRole(mID, &landmark);
>+  nsCOMPtr<nsIAtom> landmarkAtom = do_GetAtom(landmark);
>+  *aLandmark = *landmarkAtom.get();

how exactly do you expect that to work?  Why aren't you just returning a nsIAtom*?

>+}
>+
>+void ProxyAccessible::ARIARoleAtom(nsIAtom* aRole) const
>+{
>+  nsString role;
>+  unused << mDoc->SendARIARoleAtom(mID, &role);
>+  nsCOMPtr<nsIAtom> roleAtom = do_GetAtom(role);
>+  *aRole = *roleAtom.get();

same

>-  if (accWrap->HasARIARole()) {
>+  if (accWrap && accWrap->HasARIARole()) {
>     nsRoleMapEntry* roleMap = accWrap->ARIARoleMap();
>     if (roleMap->Is(nsGkAtoms::alert))
>       return @"AXApplicationAlert";
>     if (roleMap->Is(nsGkAtoms::alertdialog))
>       return @"AXApplicationAlertDialog";
>     if (roleMap->Is(nsGkAtoms::article))
>       return @"AXDocumentArticle";
>     if (roleMap->Is(nsGkAtoms::dialog))
>@@ -970,23 +975,58 @@ ConvertToNSArray(nsTArray<ProxyAccessibl
>       return @"AXApplicationStatus";
>     if (roleMap->Is(nsGkAtoms::tabpanel))
>       return @"AXTabPanel";
>     if (roleMap->Is(nsGkAtoms::timer))
>       return @"AXApplicationTimer";
>     if (roleMap->Is(nsGkAtoms::tooltip))
>       return @"AXUserInterfaceTooltip";
>   }
>+  if (proxy) {
>+    nsIAtom* roleAtom = nullptr;
>+    proxy->ARIARoleAtom(roleAtom);

so not only are you copying the atom, but you are trying to copy it to 0x0???

>+    if (roleAtom) {
>+      if (roleAtom == nsGkAtoms::alert)
>+        return @"AXApplicationAlert";
>+      if (roleAtom == nsGkAtoms::alertdialog)
>+        return @"AXApplicationAlertDialog";
>+      if (roleAtom == nsGkAtoms::article)
>+        return @"AXDocumentArticle";
>+      if (roleAtom == nsGkAtoms::dialog)
>+        return @"AXApplicationDialog";
>+      if (roleAtom == nsGkAtoms::document)
>+        return @"AXDocument";
>+      if (roleAtom == nsGkAtoms::log_)
>+        return @"AXApplicationLog";
>+      if (roleAtom == nsGkAtoms::marquee)
>+        return @"AXApplicationMarquee";
>+      if (roleAtom == nsGkAtoms::math)
>+        return @"AXDocumentMath";
>+      if (roleAtom == nsGkAtoms::note_)
>+        return @"AXDocumentNote";
>+      if (roleAtom == nsGkAtoms::region)
>+        return @"AXDocumentRegion";
>+      if (roleAtom == nsGkAtoms::status)
>+        return @"AXApplicationStatus";
>+      if (roleAtom == nsGkAtoms::tabpanel)
>+        return @"AXTabPanel";
>+      if (roleAtom == nsGkAtoms::timer)
>+        return @"AXApplicationTimer";
>+      if (roleAtom == nsGkAtoms::tooltip)
>+        return @"AXUserInterfaceTooltip";
>+    }

why can't you get the atom and share the rest of this?

	Trevor Saunders (:tbsaunde)
	Comment 5 • 9 years ago

Comment on attachment 8641734 [details] [diff] [review]
proxies in role, subrole

>+  if (nsIAtom* roleAtom = acc->LandmarkRole())
>+    roleAtom->ToString(*aLandmark);
>+  return true;

blank line before return please

>+  if (nsRoleMapEntry* roleMap = acc->ARIARoleMap())
>+    if (nsIAtom* roleAtom = *(roleMap->roleAtom))
>+      roleAtom->ToString(*aRole);
>+  return true;

same

>+  virtual bool RecvARIARoleAtom(const uint64_t& aID, nsString* aRole);

override

>+ProxyAccessible::LandmarkRole() const
>+{
>+  nsString landmark;
>+  unused << mDoc->SendLandmarkRole(mID, &landmark);
>+  nsCOMPtr<nsIAtom> landmarkAtom = do_GetAtom(landmark);
>+  return landmarkAtom.get();

just use NS_GetStaticAtom().   those methods on Accessible only ever return static atoms, so the child is broken if it sends us something else, and we don't want to create atoms we won't use for anything.

>+ProxyAccessible::ARIARoleAtom() const
>+{
>+  nsString role;
>+  unused << mDoc->SendARIARoleAtom(mID, &role);
>+  nsCOMPtr<nsIAtom> roleAtom = do_GetAtom(role);
>+  return roleAtom.get();

same

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

carry r=tbsaunde

	Pulsebot
	Comment 7 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/081d48ca7e97

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

https://hg.mozilla.org/mozilla-central/rev/081d48ca7e97