1040779 - Add a button to enable certified app debugging

Status: VERIFIED FIXED

(DevTools Graveyard :: WebIDE, defect)

Comment 1

[Paul Rouget [:paul]]

Attachment: v1

What do you think?

We might want to check first if we are root or not. This might fail if adb runs as a user (shell), not root. But we can request a switch to root (adb root), but I don't know how to do that via adb client (see http://androidxref.com/4.0.4/xref/system/core/adb/SERVICES.TXT)

Also - the UI could be better.

Comment 2

[Paul Rouget [:paul]]

... and add a message if it's not a USB runtime.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8458683 [details] [diff] [review]
v1

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

This is a decent first pass.  It's nice to help the user like this when we can.

About ADB root, it looks like the command to send is just "root:"[1], so that could be added to the addon.

However, I am guessing it's unlikely to succeed based on system properties[2] of production builds?  Not sure about this.  "adb shell getprop" can be used to examine these values.

[1]: http://androidxref.com/4.0.4/xref/system/core/adb/services.c#478
[2]: http://androidxref.com/4.0.4/xref/system/core/adb/services.c#110

::: browser/devtools/webide/content/runtimedetails.js
@@ +59,5 @@
>    }
>  }
> +
> +function EnableCertifiedApps() {
> +  if (AppManager.selectedRuntime instanceof USBRuntime) {

If it's not USB, you could show another message like "To enable certified app debugging, you need to connect via USB first." or something.

::: browser/devtools/webide/locales/en-US/webide.properties
@@ +43,5 @@
>  addons_status_installing=installing
> +
> +certifiedapps_msg_restart=Restarting B2G
> +certifiedapps_msg_error=Error: %1$S
> +certifiedapps_msg_noshell=No shell access. Update ADB Addon Helper

Maybe mention it should be >= 0.3.0?  Also, end with a period.

Comment 4

[Paul Rouget [:paul]]

Attachment: v2

Comment 5

[Paul Rouget [:paul]]

(there's an issue with non-rooted devices, working on it)

Comment 6

[Paul Rouget [:paul]]

Attachment: v2.1

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Currently unable to build fx-team.  I'll check this out tomorrow when I can hopefully apply the patch.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8459628 [details] [diff] [review]
v2.1

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

Looks good overall.  Tested the privileges switch, and it works well.

Would be even nicer with a button to go back down to restricted, but I won't hold you up for that here.

::: browser/devtools/webide/content/runtimedetails.js
@@ +101,5 @@
> +      } else {
> +        adbCheckResult.textContent = sUnknown;
> +      }
> +    } else {
> +      adbCheckResult.textContent = "not a USB device";

Shouldn't this be in the properties file too?

::: browser/devtools/webide/content/runtimedetails.xhtml
@@ +34,5 @@
> +      </p>
> +      <p id="certified-check">
> +        &runtimedetails_restrictedPrivileges;<span class="yesno"></span>
> +        <div class="action">
> +          <button>&runtimedetails_requestPrivileges;</button>

Seems like it would easy to have a button to lock the device down again as well.  Would be helpful for our own testing to easily set the device back to a more production-like state.  For that, you could use the pref actor as well.

::: browser/devtools/webide/locales/en-US/webide.dtd
@@ +123,5 @@
>  <!-- Runtime Details -->
>  <!ENTITY runtimedetails_title "Runtime Info">
> +<!ENTITY runtimedetails_adbIsRoot "ADB is root: ">
> +<!ENTITY runtimedetails_summonADBRoot "root device">
> +<!ENTITY runtimedetails_ADBRootWarning "(require unlocked bootloader)">

s/require/requires/

@@ +126,5 @@
> +<!ENTITY runtimedetails_summonADBRoot "root device">
> +<!ENTITY runtimedetails_ADBRootWarning "(require unlocked bootloader)">
> +<!ENTITY runtimedetails_restrictedPrivileges "DevTools restricted privileges: ">
> +<!ENTITY runtimedetails_requestPrivileges "request higher privileges">
> +<!ENTITY runtimedetails_privilegesWarning "(Will reboot device. Requires root access)">

Add another period after "access".

Comment 9

[Paul Rouget [:paul]]

(In reply to J. Ryan Stinnett [:jryans] from comment #8)
> Seems like it would easy to have a button to lock the device down again as
> well.  Would be helpful for our own testing to easily set the device back to
> a more production-like state.  For that, you could use the pref actor as
> well.

Right. Follow-up: bug 1042160

Comment 10

[Paul Rouget [:paul]]

Attachment: v2.2

Comment 11

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=4c04c3d582c8

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/2acbfbbc8c6f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2acbfbbc8c6f

Comment 14

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

This page should probably be updated: https://developer.mozilla.org/en-US/Firefox_OS/Using_the_App_Manager#Debugging_Certified_Apps_2

Comment 15

[Paul Rouget [:paul]]

(In reply to Liz Henry :lizzard from comment #14)
> This page should probably be updated:
> https://developer.mozilla.org/en-US/Firefox_OS/
> Using_the_App_Manager#Debugging_Certified_Apps_2

This is an old URL. New URL: https://developer.mozilla.org/en-US/docs/Tools/WebIDE#Debugging_certified_apps

Comment 17

[Petruta Horea [:phorea]]

I could debug certified apps from a Flame device with FirefoxOS 1.4 using latest Aurora 34.0a2 after selecting "Request higher privileges".