671510 - make sure accessible tree is created for content processes

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

If accessibility were instantiated in chrome process then child process should start accessibility when created.

Especially this make sense on Windows because accessibility is started when window handles WM_GETOBJECT message, the same time window is not created for content process until accessibility is started.

Who is right person to review the patch?

Comment 1

[Trevor Saunders (:tbsaunde)]

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

+    return ShowRemoteFrame(GetSubDocumentSize(frame),
>+                           static_cast<bool>(nsIPresShell::AccService())

since non null is true I'm not sure you need the static cast, but I don't think it hurts.

> bool
>-nsFrameLoader::ShowRemoteFrame(const nsIntSize& size)
>+nsFrameLoader::ShowRemoteFrame(const nsIntSize& size, bool isA11yEnabled)

on one hand it seems IsA11yRunning is moe descriptive of what the bool actually means, on the other we've always used isA11yEnabled before.

>+    // process.
>+    if (isA11yEnabled) {
>+      nsCOMPtr<nsIAccessibilityService> accService =
>+        do_GetService("@mozilla.org/accessibilityService;1");
>+    }

I don't think you need the braces, but it looks it might be the style of the file 

> {
>     // sigh
>-    unused << SendShow(size);
>+    unused << SendShow(size, isA11yEnabled);

wel, unused is a creapy way of getting around that warning, and I'm not convinced its even actually warned about here, but you didn't added so I'll not bother you about it.

otherwise looks good to me, but we should probably find someone else to make sure where we do this actually makes sense, but it  all looks reasonable to me based on the context in the patch.

Comment 2

[Josh Matthews [:jdm]]

(In reply to comment #1)
> > {
> >     // sigh
> >-    unused << SendShow(size);
> >+    unused << SendShow(size, isA11yEnabled);
> 
> wel, unused is a creapy way of getting around that warning, and I'm not
> convinced its even actually warned about here, but you didn't added so I'll
> not bother you about it.

IPDL parent process functions scream bloody murder if you don't use their return value. This is the only construct that both silences them and explicitly shows that the return value is not cared about.

Comment 3

[Josh Matthews [:jdm]]

>+static_cast<bool>(nsIPresShell::AccService())

Why not !!nsIPresShell::AccService()?

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #0)
> Who is right person to review the patch?

I'd try Chris Jones to start, and probably good to get Trevor's opinion as you go as well. Josh is good too, since I know where he lives. I'll be proactively looking at patches, but please r? or f? me on a11y mods.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to comment #2)
> (In reply to comment #1)
> > > {
> > >     // sigh
> > >-    unused << SendShow(size);
> > >+    unused << SendShow(size, isA11yEnabled);
> > 
> > wel, unused is a creapy way of getting around that warning, and I'm not
> > convinced its even actually warned about here, but you didn't added so I'll
> > not bother you about it.
> 
> IPDL parent process functions scream bloody murder if you don't use their
> return value. This is the only construct that both silences them and
> explicitly shows that the return value is not cared about.

the prototypes are marked __attribute__((warn_unused_result)) ?  would it make sense to get rid of that / not have the ipdl compiler generate it? I' haven't really looked at much of this code so I don't have an opinion.

It is more explicit than foo = bar(); foo = foo; which is the way I've seen of dealing with similar warnings, and I'm not saying its not the right answer, just that its not what one expects to read.

Comment 6

[Josh Matthews [:jdm]]

No, we definitely want the warning. It's frequently a mistake to simply ignore the fact that the function returns a success value, and the unused << idiom makes it clear that the value serves no purpose in this particular instance.

Comment 7

[alexander :surkov (:asurkov)]

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

Felipe, are you right person to review dom/ipc part?

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> (In reply to comment #0)
> > Who is right person to review the patch?
> 
> I'd try Chris Jones to start, and probably good to get Trevor's opinion as
> you go as well. Josh is good too, since I know where he lives. 

owners page is out of date, I failed to find who's responsible for dom/ipc part. Are Chris, Josh, Felipe peers or owners?

> I'll be
> proactively looking at patches, but please r? or f? me on a11y mods.

I don't see why would your f+ is needed but if you're feel hot to do reviews of a11y parts then I don't mind of course.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> Comment on attachment 545863 [details] [diff] [review] [review]
> patch
> 
> +    return ShowRemoteFrame(GetSubDocumentSize(frame),
> >+                           static_cast<bool>(nsIPresShell::AccService())
> 
> since non null is true I'm not sure you need the static cast, but I don't
> think it hurts.

I've changed to nsIPresShell::IsAccessibilityActive()

> >+nsFrameLoader::ShowRemoteFrame(const nsIntSize& size, bool isA11yEnabled)

> on one hand it seems IsA11yRunning is moe descriptive of what the bool
> actually means, on the other we've always used isA11yEnabled before.

if you prefer I don't mind

Comment 10

[:Felipe Gomes (needinfo for replies!)]

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

I'm not a peer, so I'm giving feedback instead. cjones or smaug should be the reviewers.

is it needed to add an extra parameter to ShowRemoteFrame? I think you should just retrieve nsIPresShell::AccService() from inside the function itself, so you don't need to do that for every call, just the the one that will call mRemoteBrowser->Show.

Also I wonder if this should be a new message (e.g. ->StartAccService) instead of a new parameter to Show. I don't have a strong opinion for either form in this case.

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #10)

> is it needed to add an extra parameter to ShowRemoteFrame?

oversight obviously, thank you!

> Also I wonder if this should be a new message (e.g. ->StartAccService)
> instead of a new parameter to Show. I don't have a strong opinion for either
> form in this case.

Ok, hoping to hear back from Olli or Chris about this.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 546976 [details] [diff] [review]
patch2

Why is this per tab, and not per process.
a11y service is anyway global inside a process.
So, I think you should add a new message to PContent, perhaps called
activateA11y().
Then when ParentContent is created check if a11y is activated and if so,
send activateA11y() message to child process.

Also, the same message should be send for dynamic a11y activation.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> Comment on attachment 546976 [details] [diff] [review] [review]
> patch2
> 
> Why is this per tab, and not per process.
> a11y service is anyway global inside a process.

didn't know these are different things (except maybe first tab).

> So, I think you should add a new message to PContent, perhaps called
> activateA11y().
> Then when ParentContent is created check if a11y is activated and if so,
> send activateA11y() message to child process.

Did you mean ContentParent class or something else? Is ContentParent::Init() proper place to call activateA11y()?

> Also, the same message should be send for dynamic a11y activation.

Do you mean the case when a11y is started after some tabs and processes are running, right? What's the right place to send notifications?

Comment 15

[alexander :surkov (:asurkov)]

cc'ing Olli

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #14)
> Did you mean ContentParent class or something else? Is ContentParent::Init()
> proper place to call activateA11y()?
Looks like a good place.

> Do you mean the case when a11y is started after some tabs and processes are
> running, right? What's the right place to send notifications?
I was assuming that when a11y is activated, it fires some notifications.
ParentContent could observer the notification.
(If a11y doesn't fire the notification, perhaps it should)

Comment 17

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

Neato!

Comment 19

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/99d121a0f799

Comment 20

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/99d121a0f799