Last Comment Bug 751497 - replace nsHTMLSelectOptionAccessible::GetSelectState by nice inline
: replace nsHTMLSelectOptionAccessible::GetSelectState by nice inline
Status: RESOLVED FIXED
[good first bug][mentor=hub@mozilla.c...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Mark Capella [:capella]
:
:
Mentors:
Depends on: 751496
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2012-05-03 01:37 PDT by alexander :surkov
Modified: 2012-05-11 11:44 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (4.59 KB, patch)
2012-05-10 14:06 PDT, Mark Capella [:capella]
hub: feedback+
Details | Diff | Splinter Review
Patch (v2) (4.83 KB, patch)
2012-05-10 16:54 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v3) (3.76 KB, patch)
2012-05-10 20:21 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review
Patch (v4) (3.84 KB, patch)
2012-05-11 03:39 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review

Description alexander :surkov 2012-05-03 01:37:35 PDT
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] 2012-05-10 14:06:28 PDT
Created attachment 622909 [details] [diff] [review]
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] 2012-05-10 14:35:44 PDT
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] 2012-05-10 16:11:49 PDT
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] 2012-05-10 16:54:17 PDT
Created attachment 622974 [details] [diff] [review]
Patch (v2)

Added null guards ...
Comment 5 alexander :surkov 2012-05-10 19:04:35 PDT
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] 2012-05-10 20:21:24 PDT
Created attachment 623025 [details] [diff] [review]
Patch (v3)

Nits and performance issue addressed, re-built, re-tested locally.
Comment 7 alexander :surkov 2012-05-10 20:28:25 PDT
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 10 Mark Capella [:capella] 2012-05-11 03:39:01 PDT
Created attachment 623094 [details] [diff] [review]
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] 2012-05-11 04:02:55 PDT
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] 2012-05-11 04:33:20 PDT
Inbound status:

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d17410f2c261
Comment 13 Matt Brubeck (:mbrubeck) 2012-05-11 11:44:00 PDT
https://hg.mozilla.org/mozilla-central/rev/d17410f2c261

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