Make xpcAccessibleValue work with proxied accessibles

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

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

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
Created attachment 8769328 [details] [diff] [review]
Make xpcAccessibleValue work with proxied accessibles. r?tbsaunde
Attachment #8769328 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 3

2 years ago
Created attachment 8769356 [details] [diff] [review]
Make xpcAccessibleValue work with proxied accessibles. r?tbsaunde
Attachment #8769356 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Updated

2 years ago
Attachment #8769328 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 5

2 years ago
Created attachment 8769887 [details] [diff] [review]
Make xpcAccessibleValue work with proxied accessibles. r?tbsaunde
Attachment #8769887 - Flags: review?(tbsaunde+mozbugs)
(Assignee)

Updated

2 years ago
Attachment #8769356 - Attachment is obsolete: true
Attachment #8769887 - Flags: review?(tbsaunde+mozbugs) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 6

2 years ago
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

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9e87f6e3d61f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Assignee)

Comment 9

2 years ago
Created attachment 8770645 [details] [diff] [review]
Remove unsafe assignment in xpcAccessibleValue::GetMinimumIncrement
(Assignee)

Comment 10

2 years ago
I missed this error in my first patch, my bad.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

2 years ago
Attachment #8770645 - Flags: review+
(Assignee)

Comment 11

2 years ago
Created attachment 8770653 [details] [diff] [review]
Change MinValue() call to CurValue() in xpcAccessibleValue::GetCurrentValue
Attachment #8770653 - Flags: review+
(Assignee)

Comment 12

2 years ago
After these two extra patches are applied, bug 1276721 is fixed and all tests in accessible/tests/browser_caching_values.js pass.
(Assignee)

Updated

2 years ago
Blocks: 1276721
(Assignee)

Updated

2 years ago
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
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Attachment #8770645 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8770653 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.