The default bug view has changed. See this FAQ.

stop using QueryInterface in CAccessibleAction

RESOLVED FIXED in mozilla14

Status

()

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

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

5 years ago
we use QI in the CAccessible classes to get the actual type instead lets use this template stuff as we discussed.  I plan to make following bugs for other interfaces good first bugs.
(Assignee)

Comment 1

5 years ago
Created attachment 606347 [details] [diff] [review]
stop using QueryInterface in CAccessibleAction to get an nsAccessible*
(Assignee)

Updated

5 years ago
Attachment #606347 - Flags: review?(surkov.alexander)

Updated

5 years ago
Attachment #606347 - Flags: superreview?(neil)
Attachment #606347 - Flags: review?(surkov.alexander)
Attachment #606347 - Flags: review+
(Assignee)

Comment 2

5 years ago
Created attachment 606355 [details] [diff] [review]
patch 2

just rename the classes (per  discussion in real life)
Assignee: nobody → trev.saunders
Attachment #606347 - Attachment is obsolete: true
Attachment #606347 - Flags: superreview?(neil)
Attachment #606355 - Flags: superreview?(neil)
Attachment #606355 - Flags: review?(surkov.alexander)

Comment 3

5 years ago
Comment on attachment 606355 [details] [diff] [review]
patch 2

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

r=me as long as Neil is fine with approach

::: accessible/src/msaa/CAccessibleAction.cpp
@@ +135,2 @@
>                                    BSTR **aKeyBinding,
>                                    long *aNumBinding)

align them

::: accessible/src/msaa/CAccessibleHyperlink.h
@@ +51,1 @@
>                              public IAccessibleHyperlink

align
Attachment #606355 - Flags: review?(surkov.alexander) → review+

Comment 4

5 years ago
So, how many classes actually derive from CAccessibleAction and CAccessibleHyperlink? Currently I can only see nsAccessibleWrap.

Also, you didn't remove the dummy virtual nsISupports QueryInterface calls.
(Assignee)

Comment 5

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #4)
> So, how many classes actually derive from CAccessibleAction and
> CAccessibleHyperlink? Currently I can only see nsAccessibleWrap.

I believe that's it.  What does that effect?  We could just throw all of it in nsAccessibleWrap, but keeping code seperate by what interface it supporst seems nice.

> Also, you didn't remove the dummy virtual nsISupports QueryInterface calls.

I assume you mean NS_IMETHOD QueryInterface() = 0; ?  it didn't occur to me before, but yeah we can do that.

Comment 6

5 years ago
(In reply to Trevor Saunders from comment #5)
> (In reply to comment #4)
> > So, how many classes actually derive from CAccessibleAction and
> > CAccessibleHyperlink? Currently I can only see nsAccessibleWrap.
> I believe that's it.  What does that effect?
Well, seems a shame to have a template with only one use, why not just static_cast<nsAccessibleWrap*>(this) your way to victory?
(Assignee)

Comment 7

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #6)
> (In reply to Trevor Saunders from comment #5)
> > (In reply to comment #4)
> > > So, how many classes actually derive from CAccessibleAction and
> > > CAccessibleHyperlink? Currently I can only see nsAccessibleWrap.
> > I believe that's it.  What does that effect?
> Well, seems a shame to have a template with only one use, why not just
> static_cast<nsAccessibleWrap*>(this) your way to victory?

yeah, templating is a bit of a shame, but will msvc allow us to do that static cast? I'd have expected not since nsAccessibleWrap inherits CAccessibleAction, so I'd think it has no way to know at the site of the cast that its a downcast.  I don't have an easy way to check so I would be willing to try that if you suspect it will work.

Comment 8

5 years ago
(In reply to Trevor Saunders from comment #7)
> (In reply to comment #6)
> > (In reply to Trevor Saunders from comment #5)
> > > (In reply to comment #4)
> > > > So, how many classes actually derive from CAccessibleAction and
> > > > CAccessibleHyperlink? Currently I can only see nsAccessibleWrap.
> > > I believe that's it.  What does that effect?
> > Well, seems a shame to have a template with only one use, why not just
> > static_cast<nsAccessibleWrap*>(this) your way to victory?
> yeah, templating is a bit of a shame, but will msvc allow us to do that
> static cast? I'd have expected not since nsAccessibleWrap inherits
> CAccessibleAction, so I'd think it has no way to know at the site of the
> cast that its a downcast.
Surely it would have to be able to do that for the template anyway?
(Assignee)

Comment 9

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #8)
> (In reply to Trevor Saunders from comment #7)
> > (In reply to comment #6)
> > > (In reply to Trevor Saunders from comment #5)
> > > > (In reply to comment #4)
> > > > > So, how many classes actually derive from CAccessibleAction and
> > > > > CAccessibleHyperlink? Currently I can only see nsAccessibleWrap.
> > > > I believe that's it.  What does that effect?
> > > Well, seems a shame to have a template with only one use, why not just
> > > static_cast<nsAccessibleWrap*>(this) your way to victory?
> > yeah, templating is a bit of a shame, but will msvc allow us to do that
> > static cast? I'd have expected not since nsAccessibleWrap inherits
> > CAccessibleAction, so I'd think it has no way to know at the site of the
> > cast that its a downcast.
> Surely it would have to be able to do that for the template anyway?

yes, but I believe the timing is different since in the template case it can only check when it knows the template is instantiated, and so has access to the full definition of the type.  However I could be wrong, Alex mind changing one to just static cast without the templating and see if msvc likes it?  If just static casting to the inheriting type works I really wonder why that wasn't done originally...

Comment 10

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #9)

> yes, but I believe the timing is different since in the template case it can
> only check when it knows the template is instantiated, and so has access to
> the full definition of the type.  However I could be wrong, Alex mind
> changing one to just static cast without the templating and see if msvc
> likes it?  If just static casting to the inheriting type works I really
> wonder why that wasn't done originally...

I don't think it works. Trevor, you could try to run try server build ;)
(In reply to Trevor Saunders from comment #9)
> (In reply to neil@parkwaycc.co.uk from comment #8)
> > (In reply to Trevor Saunders from comment #7)
> > > (In reply to comment #6)
> > > > (In reply to Trevor Saunders from comment #5)
> > > > > (In reply to comment #4)
> > > > > > So, how many classes actually derive from CAccessibleAction and
> > > > > > CAccessibleHyperlink? Currently I can only see nsAccessibleWrap.
> > > > > I believe that's it.  What does that effect?
> > > > Well, seems a shame to have a template with only one use, why not just
> > > > static_cast<nsAccessibleWrap*>(this) your way to victory?
> > > yeah, templating is a bit of a shame, but will msvc allow us to do that
> > > static cast? I'd have expected not since nsAccessibleWrap inherits
> > > CAccessibleAction, so I'd think it has no way to know at the site of the
> > > cast that its a downcast.
> > Surely it would have to be able to do that for the template anyway?
> yes, but I believe the timing is different since in the template case it can
> only check when it knows the template is instantiated, and so has access to
> the full definition of the type.
This would be useful if you were defining the template in the header file and instantiating it in the cpp file, but you're not doing that here.
(Assignee)

Comment 12

5 years ago
ok, https://tbpl.mozilla.org/?tree=Try&rev=9479548e2514 should tell us what msvc thinks of just static casting to nsAccessible*
(Assignee)

Comment 13

5 years ago
ok, looks like Alex was right it doesn't compile the related bit of the log is

  -DUNICODE -D_UNICODE -DNOMINMAX -D_CRT_RAND_S -DCERT_CHAIN_PARA_HAS_EXTRA_FIELDS -D_SECURE_ATL -DCHROMIUM_BUILD -DU_STATIC_IMP
LEMENTATION -DOS_WIN=1 -DWIN32 -D_WIN32 -D_WINDOWS -DWIN32_LEAN_AND_MEAN  -DCOMPILER_MSVC -I/e/builds/moz2_slave/try-w32/build/i
pc/chromium/src -I/e/builds/moz2_slave/try-w32/build/ipc/glue -I../../../ipc/ipdl/_ipdlheaders  -I/e/builds/moz2_slave/try-w32/b
uild/accessible/src/msaa -I/e/builds/moz2_slave/try-w32/build/accessible/src/msaa/../base -I/e/builds/moz2_slave/try-w32/build/a
ccessible/src/msaa/../html -I/e/builds/moz2_slave/try-w32/build/accessible/src/msaa/../xul -I/e/builds/moz2_slave/try-w32/build/
accessible/src/msaa/../../../content/base/src -I/e/builds/moz2_slave/try-w32/build/accessible/src/msaa/../../../content/events/s
rc  -I/e/builds/moz2_slave/try-w32/build/accessible/src/msaa -I. -I../../../dist/include -I../../../dist/include/nsprpub  -Ie:/b
uilds/moz2_slave/try-w32/build/obj-firefox/dist/include/nspr -Ie:/builds/moz2_slave/try-w32/build/obj-firefox/dist/include/nss
      -TP -nologo -W3 -Gy -Fdgenerated.pdb -wd4800 -we4553  -DNDEBUG -DTRIMMED -Zi -UDEBUG -DNDEBUG -O1 -Oy -MD            -FI .
./../../dist/include/mozilla-config.h -DMOZILLA_CLIENT /e/builds/moz2_slave/try-w32/build/accessible/src/msaa/CAccessibleAction.
cpp
CAccessibleAction.cpp

e:/builds/moz2_slave/try-w32/build/accessible/src/msaa/CAccessibleAction.cpp(74) : error C2440: 'static_cast' : cannot convert f
rom 'CAccessibleAction *const ' to 'nsAccessible *'

        Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast

make[7]: Leaving directory `/e/builds/moz2_slave/try-w32/build/obj-firefox/accessible/src/msaa'
make[6]: Leaving directory `/e/builds/moz2_slave/try-w32/build/obj-firefox/accessible/src'
Is it not possible to add an include for nsAccessibleWrap.h?
(Assignee)

Comment 15

5 years ago
oh, lol at me, I changed that and pushed https://tbpl.mozilla.org/?tree=Try&rev=9db2d14bd0a4
(Assignee)

Comment 16

5 years ago
If we are going to just static cast to nsAccessible is there a nice way we can assert at build time that only one class directly inherits us?
Make your constructor private and your subclass a friend.
(Assignee)

Comment 18

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #17)
> Make your constructor private and your subclass a friend.

doa


(In reply to neil@parkwaycc.co.uk from comment #14)
> Is it not possible to add an include for nsAccessibleWrap.h?

looks like that doesn't help.
Comment on attachment 606355 [details] [diff] [review]
patch 2

So, this is a Windows-only patch, but it uses renames, which I can't apply using patch, and mozillabuild doesn't include git-apply, and hg import requires a clean tree, and mq sucks because it requires a full rebuild...
(Assignee)

Comment 20

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #19)
> Comment on attachment 606355 [details] [diff] [review]
> patch 2
> 
> So, this is a Windows-only patch, but it uses renames, which I can't apply
> using patch, and mozillabuild doesn't include git-apply, and hg import
> requires a clean tree, and mq sucks because it requires a full rebuild...

ok, :( I'll provide a non-rename using patch in a minute
(Assignee)

Comment 21

5 years ago
Created attachment 608929 [details] [diff] [review]
patch without renames
(Assignee)

Comment 22

5 years ago
Created attachment 608933 [details] [diff] [review]
no renames -U8 -p
Comment on attachment 608929 [details] [diff] [review]
patch without renames

This failed to compile.
>accessible\src\base\Relation.h(151) : warning C4522: 'Relation' : multiple assignment operators specified
>accessible/src/msaa/nsAccessibleWrap.cpp(142) : error C2065: 'derived' : undeclared identifier
(Assignee)

Comment 24

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #23)
> Comment on attachment 608929 [details] [diff] [review]
> patch without renames
> 
> This failed to compile.
> >accessible\src\base\Relation.h(151) : warning C4522: 'Relation' : multiple assignment operators specified
> >accessible/src/msaa/nsAccessibleWrap.cpp(142) : error C2065: 'derived' : undeclared identifier

! I mudy hsve pushed the wrong thing to try or something.  anyway I changed derived to nsAccessibleWrap and pushed that to try as https://tbpl.mozilla.org/?tree=Try&rev=ac9ac1518b06
(Assignee)

Comment 25

5 years ago
Created attachment 609151 [details] [diff] [review]
patch that builds

I feel like I must have made these changes to the patch before, but must hve lost them some how, anyway this should build (see https://tbpl.mozilla.org/?tree=Try&rev=45a5c262cc08 
 )
Attachment #606355 - Attachment is obsolete: true
Attachment #608929 - Attachment is obsolete: true
Attachment #608933 - Attachment is obsolete: true
Attachment #606355 - Flags: superreview?(neil)
Attachment #609151 - Flags: superreview?(neil)
Comment on attachment 609151 [details] [diff] [review]
patch that builds

>+#include "nsAccessibleWrap.h"
>+
>+template class ia2AccessibleAction<nsAccessibleWrap>;
Well this is a dead giveaway that you should just use nsAccessibleWrap throughout instead of templating on derived.
Created attachment 609743 [details] [diff] [review]
What you actually end up doing

Once you remove the renaming and the templates, this is all you've done...
Attachment #609151 - Attachment is obsolete: true
Attachment #609151 - Flags: superreview?(neil)
(Assignee)

Comment 28

5 years ago
(In reply to neil@parkwaycc.co.uk from comment #26)
> Comment on attachment 609151 [details] [diff] [review]
> patch that builds
> 
> >+#include "nsAccessibleWrap.h"
> >+
> >+template class ia2AccessibleAction<nsAccessibleWrap>;
> Well this is a dead giveaway that you should just use nsAccessibleWrap
> throughout instead of templating on derived.

yeah, I just thought that didn't build even when including nsAccessibleWrap.h see comment 18, but for some reason I didn't include a tbpl link, so its hard to know what I got wrong.

(In reply to neil@parkwaycc.co.uk from comment #27)
> Created attachment 609743 [details] [diff] [review]
> What you actually end up doing
> 
> Once you remove the renaming and the templates, this is all you've done...

ok, that's basically the patch so should we review that and I'll do the renames for a part 2?
(Assignee)

Comment 29

5 years ago
Comment on attachment 609743 [details] [diff] [review]
What you actually end up doing

I'll do a second part to rename everything.
Attachment #609743 - Flags: review?(surkov.alexander)
(Assignee)

Comment 30

5 years ago
Created attachment 609796 [details] [diff] [review]
part 2
Attachment #609796 - Flags: review?(surkov.alexander)

Comment 31

5 years ago
Comment on attachment 609743 [details] [diff] [review]
What you actually end up doing

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

that's why I like to ask Neil for superreview :)

I wonder if we can do similar for CAccessibleTable casting to nsAccessibleWrap and ITableAccessible by including files like nsARIAGridAccessibleWrap.h
Attachment #609743 - Flags: review?(surkov.alexander) → review+

Updated

5 years ago
Attachment #609796 - Flags: review?(surkov.alexander) → review+

Comment 32

5 years ago
One thing: we should remove nsISupports::QueryInterface from CAccessibleAction
(In reply to alexander surkov from comment #31)
> I wonder if we can do similar for CAccessibleTable casting to
> nsAccessibleWrap and ITableAccessible by including files like
> nsARIAGridAccessibleWrap.h
No, the patch only works because nsAccessibleWrap is the only class that inherits from CAccessibleHyperlink/CAccessibleAction. (In theory you could remove those classes and just add their methods directly to nsAccessibleWrap.)
(Assignee)

Comment 34

5 years ago
landed with the dummy nsISupports qi rmed
https://hg.mozilla.org/integration/mozilla-inbound/rev/b569eb963ce1
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec9582b2ad15
https://hg.mozilla.org/mozilla-central/rev/b569eb963ce1
https://hg.mozilla.org/mozilla-central/rev/ec9582b2ad15
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.