Add a button to enable certified app debugging

VERIFIED FIXED in Firefox 34

Status

DevTools
WebIDE
VERIFIED FIXED
4 years ago
a month ago

People

(Reporter: paul, Assigned: paul)

Tracking

({dev-doc-needed})

Trunk
Firefox 34
x86_64
All
dev-doc-needed
Dependency tree / graph

Firefox Tracking Flags

(firefox34 verified)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

4 years ago
Depends on: 1040754
(Assignee)

Comment 1

4 years ago
Created attachment 8458683 [details] [diff] [review]
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.
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #8458683 - Flags: feedback?(jryans)
(Assignee)

Comment 2

4 years ago
... and add a message if it's not a USB runtime.
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.
Attachment #8458683 - Flags: feedback?(jryans) → feedback+
(Assignee)

Updated

4 years ago
Depends on: 1041513
(Assignee)

Comment 4

4 years ago
Created attachment 8459619 [details] [diff] [review]
v2
Attachment #8458683 - Attachment is obsolete: true
Attachment #8459619 - Flags: review?(jryans)
(Assignee)

Comment 5

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

Comment 6

4 years ago
Created attachment 8459628 [details] [diff] [review]
v2.1
Attachment #8459619 - Attachment is obsolete: true
Attachment #8459619 - Flags: review?(jryans)
Attachment #8459628 - Flags: review?(jryans)
Currently unable to build fx-team.  I'll check this out tomorrow when I can hopefully apply the patch.
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".
Attachment #8459628 - Flags: review?(jryans) → review+
(Assignee)

Comment 9

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

Comment 10

4 years ago
Created attachment 8460350 [details] [diff] [review]
v2.2
Attachment #8459628 - Attachment is obsolete: true
Attachment #8460350 - Flags: review+
(Assignee)

Comment 11

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=4c04c3d582c8
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/2acbfbbc8c6f
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2acbfbbc8c6f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
(Assignee)

Comment 15

4 years ago
(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
QA Whiteboard: [qa+]
status-firefox34: --- → fixed
Duplicate of this bug: 928137
I could debug certified apps from a Flame device with FirefoxOS 1.4 using latest Aurora 34.0a2 after selecting "Request higher privileges".
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
status-firefox34: fixed → verified

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.