Last Comment Bug 739883 - decomtaminate impl of GetSummary() on accessible tables
: decomtaminate impl of GetSummary() on accessible tables
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Max Li [:maxli]
:
Mentors:
Depends on:
Blocks: dexpcoma11y
  Show dependency treegraph
 
Reported: 2012-03-27 21:14 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-04-16 08:37 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (9.52 KB, patch)
2012-04-15 16:51 PDT, Max Li [:maxli]
surkov.alexander: feedback+
Details | Diff | Review
Patch v2 (7.35 KB, patch)
2012-04-15 19:45 PDT, Max Li [:maxli]
tbsaunde+mozbugs: review+
Details | Diff | Review

Description Trevor Saunders (:tbsaunde) 2012-03-27 21:14:12 PDT
see procedure in bug 739882 accept this time with GetSummary()
Comment 1 Max Li [:maxli] 2012-04-15 16:51:06 PDT
Created attachment 615206 [details] [diff] [review]
Patch (v1)
Comment 2 alexander :surkov 2012-04-15 19:23:26 PDT
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
Comment 3 Max Li [:maxli] 2012-04-15 19:45:31 PDT
Created attachment 615231 [details] [diff] [review]
Patch v2

Comments addressed
Comment 4 Trevor Saunders (:tbsaunde) 2012-04-15 21:53:29 PDT
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
Comment 5 alexander :surkov 2012-04-16 00:44:22 PDT
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 7 :Ehsan Akhgari (out sick) 2012-04-16 08:37:32 PDT
https://hg.mozilla.org/mozilla-central/rev/3302067526f6

Note You need to log in before you can comment on or make changes to this bug.