alexander :surkov (:asurkov) Assignee
	Description • 9 years ago

we'll save 4 bytes per instance.

	Marco Zehe (:MarcoZ)
	Comment 1 • 9 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.

	Trevor Saunders (:tbsaunde)
	Comment 2 • 9 years ago

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.

	alexander :surkov (:asurkov) Assignee
	Comment 3 • 9 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

	Trevor Saunders (:tbsaunde)
	Comment 4 • 9 years ago

> (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.

	alexander :surkov (:asurkov) Assignee
	Comment 5 • 9 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

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 6 • 9 years ago

Comment on attachment 8572100 [details] [diff] [review]
patch

Looks like others are reviewing :)

	alexander :surkov (:asurkov) Assignee
	Comment 7 • 9 years ago

Comment on attachment 8572100 [details] [diff] [review]
patch

that's fine, comments are welcome, however review is still required

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 8 • 9 years ago

OK you can assume an r+ from me with the comments addressed :)

	alexander :surkov (:asurkov) Assignee
	Comment 9 • 9 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?

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 10 • 9 years ago

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.

	alexander :surkov (:asurkov) Assignee
	Comment 11 • 9 years ago

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

	alexander :surkov (:asurkov) Assignee
	Comment 12 • 9 years ago

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

	Guilherme Lima
	Comment 13 • 9 years ago

These patches landed on MC
https://hg.mozilla.org/mozilla-central/rev/f8d927b5aed3
https://hg.mozilla.org/mozilla-central/rev/162be88fcce8