turn EnumRoleAccessible into template

RESOLVED FIXED in mozilla39

Status

()

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

People

(Reporter: surkov, Assigned: surkov)

Tracking

unspecified
mozilla39
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8572100 [details] [diff] [review]
patch

we'll save 4 bytes per instance.
Attachment #8572100 - Flags: review?(dbolter)

Comment 1

3 years ago
Comment on attachment 8572100 [details] [diff] [review]
patch

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

::: accessible/generic/BaseAccessibles.cpp
@@ +240,3 @@
>  {
> +  return Role;
> +}*/

This looks strange. Was this meant to be removed? It's commented out.
Comment on attachment 8572100 [details] [diff] [review]
patch

>+template<a11y::role R>
>+class RoleTAccessible : public AccessibleWrap

the T in the name is pretty weird.

might as well make it final

>+  virtual nsresult QueryInterface(REFNSIID aIID, void** aPtr) MOZ_OVERRIDE
>+    { return Accessible::QueryInterface(aIID, aPtr); }

I don't think that serves any useful purpose.
(Assignee)

Comment 3

3 years ago
(In reply to Marco Zehe (:MarcoZ) from comment #1)

> > +}*/
> 
> This looks strange. Was this meant to be removed? It's commented out.

right, forgot to remove

(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> Comment on attachment 8572100 [details] [diff] [review]
> patch
> 
> >+template<a11y::role R>
> >+class RoleTAccessible : public AccessibleWrap
> 
> the T in the name is pretty weird.

other ideas? RoleAccessible seems strange, PredefinedRoleAccessible too long

> might as well make it final

k

> >+  virtual nsresult QueryInterface(REFNSIID aIID, void** aPtr) MOZ_OVERRIDE
> >+    { return Accessible::QueryInterface(aIID, aPtr); }
> 
> I don't think that serves any useful purpose.

it disconnects XPCOM and MSAA QI versions
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > Comment on attachment 8572100 [details] [diff] [review]
> > patch
> > 
> > >+template<a11y::role R>
> > >+class RoleTAccessible : public AccessibleWrap
> > 
> > the T in the name is pretty weird.
> 
> other ideas? RoleAccessible seems strange, PredefinedRoleAccessible too long

RoleAccessible seems fine to me, or you could just keep EnumRoleAccessible which still seems fine too.

> > >+  virtual nsresult QueryInterface(REFNSIID aIID, void** aPtr) MOZ_OVERRIDE
> > >+    { return Accessible::QueryInterface(aIID, aPtr); }
> > 
> > I don't think that serves any useful purpose.
> 
> it disconnects XPCOM and MSAA QI versions

I'm kind of suspicious of that being necessary, but I guess I don't care enough ot investigate.
(Assignee)

Comment 5

3 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #4)

> RoleAccessible seems fine to me, or you could just keep EnumRoleAccessible
> which still seems fine too.

ok, then perhaps EnumRoleAccessible, no change is better than disputable change


> > it disconnects XPCOM and MSAA QI versions
> 
> I'm kind of suspicious of that being necessary, but I guess I don't care
> enough ot investigate.

so am I, just following the pattern we do
Comment on attachment 8572100 [details] [diff] [review]
patch

Looks like others are reviewing :)
Attachment #8572100 - Flags: review?(dbolter)
(Assignee)

Comment 7

3 years ago
Comment on attachment 8572100 [details] [diff] [review]
patch

that's fine, comments are welcome, however review is still required
Attachment #8572100 - Flags: review?(dbolter)
OK you can assume an r+ from me with the comments addressed :)
(Assignee)

Comment 9

3 years ago
(In reply to David Bolter [:davidb] from comment #8)
> OK you can assume an r+ from me with the comments addressed :)

do you need fresh patch?
Comment on attachment 8572100 [details] [diff] [review]
patch

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

(In reply to alexander :surkov from comment #9)
> (In reply to David Bolter [:davidb] from comment #8)
> > OK you can assume an r+ from me with the comments addressed :)
> 
> do you need fresh patch?

I trust you.
Attachment #8572100 - Flags: review?(dbolter) → review+
(Assignee)

Comment 11

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8d927b5aed3
Assignee: nobody → surkov.alexander
(Assignee)

Comment 12

3 years ago
 https://hg.mozilla.org/integration/mozilla-inbound/rev/162be88fcce8

Comment 13

3 years ago
These patches landed on MC
https://hg.mozilla.org/mozilla-central/rev/f8d927b5aed3
https://hg.mozilla.org/mozilla-central/rev/162be88fcce8
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.