decomtaminate impl of GetSummary() on accessible tables

RESOLVED FIXED in mozilla14

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tbsaunde, Assigned: maxli)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 attachment, 1 obsolete attachment)

7.35 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
see procedure in bug 739882 accept this time with GetSummary()
(Assignee)

Comment 1

5 years ago
Created attachment 615206 [details] [diff] [review]
Patch (v1)
Assignee: nobody → maxli
Attachment #615206 - Flags: feedback?(trev.saunders)

Comment 2

5 years ago
Comment on attachment 615206 [details] [diff] [review]
Patch (v1)

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

f=me with comments addressed, requesting r from Trevor

::: accessible/src/generic/ARIAGridAccessible.h
@@ +65,5 @@
>    // nsIAccessibleTable
>    NS_DECL_OR_FORWARD_NSIACCESSIBLETABLE_WITH_XPCACCESSIBLETABLE
>  
> +  // TableAccessible
> +  virtual void Summary(nsString& aSummary);

you don't need to override it since it does the same as TableAccessible::Summary

::: accessible/src/html/nsHTMLTableAccessible.cpp
@@ +561,2 @@
>  
> +  table->GetSummary(aSummary);

I'm not sure whether things like <div style="display: table"> implement nsIDOMHTMLTableElement interface, it's safer to have null check like
if (table)
  table->GetSummary(aSummary);

::: accessible/src/xul/nsXULListboxAccessible.h
@@ +104,5 @@
>    // nsIAccessibleTable
>    NS_DECL_OR_FORWARD_NSIACCESSIBLETABLE_WITH_XPCACCESSIBLETABLE
>  
> +  // TableAccessible
> +  virtual void Summary(nsString& aSummary);

you don't need it as well

::: accessible/src/xul/nsXULTreeGridAccessible.h
@@ +60,5 @@
>    // nsIAccessibleTable
>    NS_DECL_OR_FORWARD_NSIACCESSIBLETABLE_WITH_XPCACCESSIBLETABLE
>  
> +  // TableAccessible
> +  virtual void Summary(nsString& aSummary);

same
Attachment #615206 - Flags: review?(trev.saunders)
Attachment #615206 - Flags: feedback?(trev.saunders)
Attachment #615206 - Flags: feedback+
(Assignee)

Comment 3

5 years ago
Created attachment 615231 [details] [diff] [review]
Patch v2

Comments addressed
Attachment #615206 - Attachment is obsolete: true
Attachment #615206 - Flags: review?(trev.saunders)
Attachment #615231 - Flags: review?(trev.saunders)
(Reporter)

Comment 4

5 years ago
Comment on attachment 615231 [details] [diff] [review]
Patch v2

>+  nsAutoString summary(aSummary);
>+  mTable->Summary(summary);

don't pass anything to the nsAutoString constructor.

r=me with that
Attachment #615231 - Flags: review?(trev.saunders) → review+

Comment 5

5 years ago
Comment on attachment 615231 [details] [diff] [review]
Patch v2


>+nsHTMLTableAccessible::Summary(nsString& aSummary)
> {
>   nsCOMPtr<nsIDOMHTMLTableElement> table(do_QueryInterface(mContent));
>-  NS_ENSURE_TRUE(table, NS_ERROR_FAILURE);
>-
>-  return table->GetSummary(aSummary);
>+  

empty line should be empty (no spaces)

I'll fix nits before landing

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3302067526f6
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/3302067526f6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.