751497 - replace nsHTMLSelectOptionAccessible::GetSelectState by nice inline

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

You could add inline GetSelect() as

if (mParent && mParent->IsListControl()) {
  nsAccessible* combobox = mParent->Parent();
  return combobox->IsCombobox() ? combobox : mParent;
}
return nsnull;

and then remove GetSelectState

http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHTMLSelectAccessible.cpp#406

please wait until bug 751496 is fixed

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

Requesting feedback from the mentor for this small technical change ... builds and tests ok on WIN7 machine.

Let me know if you spot anything that needs >help<  -- mark

Comment 2

[Hubert Figuiere [:hub]]

Comment on attachment 622909 [details] [diff] [review]
Patch (v1)

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

::: accessible/src/html/nsHTMLSelectAccessible.h
@@ +138,5 @@
> +  nsAccessible* GetSelect() const
> +  {
> +    if (mParent && mParent->IsListControl()) {
> +      nsAccessible* combobox = mParent->Parent();
> +      return combobox->IsCombobox() ? combobox : mParent;

I'd check for combobox to be not null as well.

Comment 3

[Mark Capella [:capella]]

I notice inline function GetCombobox() in the header file which is pretty much the exact same logic. I'll null check both of them then.

Comment 4

[Mark Capella [:capella]]

Attachment: Patch (v2)

Added null guards ...

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 622974 [details] [diff] [review]
Patch (v2)

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

I'd r+ that if the patch wouldn't add extra perf problems

::: accessible/src/html/nsHTMLSelectAccessible.cpp
@@ +264,5 @@
>    // Upcall to nsAccessible, but skip nsHyperTextAccessible impl
>    // because we don't want EDITABLE or SELECTABLE_TEXT
>    PRUint64 state = nsAccessible::NativeState();
>  
> +  nsAccessible* selectContent = GetSelect();

don't use 'content' for accessible object name. I'd use 'select'

@@ +281,5 @@
>      if (isSelected)
>        state |= states::SELECTED;
>    }
>  
> +  if (selectContent->State() & states::OFFSCREEN) {

don't get State() more than once, it's perf

::: accessible/src/html/nsHTMLSelectAccessible.h
@@ +132,5 @@
>  
>  private:
>    
>    /**
> +   * Return an accessible the option belongs to if any.

"Return a select accessible" otherwise it's ambigious

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v3)

Nits and performance issue addressed, re-built, re-tested locally.

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 623025 [details] [diff] [review]
Patch (v3)

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

r=me, thank you! Mark, are you going to land it? :)

::: accessible/src/html/nsHTMLSelectAccessible.cpp
@@ +266,5 @@
>    PRUint64 state = nsAccessible::NativeState();
>  
> +  nsAccessible* select = GetSelect();
> +  if (!select)
> +    return state;

I'd use empty line here

Comment 8

[Mark Capella [:capella]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/386fac446673

Comment 9

[Ed Morley [:emorley]]

Sorry, had to back out for compilation failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=386fac446673

eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=11669966&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/733897688bed

Comment 10

[Mark Capella [:capella]]

Attachment: Patch (v4)

New version due to fails on linux builds ...
push to try: https://tbpl.mozilla.org/?tree=Try&rev=a78ef83ea707

Comment 11

[Mark Capella [:capella]]

Try seemed to work for linux, Windows works for me locally ...
Pushing to inbound again...

https://hg.mozilla.org/integration/mozilla-inbound/rev/d17410f2c261

Comment 12

[Mark Capella [:capella]]

Inbound status:

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d17410f2c261

Comment 13

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/d17410f2c261