The default bug view has changed. See this FAQ.

make sure accessible tree is created for content processes

RESOLVED FIXED in mozilla8

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

({access})

unspecified
mozilla8
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [e10s][inbound])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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 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

6 years ago
(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

6 years ago
>+static_cast<bool>(nsIPresShell::AccService())

Why not !!nsIPresShell::AccService()?
(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.
(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

6 years ago
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.
(Assignee)

Comment 7

6 years ago
Comment on attachment 545863 [details] [diff] [review]
patch

Felipe, are you right person to review dom/ipc part?
Attachment #545863 - Flags: superreview?(Olli.Pettay)
Attachment #545863 - Flags: review?(felipc)
(Assignee)

Comment 8

6 years ago
(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.
(Assignee)

Updated

6 years ago
Whiteboard: [e10s]
(Assignee)

Comment 9

6 years ago
(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 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.
Attachment #545863 - Flags: review?(felipc) → feedback+
(Assignee)

Comment 11

6 years ago
Created attachment 546976 [details] [diff] [review]
patch2
Attachment #545863 - Attachment is obsolete: true
Attachment #546976 - Flags: superreview?(Olli.Pettay)
Attachment #546976 - Flags: review?(jones.chris.g)
Attachment #545863 - Flags: superreview?(Olli.Pettay)
(Assignee)

Comment 12

6 years ago
(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 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.
Attachment #546976 - Flags: superreview?(Olli.Pettay) → superreview-
(Assignee)

Comment 14

6 years ago
(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?
(Assignee)

Comment 15

6 years ago
cc'ing Olli
(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)
(Assignee)

Comment 17

6 years ago
Created attachment 547078 [details] [diff] [review]
patch3
Attachment #546976 - Attachment is obsolete: true
Attachment #547078 - Flags: superreview?(Olli.Pettay)
Attachment #547078 - Flags: review?(jones.chris.g)
Attachment #546976 - Flags: review?(jones.chris.g)
Attachment #547078 - Flags: superreview?(Olli.Pettay) → superreview+
Neato!
Attachment #547078 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 19

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/99d121a0f799
Whiteboard: [e10s] → [e10s][inbound]
http://hg.mozilla.org/mozilla-central/rev/99d121a0f799
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.