Closed Bug 1288199 Opened 3 years ago Closed 3 years ago

Modify ia2marshal.dll to expose additional type information

Categories

(Core :: Disability Access APIs, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: aklotz, Assigned: aklotz)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

In order to make IA2 interfaces compatible with the mscom com registration in bug 1286986 and mscom interceptor code from bug 1263224, we need to expose some additional information from ia2marshal.dll.

Firstly, we need to export the GetProxyDllInfo function in the def file.

Secondly, we need to expose the IA2 interfaces as a typelib:

1) Add the merged IA2 IDL file, ia2_api_all.idl. This is added verbatim from the ia2 website, sourced from http://accessibility.linuxfoundation.org/a11yspecs/ia2/ia2_api_all.idl

2) Generate a typelib from the merged IDL;

3) Embed that typelib in ia2marshal.dll's resources.
Note about generating the IA2 typelib:

By default, HWND marshaling is not compatible with the mscom interceptor because it contains a union (see the RemotableHandle structure declared in wtypes.idl in the Windows SDK).

Fortunately both fields in that union are the same type (LONG) but just different names. This gives us some flexibility at the binary level.

Instead of running midl directly on ia2_api_all.idl, I have written accessible/interfaces/ia2/IA2Typelib.idl. This generates the same typelib as ia2_api_all.idl would generate, but with the exception that is uses slightly different marshaling types for Windows UI handles. These types are wire-compatible with existing handle marshaling but also work with the mscom interceptor because they lack the union.
Trevor, please read comment 1 in the bug for some discussion around the idl files in this patch.

Review commit: https://reviewboard.mozilla.org/r/65628/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65628/
Attachment #8772959 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8772959 [details]
Bug 1288199: Add building of typelib to be embedded in ia2marshal.dll;

This all seems "reasonable", but I'm not really sure I'm enough of a make wizard to say that part is ok.
Attachment #8772959 - Flags: review?(tbsaunde+mozbugs)
Attachment #8772959 - Flags: review?(mshal)
Attachment #8772959 - Flags: review+
Attachment #8772959 - Flags: review?(mshal) → review+
Comment on attachment 8772959 [details]
Bug 1288199: Add building of typelib to be embedded in ia2marshal.dll;

https://reviewboard.mozilla.org/r/65628/#review63008

Looks reasonable to me, though I wish we had a better way to handle midl in moz.build. Just a few nits & comments to fix.

::: accessible/interfaces/ia2/Makefile.in:8
(Diff revision 1)
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  IA2DIR        = $(topsrcdir)/other-licenses/ia2
>  
> -GARBAGE       += $(MIDL_GENERATED_FILES)
> +GARBAGE       += $(MIDL_GENERATED_FILES) \
> +								 $(MIDL_UNUSED_GENERATED_FILES) \

nit: use spaces to indent here, not tabs.

::: accessible/interfaces/ia2/Makefile.in:45
(Diff revision 1)
>    AccessibleStates.idl \
>    IA2CommonTypes.idl \
>    $(NULL)
>  
> +MIDL_LIBRARIES = \
> +	IA2Typelib.idl \

nit: use spaces here too.

::: accessible/interfaces/ia2/Makefile.in:62
(Diff revision 1)
>    $(MIDL_INTERFACES:%.idl=%_i.c) \
>    $(MIDL_INTERFACES:%.idl=%.h) \
>    $(MIDL_ENUMS:%.idl=%.h) \
>    $(NULL)
>  
> +MIDL_UNUSED_GENERATED_FILES = \

Can you document what MIDL_UNUSED_GENERATED_FILES is intended for as a comment here? It's not immediately obvious without understanding how the .tlb file is generated.

::: accessible/interfaces/ia2/Makefile.in:63
(Diff revision 1)
>    $(MIDL_INTERFACES:%.idl=%.h) \
>    $(MIDL_ENUMS:%.idl=%.h) \
>    $(NULL)
>  
> +MIDL_UNUSED_GENERATED_FILES = \
> +	$(MIDL_LIBRARIES:%.idl=%_p.c) \

nit: spaces here too.

::: accessible/interfaces/ia2/Makefile.in:96
(Diff revision 1)
>  	for idl in $(sort $(subst FORCE,,$?) $(addsuffix .idl,$(addprefix $(IA2DIR)/,$(missing_base)))); do \
>  	  $(MIDL) $(MIDL_FLAGS) -app_config -I $(IA2DIR) -Oicf $$idl; \
>  	done
>  	touch $@
>  
> -# This marshall dll is also registered in the installer
> +typelib_done : $(MIDL_LIBRARIES)

Please also add a comment to this rule that the intent is to generate the .tlb file used by the .rc file.
Comment on attachment 8772959 [details]
Bug 1288199: Add building of typelib to be embedded in ia2marshal.dll;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65628/diff/1-2/
Attachment #8772959 - Flags: review?(tbsaunde+mozbugs)
Attachment #8772959 - Flags: review+
Comment on attachment 8772959 [details]
Bug 1288199: Add building of typelib to be embedded in ia2marshal.dll;

Carrying forward r+
Attachment #8772959 - Flags: review?(tbsaunde+mozbugs) → review+
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/73dff614932f
Add building of typelib to be embedded in ia2marshal.dll; r=mshal
https://hg.mozilla.org/mozilla-central/rev/73dff614932f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.