862049 - setAccessibilityEnabled() isn't called on the UI thread in GeckoAccessibility

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

setAccessibilityEnabled() is called on the background thread here:
http://hg.mozilla.org/mozilla-central/file/4f364593f8bf/mobile/android/base/GeckoAccessibility.java#l72

That method calls setDynamicToolbarEnabled() which touches the UI.

Comment 1

[Chris Lord [:cwiiis]]

Think you could handle this? You know what the problem is and I'm pretty swamped dealing with various other regressions atm.

Comment 2

[Marco Zehe (:MarcoZ)]

I can assist with accessibility testing if needed.

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: Spacing cleanup in GeckoAccessibility

Sure.

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Run setAccessibilityEnabled() on the UI thread

Comment 5

[Chris Lord [:cwiiis]]

Comment on attachment 738315 [details] [diff] [review]
Spacing cleanup in GeckoAccessibility

Review of attachment 738315 [details] [diff] [review]:
-----------------------------------------------------------------

This seems like it ought to have been a separate bug, and I wonder if it would be a good idea to consult with whoever wrote the majority of it first before doing this? I also think the 8-space indentation for a line-break continuation is unusual, but if you're planning on attending to this file regularly, I'll leave that up to you.

r+, but I'd have preferred a separate bug for this, and a consultation with whoever wrote the code might be a good idea.

::: mobile/android/base/GeckoAccessibility.java
@@ +56,1 @@
>                          (AccessibilityManager) GeckoApp.mAppContext.getSystemService(Context.ACCESSIBILITY_SERVICE);

Shouldn't there be 4 fewer spaces here too?

@@ +59,1 @@
>                              (ActivityManager) GeckoApp.mAppContext.getSystemService(Context.ACTIVITY_SERVICE);

ditto

@@ +118,4 @@
>          accEvent.setPackageName(GeckoApp.mAppContext.getPackageName());
>          populateEventFromJSON(accEvent, message);
>          AccessibilityManager accessibilityManager =
> +                (AccessibilityManager) GeckoApp.mAppContext.getSystemService(Context.ACCESSIBILITY_SERVICE);

Oh ok, I guess you're going for 8-space indentations for line-breaks? I don't think we do this in many other files (certainly none of the ones I've been working on), but I'm not against it. Given that this file had 4-space for line-breaks before, I'd be inclined to keep it that way, but I'll leave it up to you.

Comment 6

[Chris Lord [:cwiiis]]

Comment on attachment 738316 [details] [diff] [review]
Run setAccessibilityEnabled() on the UI thread

Review of attachment 738316 [details] [diff] [review]:
-----------------------------------------------------------------

Neat, I wasn't aware of UiAsyncTask. LGTM.

Comment 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 738315 [details] [diff] [review]
Spacing cleanup in GeckoAccessibility

Review of attachment 738315 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Chris Lord [:cwiiis] from comment #5)
> Comment on attachment 738315 [details] [diff] [review]
> Spacing cleanup in GeckoAccessibility
> 
> Review of attachment 738315 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems like it ought to have been a separate bug, and I wonder if it
> would be a good idea to consult with whoever wrote the majority of it first
> before doing this? I also think the 8-space indentation for a line-break
> continuation is unusual, but if you're planning on attending to this file
> regularly, I'll leave that up to you.

From what I've seen, 8-space indentation for line wrapping is fairly common in Java code. The standard convention [1] is to either align the wrapped line to the expression or use 8-space wrapping, at your discretion. 8-space wrapping indentation is also the default setting for line wrapping in Eclipse, which perhaps another reason I'm used to it.

With that said, I'll just leave this alone -- I don't plan on touching this file much in the future.

[1] http://www.oracle.com/technetwork/java/javase/documentation/codeconventions-136091.html#248

Comment 8

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fdf667ca506

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1fdf667ca506