Last Comment Bug 736252 - stop using QueryInterface in CAccessibleAction
: stop using QueryInterface in CAccessibleAction
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Trevor Saunders (:tbsaunde)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-15 13:59 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-04-02 11:05 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
stop using QueryInterface in CAccessibleAction to get an nsAccessible* (11.86 KB, patch)
2012-03-15 13:59 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Review
patch 2 (8.78 KB, patch)
2012-03-15 14:44 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Review
patch without renames (45.57 KB, patch)
2012-03-23 17:31 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
no renames -U8 -p (46.69 KB, patch)
2012-03-23 17:42 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
patch that builds (46.36 KB, patch)
2012-03-25 13:21 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
What you actually end up doing (3.13 KB, patch)
2012-03-27 09:11 PDT, neil@parkwaycc.co.uk
surkov.alexander: review+
Details | Diff | Review
part 2 (8.17 KB, patch)
2012-03-27 11:13 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Review

Description Trevor Saunders (:tbsaunde) 2012-03-15 13:59:44 PDT
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.
Comment 1 Trevor Saunders (:tbsaunde) 2012-03-15 13:59:47 PDT
Created attachment 606347 [details] [diff] [review]
stop using QueryInterface in CAccessibleAction to get an nsAccessible*
Comment 2 Trevor Saunders (:tbsaunde) 2012-03-15 14:44:22 PDT
Created attachment 606355 [details] [diff] [review]
patch 2

just rename the classes (per  discussion in real life)
Comment 3 alexander :surkov 2012-03-15 14:47:12 PDT
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
Comment 4 neil@parkwaycc.co.uk 2012-03-17 08:35:33 PDT
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.
Comment 5 Trevor Saunders (:tbsaunde) 2012-03-19 16:44:39 PDT
(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 neil@parkwaycc.co.uk 2012-03-19 17:06:34 PDT
(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?
Comment 7 Trevor Saunders (:tbsaunde) 2012-03-19 17:18:40 PDT
(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 neil@parkwaycc.co.uk 2012-03-20 01:44:17 PDT
(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?
Comment 9 Trevor Saunders (:tbsaunde) 2012-03-20 01:59:29 PDT
(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 alexander :surkov 2012-03-20 02:41:05 PDT
(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 ;)
Comment 11 neil@parkwaycc.co.uk 2012-03-20 03:20:19 PDT
(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.
Comment 12 Trevor Saunders (:tbsaunde) 2012-03-20 06:01:13 PDT
ok, https://tbpl.mozilla.org/?tree=Try&rev=9479548e2514 should tell us what msvc thinks of just static casting to nsAccessible*
Comment 13 Trevor Saunders (:tbsaunde) 2012-03-20 09:21:56 PDT
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'
Comment 14 neil@parkwaycc.co.uk 2012-03-20 09:40:05 PDT
Is it not possible to add an include for nsAccessibleWrap.h?
Comment 15 Trevor Saunders (:tbsaunde) 2012-03-20 10:29:00 PDT
oh, lol at me, I changed that and pushed https://tbpl.mozilla.org/?tree=Try&rev=9db2d14bd0a4
Comment 16 Trevor Saunders (:tbsaunde) 2012-03-20 10:30:29 PDT
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?
Comment 17 neil@parkwaycc.co.uk 2012-03-20 10:39:10 PDT
Make your constructor private and your subclass a friend.
Comment 18 Trevor Saunders (:tbsaunde) 2012-03-20 14:42:13 PDT
(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 19 neil@parkwaycc.co.uk 2012-03-23 17:17:22 PDT
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...
Comment 20 Trevor Saunders (:tbsaunde) 2012-03-23 17:28:28 PDT
(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
Comment 21 Trevor Saunders (:tbsaunde) 2012-03-23 17:31:24 PDT
Created attachment 608929 [details] [diff] [review]
patch without renames
Comment 22 Trevor Saunders (:tbsaunde) 2012-03-23 17:42:59 PDT
Created attachment 608933 [details] [diff] [review]
no renames -U8 -p
Comment 23 neil@parkwaycc.co.uk 2012-03-24 06:28:50 PDT
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
Comment 24 Trevor Saunders (:tbsaunde) 2012-03-24 10:59:14 PDT
(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
Comment 25 Trevor Saunders (:tbsaunde) 2012-03-25 13:21:11 PDT
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 
 )
Comment 26 neil@parkwaycc.co.uk 2012-03-27 08:45:12 PDT
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.
Comment 27 neil@parkwaycc.co.uk 2012-03-27 09:11:18 PDT
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...
Comment 28 Trevor Saunders (:tbsaunde) 2012-03-27 09:49:45 PDT
(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?
Comment 29 Trevor Saunders (:tbsaunde) 2012-03-27 11:02:34 PDT
Comment on attachment 609743 [details] [diff] [review]
What you actually end up doing

I'll do a second part to rename everything.
Comment 30 Trevor Saunders (:tbsaunde) 2012-03-27 11:13:00 PDT
Created attachment 609796 [details] [diff] [review]
part 2
Comment 31 alexander :surkov 2012-03-28 01:06:12 PDT
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
Comment 32 alexander :surkov 2012-03-28 01:11:18 PDT
One thing: we should remove nsISupports::QueryInterface from CAccessibleAction
Comment 33 neil@parkwaycc.co.uk 2012-03-28 01:16:03 PDT
(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.)
Comment 34 Trevor Saunders (:tbsaunde) 2012-04-01 14:32:32 PDT
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

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