1076735 - Improve display of super DevTools mode request

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

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

The display for "the most" access is a bit confusing today:

ADB is root: Yes
DevTools restricted privileges: No

I think they should be reworded to both say "Yes" when you have the most access.  Also, maybe some red / green colors and links to MDN docs about what the privileges mean would be great to have.

Comment 1

[Gaurav Rai [:gaurav0x]]

I want to work on this bug. Tell me how to start?

Comment 2

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

Great!  Please take a look at our hacking guide[1] to get started.

Here's a list[2] of the files that will likely be involved in this change.

We should flip the meaning of "DevTools restricted privileges" to say something like "DevTools superuser privileges" with the opposite boolean value.

Comment 0 suggests some other improvements to make the display clearer, but you may have some other tweaks as well!

Note that you can test some of this via a Firefox OS Simulator, but getting some of the values may be tricky if you don't have a real Firefox OS device.

[1]: https://wiki.mozilla.org/DevTools/Hacking
[2]: http://dxr.mozilla.org/mozilla-central/search?q=path%3Aruntimedetails&case=true&=mozilla-central&redirect=true

Comment 3

[Gaurav Rai [:gaurav0x]]

can you elaborate you comment 0. what exactly you want to change?

Comment 4

[Jesal]

Hi I am also interested in this bug, is it possible to see in this bug in action? where I
can I recreate this?

Comment 5

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

Hmm, I guess the main thing I failed to clarify before is that these things are shown on WebIDE's[1] "Runtime Info" panel after connecting to a device.

If this is still unclear, it may be simpler to discuss it in the #devtools IRC room.

[1]: https://developer.mozilla.org/en-US/docs/Tools/WebIDE

Comment 6

[bilal]

I would like to work on this bug what do i need?

Comment 7

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

(In reply to bilal from comment #6)
> I would like to work on this bug what do i need?

I've added several general comments already, so you'll need to ask a more specific question.

Comment 8

[bilal]

This going to be my first bug so stuff like the code how to start what coding software would be best to use

Comment 9

[bilal]

Is*

Comment 10

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

(In reply to bilal from comment #8)
> This going to be my first bug so stuff like the code how to start what
> coding software would be best to use

Please take a look at the "Hacking" link I already posted in comment 2.

Comment 11

[bilal]

Cool

Comment 12

[bilal]

Cool

Comment 13

[Gene V]

Attachment: name.patch

hey @jryans, i saw that this hasn't gotten any attention in a while. i submitted an initial patch. it does not include the requested color changes or links to MDN docs.

Comment 14

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

Comment on attachment 8523473 [details] [diff] [review]
name.patch

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

Generally, this looks good, just a small text improvement.

It would be even better to include an MDN link though! :)

To do it, wrap the "Unrestricted DevTools privileges: " text in an <a> tag, but with no href.  It's a bit harder than usual to open the page, since we're outside of a normal browser tab.  But you'd bind a click handler to call |UI.openInBrowser|, like this[1].

[1]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/webide/content/addons.js#13-15

::: browser/devtools/webide/content/runtimedetails.js
@@ +116,4 @@
>          }
>        }, e => console.error(e));
>      } catch(e) {
> +      // Exception. pref actor is only accessible if forbid-certified-apps is false

Revert the change to this line

::: browser/locales/en-US/chrome/browser/devtools/webide.dtd
@@ +140,5 @@
>  <!ENTITY runtimedetails_title "Runtime Info">
>  <!ENTITY runtimedetails_adbIsRoot "ADB is root: ">
>  <!ENTITY runtimedetails_summonADBRoot "root device">
>  <!ENTITY runtimedetails_ADBRootWarning "(requires unlocked bootloader)">
> +<!ENTITY runtimedetails_superuserPrivileges "DevTools superuser privileges: ">

In the since the bug has been open, the WebIDE docs were updated[1] to call this "Unrestricted debugging", so I think we should avoid the term "superuser" and instead use something like "Unrestricted DevTools privileges: ".  Be sure to fix up the entity ID too.

[1]: https://developer.mozilla.org/en-US/docs/Tools/WebIDE#Unrestricted_app_debugging_%28including_certified_apps.2C_main_process.2C_etc.%29

Comment 15

[Gene V]

Attachment: bug1076735.patch

Uploaded new patch.
I changed the css to make the link appear like a link (same color as the "Close" link in the controls) - definitely open to suggestions there.

Comment 16

[Giovanny Gongora [:gioyik]]

Seems that he changed everything needed but forgets to set a reviewer flag. What you think Ryan? Maybe this needs a rebased version due to the time of this patch.

Comment 17

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

(In reply to Giovanny Andres Gongora Granada from comment #16)
> Seems that he changed everything needed but forgets to set a reviewer flag.
> What you think Ryan? Maybe this needs a rebased version due to the time of
> this patch.

Ah, good catch!  I'll add this to my review queue.

Comment 18

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

Comment on attachment 8529263 [details] [diff] [review]
bug1076735.patch

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

Gene, thanks for working on this!  Overall, it looks great to me.

I decided to clean up a few variable names, and since it took so long for me to see the patch, I thought I should help out by just doing that bit myself.

I'll attach an updated patch in a moment.

Comment 19

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

Attachment: 0001-Bug-1076735-Improves-display-of-super-DevTools-mode-.patch

Rebased version of Gene's attachment 8529263 [details] [diff] [review], also with few variable renames.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=49a8e581af91

Comment 20

[Gene V]

Oh whoops - didn't realize I had to set the review flag again. 

Thanks for cleaning up. Let me know if I need to do anything else here.

Comment 21

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

(In reply to Gene V from comment #20)
> Oh whoops - didn't realize I had to set the review flag again. 

It's best to make sure some kind of flag is set when you need someone else to take action, or else the bug updates get lost in a sea of bug mail. :)

> Thanks for cleaning up. Let me know if I need to do anything else here.

We should be good at this point!  Feel free to look around for other bugs to work on!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/9529933bb51c

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9529933bb51c