Last Comment Bug 671510 - make sure accessible tree is created for content processes
: make sure accessible tree is created for content processes
Status: RESOLVED FIXED
[e10s][inbound]
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: 646596
  Show dependency treegraph
 
Reported: 2011-07-14 02:16 PDT by alexander :surkov
Modified: 2011-07-21 06:23 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (7.91 KB, patch)
2011-07-14 02:16 PDT, alexander :surkov
felipc: feedback+
Details | Diff | Splinter Review
patch2 (6.18 KB, patch)
2011-07-19 20:08 PDT, alexander :surkov
bugs: superreview-
Details | Diff | Splinter Review
patch3 (6.40 KB, patch)
2011-07-20 07:15 PDT, alexander :surkov
cjones.bugs: review+
bugs: superreview+
Details | Diff | Splinter Review

Description alexander :surkov 2011-07-14 02:16:27 PDT
Created attachment 545863 [details] [diff] [review]
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) 2011-07-14 05:14:43 PDT
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] 2011-07-14 08:13:11 PDT
(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] 2011-07-14 08:25:56 PDT
>+static_cast<bool>(nsIPresShell::AccService())

Why not !!nsIPresShell::AccService()?
Comment 4 David Bolter [:davidb] 2011-07-14 08:46:38 PDT
(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) 2011-07-14 14:21:25 PDT
(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] 2011-07-14 14:33:15 PDT
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 2011-07-14 23:40:32 PDT
Comment on attachment 545863 [details] [diff] [review]
patch

Felipe, are you right person to review dom/ipc part?
Comment 8 alexander :surkov 2011-07-14 23:43:54 PDT
(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 2011-07-15 00:04:39 PDT
(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 me!) 2011-07-15 16:17:01 PDT
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 2011-07-19 20:08:27 PDT
Created attachment 546976 [details] [diff] [review]
patch2
Comment 12 alexander :surkov 2011-07-19 20:10:58 PDT
(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] 2011-07-20 03:36:11 PDT
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 2011-07-20 03:41:08 PDT
(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 2011-07-20 03:41:35 PDT
cc'ing Olli
Comment 16 Olli Pettay [:smaug] 2011-07-20 03:49:44 PDT
(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 2011-07-20 07:15:52 PDT
Created attachment 547078 [details] [diff] [review]
patch3
Comment 18 David Bolter [:davidb] 2011-07-20 08:12:52 PDT
Neato!
Comment 20 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-07-21 06:23:20 PDT
http://hg.mozilla.org/mozilla-central/rev/99d121a0f799

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