[Moonlight-list] [PATCH] MoonAtkBridge initialization
"Andrés G. Aragoneses"
knocte at gmail.com
Wed Jul 29 23:09:22 EDT 2009
Version 4 of the patch.
Any other comments?
Thanks,
Andres
Andrés G. Aragoneses wrote:
> Hey Sebastien,
>
> Sebastien Pouliot wrote:
>> Hello Andrés,
>>
>> On Wed, 2009-06-17 at 23:17 -0400, "Andrés G. Aragoneses" wrote:
>>> Andrés G. Aragoneses wrote:
>>>> Hey, thanks for your prompt answer.
>>>>
>>>> Sebastien Pouliot wrote:
>>>>> All,
>>>>>
>>>>> A bit of context: this patch (should) add nothing to Moonlight unless
>>>>> you compile with an MOON_A11Y_INTERNAL_HACK environment variable.
>>>>>
>>>>> This hack will be removed (i.e. activate a11y) only when all security
>>>>> concerns* are resolved (including the one in the comments) but, in the
>>>>> mean time, it allows the a11y team to build ML+A11Y support. Which in
>>>>> turns let them run and debug their own code under the coreclr
>>>>> restrictions.
>>>>>
>>>>> * please state them so they can be either answered (right now) or
>>>>> considered (before the hack is removed and a11y becomes available).
>>>>>
>>>>>
>>>>> Andrés: do you have a wiki page that resume our past discussions ? that
>>>>> would provide answers to many questions.
>>>> Good point, I'll create one.
>>>>
>>> Created:
>>>
>>> http://mono-project.com/Accessibility:_Moonlight_Bridge
>>>
>>> Let me know if I'm missing any details please.
>> Thanks.
>>
>>>>> more comments below...
>>>>>
>>>>> On Wed, 2009-06-17 at 14:59 -0400, "Andrés G. Aragoneses" wrote:
>>>>>> Hi,
>>>>>>
>>>>>> In a similar way in which we hooked a11y initialization on MWF, I'm
>>>>>> proposing a patch we have developed for moon. It may still not be
>>>>>> complete, but we will be fixing the rest of the cases along the way (for
>>>>>> example, the case in which moon is not installed with an xpi).
>>>>> I think this is the time to think about it, not later, since this risk
>>>>> to invalidate (or replace) the current approach.
>>>> Current approach is this scenario:
>>>> Moon XPI + Moon-a11y XPI.
>>>>
>>>> Next scenarios to consider are:
>>>> 1) Moon-native + Moon-a11y XPI
>>>> 2) Moon-native + Moon-a11y native
>>>> 3) Moon XPI + Moon-a11y native?
>>>>
>>>> It seems I'll need to research how to ask firefox about possible
>>>> locations of extensions, but I would say the refactoring needed for this
>>>> would not "invalidate" but just "upgrade" the current approach.
>> You can see this as an upgrade feature-wise. However review-wise you're
>> invalidating the work (and time) of the reviewers for something that you
>> already know is not complete.
>>
>> Do *not* consider the loading code as reviewed (i.e. correct for future
>> releases purpose). We'll discuss that once all options are covered.
>
> Ok, of course I'll ask for review as well when the new code for
> all-options is done.
>
>
>>>>>> Is it ok to commit? Please review.
>> See other comments prior to commit.
>>
>>>>>> Index: src/security.c
>>>>>> ===================================================================
>>>>>> --- src/security.c (revision 136249)
>>>>>> +++ src/security.c (working copy)
>>>>>> @@ -16,6 +16,7 @@
>>>>>> #if MONO_ENABLE_CORECLR_SECURITY
>>>>>>
>>>>>> static struct stat platform_stat;
>>>>>> +static struct stat platform_a11y_stat;
>>>>>>
>>>>>> const static char* platform_code_assemblies [] = {
>>>>>> "mscorlib.dll",
>>>>>> @@ -40,6 +41,8 @@
>>>>>> struct stat info;
>>>>>> gchar *dir, *name;
>>>>>> unsigned int i;
>>>>>> + struct stat the_platform_stat = platform_stat;
>>>>>> + gboolean a11y = FALSE;
>>>>>>
>>>>>> if (!image_name)
>>>>>> return FALSE;
>>>>>> @@ -51,20 +54,35 @@
>>>>>> return FALSE;
>>>>>> }
>>>>>>
>>>>>> + name = g_path_get_basename (image_name);
>>>>>> + if (!name) {
>>>>>> + g_free (dir);
>>>>>> + return FALSE;
>>>>>> + }
>>>>>> +
>>>>>> +#if MOON_A11Y_INTERNAL_HACK_ENABLED
>>>>>> + if (g_ascii_strcasecmp (name, "MoonAtkBridge.dll") == 0) {
>>>>>> + the_platform_stat = platform_a11y_stat;
>>>>>> + a11y = TRUE;
>>>>>> + }
>>>>>> +#endif
>>>>>> +
>>>>>> /* we avoid comparing strings, e.g. /opt/mono/lib/moon versus /opt/mono//lib/moon */
>>>>>> - if ((platform_stat.st_mode != info.st_mode) ||
>>>>>> - (platform_stat.st_ino != info.st_ino) ||
>>>>>> - (platform_stat.st_dev != info.st_dev)) {
>>>>>> + if ((the_platform_stat.st_mode != info.st_mode) ||
>>>>>> + (the_platform_stat.st_ino != info.st_ino) ||
>>>>>> + (the_platform_stat.st_dev != info.st_dev)) {
>>>>>> g_free (dir);
>>>>>> + g_free (name);
>>>>>> return FALSE;
>>>>>> }
>>>>>> g_free (dir);
>>>>>>
>>>>>> + if (a11y == TRUE){
>>>>>> + g_free (name);
>>>>>> + return TRUE;
>>>>>> + }
>>>>>> +
>>>>>> /* we know the names of every platform assembly, because we ship them */
>>>>>> - name = g_path_get_basename (image_name);
>>>>>> - if (!name)
>>>>>> - return FALSE;
>>>>>> -
>>>>>> for (i = 0; i < G_N_ELEMENTS (platform_code_assemblies); i++) {
>>>>>> if (g_ascii_strcasecmp (name, platform_code_assemblies [i]) == 0) {
>>>>>> g_free (name);
>>>>>> @@ -88,7 +106,25 @@
>>>>>> "you're doing!");
>>>>>> } else if (g_path_is_absolute (platform_dir)) {
>>>>>> memset (&platform_stat, 0, sizeof (platform_stat));
>>>>>> +
>>>>>> if (stat (platform_dir, &platform_stat) == 0) {
>>>>>> +
>>>>>> + const char* moonlight_at_novell = g_strrstr (platform_dir, "moonlight at novell.com");
>>>>>> + if (moonlight_at_novell != NULL) {
>>>>>> + const char* after = g_strdup ("moonlight-a11y at novell.com/components");
>>>>>> +
>>>>>> + const char* before = g_strndup (platform_dir,
>>>>>> + strlen(platform_dir) - strlen(moonlight_at_novell));
>>>>>> + const char* platform_a11y_dir = g_strconcat (before, after, NULL);
>>>>>> +
>>>>>> + memset (&platform_a11y_stat, 0, sizeof (platform_a11y_stat));
>>>>>> + stat (platform_a11y_dir, &platform_a11y_stat);
>>>>>> + g_free (platform_a11y_dir);
>>>>>> + g_free (before);
>>>>>> + g_free (after);
>>>>>> + moonlight_at_novell = NULL;
>>>>>> + }
>>>>>> +
>> Please move this block into its own function surrounded by the
>> #if MOON_A11Y_INTERNAL_HACK_ENABLED
>
> Ok.
>
>
>>>>>> mono_security_enable_core_clr ();
>>>>>> mono_security_set_core_clr_platform_callback (determine_platform_image);
>>>>>> }
>> ...
>>
>>>>>> + public static IntPtr GetAccessible ()
>>>>> I lack context but is this method likely to be called often ? If is the
>>>>> return value expected to change over the plugin life time ?
>>>> It will be called, by the plugin. But that's a second patch we'll
>>>> propose after this one:
>>>>
>>>> http://anonsvn.mono-project.com/viewvc/trunk/uia2atk/MoonAtkBridge/patches/moonlight-a11y-slot-tree.diff?view=markup
>>>>
>>> So, the function will be called by Firefox when an AT (a11y client, like
>>> a screen reader) requests any a11y information from the plugin. The
>>> return value can change over the lifetime of the plugin (because more
>>> than 1 moonlight app can live at the same time),
>> Well that looks like one handle per deployment. Anyway the question was
>> "likely to be called often" and is unanswered ;-)
>
> I would say it's not called very often because AFAIK it cannot change
> unless it's disposed.
>
>
>>> but AFAIK if we're
>>> talking about the same app, it won't change, so were you thinking that
>>> we should cache it in a field?
>> Yes, if it is to be called often then you should cache the handle (e.g.
>> in the deployment) and return it, instead of doing reflection each time.
>
> Makes sense, will do, even if it's not called often.
>
>
>>> It will die when the moonlight app is
>>> disposed.
>> Where's that code btw ?
>
> It's not done yet, I was expecting to commit it after this.
>
> Thanks,
>
> Andrés
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: moonlight-a11y-initialization-v4.diff
Type: text/x-patch
Size: 12353 bytes
Desc: not available
Url : http://lists.ximian.com/pipermail/moonlight-list/attachments/20090729/c9f20fae/attachment.bin
More information about the Moonlight-list
mailing list