Improve display of super DevTools mode request

RESOLVED FIXED in Firefox 38

Status

()

Firefox
Developer Tools: WebIDE
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jryans, Assigned: Gene V, Mentored)

Tracking

unspecified
Firefox 38
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=html])

Attachments

(1 attachment, 2 obsolete attachments)

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.
Mentor: jryans@gmail.com
Whiteboard: [good first bug][lang=html]
I want to work on this bug. Tell me how to start?
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
can you elaborate you comment 0. what exactly you want to change?

Comment 4

3 years ago
Hi I am also interested in this bug, is it possible to see in this bug in action? where I
can I recreate this?
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

3 years ago
I would like to work on this bug what do i need?
(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

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

3 years ago
Is*
(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

3 years ago
Cool

Comment 12

3 years ago
Cool
(Assignee)

Comment 13

3 years ago
Created attachment 8523473 [details] [diff] [review]
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.
Attachment #8523473 - Flags: review?(jryans)
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
Attachment #8523473 - Flags: review?(jryans) → feedback+
(Assignee)

Comment 15

3 years ago
Created attachment 8529263 [details] [diff] [review]
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.
Attachment #8523473 - Attachment is obsolete: true
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.
Flags: needinfo?(jryans)
(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.
Flags: needinfo?(jryans)
Attachment #8529263 - Flags: review?(jryans)
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.
Attachment #8529263 - Flags: review?(jryans) → feedback+
Assignee: nobody → gene.vityugov
Status: NEW → ASSIGNED
Created attachment 8555387 [details] [diff] [review]
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
Attachment #8529263 - Attachment is obsolete: true
Attachment #8555387 - Flags: review+
Keywords: checkin-needed
(Assignee)

Comment 20

3 years ago
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.
(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!
https://hg.mozilla.org/integration/fx-team/rev/9529933bb51c
Keywords: checkin-needed
Whiteboard: [good first bug][lang=html] → [good first bug][lang=html][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9529933bb51c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=html][fixed-in-fx-team] → [good first bug][lang=html]
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.