Closed Bug 1285643 Opened 4 years ago Closed 4 years ago

Make xpcAccessibleValue work with proxied accessibles

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: michael.li11702, Assigned: michael.li11702)

References

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Attachment #8769328 - Flags: review?(tbsaunde+mozbugs)
Assignee: nobody → mili
Comment on attachment 8769328 [details] [diff] [review]
Make xpcAccessibleValue work with proxied accessibles. r?tbsaunde

> inline Accessible*
> xpcAccessibleValue::Intl()
> {
>   return static_cast<xpcAccessibleGeneric*>(this)->mIntl.AsAccessible();
> }

it might be nice to rewrite things so this function can go away.

> NS_IMETHODIMP
> xpcAccessibleValue::GetMaximumValue(double* aValue)
> {
>   NS_ENSURE_ARG_POINTER(aValue);
>   *aValue = 0;
> 
>   if (Intl()->IsDefunct())
>     return NS_ERROR_FAILURE;
>+  if (IntlGeneric().IsNull())
>+    return NS_ERROR_FAILURE;

those conditions need to be the other way around, if IntlGeneric().IsNull() is true then Intl() will return null.

>+  double value;
>+  if (IntlGeneric().IsAccessible()) {
>+    value = Intl()->MaxValue();
>+  }
>+  else if (IntlGeneric().IsProxy()) {

else if goes on same line as }

also given you have eliminated the other cases the if must be true so else should be fine I believe.

> NS_IMETHODIMP
> xpcAccessibleValue::GetMinimumValue(double* aValue)

Similar comments as the previous function.

> NS_IMETHODIMP
> xpcAccessibleValue::SetCurrentValue(double aValue)

same here
Attachment #8769328 - Flags: review?(tbsaunde+mozbugs)
Attachment #8769356 - Flags: review?(tbsaunde+mozbugs)
Attachment #8769328 - Attachment is obsolete: true
Attachment #8769356 - Attachment description: Make xpcAccessibleValue work with proxied accessibles. → Make xpcAccessibleValue work with proxied accessibles. r?tbsaunde
Comment on attachment 8769356 [details] [diff] [review]
Make xpcAccessibleValue work with proxied accessibles. r?tbsaunde

>-  if (Intl()->IsDefunct())
>+  if (Intl().IsNull())
>     return NS_ERROR_FAILURE;

there should be a blank line here I think

>+  if (Intl().AsAccessible()->IsDefunct())

you don't know Intl().IsAccessible() is true so that isn't safe.

>-  if (Intl()->IsDefunct())
>+  if (Intl().IsNull())
>     return NS_ERROR_FAILURE;
>+  if (Intl().AsAccessible()->IsDefunct())

same here and below.
Attachment #8769356 - Flags: review?(tbsaunde+mozbugs)
Attachment #8769887 - Flags: review?(tbsaunde+mozbugs)
Attachment #8769356 - Attachment is obsolete: true
Attachment #8769887 - Flags: review?(tbsaunde+mozbugs) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e87f6e3d61f
Make xpcAccessibleValue work with proxied accessibles. r=tbsaunde
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9e87f6e3d61f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
I missed this error in my first patch, my bad.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8770645 - Flags: review+
After these two extra patches are applied, bug 1276721 is fixed and all tests in accessible/tests/browser_caching_values.js pass.
Blocks: 1276721
Depends on: 1286610, 1286612
Now that the dependent bugs have landed, marking this one fixed again. Michael, you should probably mark the obsolete patches obsolete, since you actually dealt with them in the other bugs. :-)
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Attachment #8770645 - Attachment is obsolete: true
Attachment #8770653 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.