replace nsHTMLSelectOptionAccessible::GetSelectState by nice inline

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: capella)

Tracking

(Blocks: 1 bug)

unspecified
mozilla15
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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)

Updated

5 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 1

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

Comment 3

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

Comment 4

5 years ago
Created attachment 622974 [details] [diff] [review]
Patch (v2)

Added null guards ...
Attachment #622909 - Attachment is obsolete: true
Attachment #622974 - Flags: review?(surkov.alexander)
(Reporter)

Comment 5

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

Comment 6

5 years ago
Created attachment 623025 [details] [diff] [review]
Patch (v3)

Nits and performance issue addressed, re-built, re-tested locally.
Attachment #622974 - Attachment is obsolete: true
(Reporter)

Comment 7

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

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/386fac446673
Target Milestone: --- → mozilla15

Comment 9

5 years ago
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
Target Milestone: mozilla15 → ---
(Assignee)

Comment 10

5 years ago
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
Attachment #623025 - Attachment is obsolete: true
(Assignee)

Comment 11

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

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

Updated

5 years ago
Target Milestone: --- → mozilla15
(Assignee)

Comment 12

5 years ago
Inbound status:

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d17410f2c261
https://hg.mozilla.org/mozilla-central/rev/d17410f2c261
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.