Closed Bug 751497 Opened 9 years ago Closed 9 years ago

replace nsHTMLSelectOptionAccessible::GetSelectState by nice inline

Categories

(Core :: Disability Access APIs, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: surkov, Assigned: capella)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][mentor=hub@mozilla.com][lang=c++])

Attachments

(1 file, 3 obsolete files)

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
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch Patch (v1) (obsolete) — Splinter Review
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
Attachment #622909 - Flags: feedback?(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.
Attachment #622909 - Flags: feedback?(hub) → feedback+
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.
Attached patch Patch (v2) (obsolete) — Splinter Review
Added null guards ...
Attachment #622909 - Attachment is obsolete: true
Attachment #622974 - Flags: review?(surkov.alexander)
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
Attachment #622974 - Flags: review?(surkov.alexander)
Attached patch Patch (v3) (obsolete) — Splinter Review
Nits and performance issue addressed, re-built, re-tested locally.
Attachment #622974 - Attachment is obsolete: true
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
Attachment #623025 - Flags: review+
Attached patch Patch (v4)Splinter Review
New version due to fails on linux builds ...
push to try: https://tbpl.mozilla.org/?tree=Try&rev=a78ef83ea707
Attachment #623025 - Attachment is obsolete: true
Try seemed to work for linux, Windows works for me locally ...
Pushing to inbound again...

https://hg.mozilla.org/integration/mozilla-inbound/rev/d17410f2c261
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/d17410f2c261
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.