[remote-dbg-next] Add control to enable addon debugging in new about:debugging

VERIFIED FIXED in Firefox 67

Status

enhancement
P1
normal
VERIFIED FIXED
7 months ago
2 months ago

People

(Reporter: jdescottes, Assigned: daisuke)

Tracking

(Blocks 1 bug)

unspecified
Firefox 67
Dependency tree / graph

Firefox Tracking Flags

(firefox67 verified, firefox68 verified)

Details

(Whiteboard: old-remote-debugging-ng-m3)

Attachments

(5 attachments, 1 obsolete attachment)

Reporter

Description

7 months ago
about:debugging gap

With the current about:debugging, if you go to about:debugging#addons, there is a checkbox that allows to enable/disable addons debugging:

  [ ] Enable add-on debugging Learn more

Behind the scenes, this sets a couple of preferences as detailed on https://developer.mozilla.org/en-US/docs/Tools/about:debugging#Enabling_add-on_debugging

This feature is missing from the new about:debugging and should be implemented.
Reporter

Comment 1

6 months ago
filter on remote-debugging-next-move-m3-to-m2
Reporter

Comment 2

6 months ago
filter on remote-debugging-next-move-m3-to-m2
Reporter

Comment 3

6 months ago
filter on remote-debugging-next-move-m3-to-m2
No longer blocks: remote-debugging-ng-m3
Priority: -- → P3
Whiteboard: old-remote-debugging-ng-m3
Reporter

Comment 4

4 months ago

This is definitely P2!!!

Priority: P3 → P2
Reporter

Comment 5

4 months ago

The Inspect buttons should also be disabled when this preference is not set.

Reporter

Updated

4 months ago
Blocks: 1500385
Reporter

Comment 6

4 months ago

If tests are added to cover this, we can close Bug 1500385 as Duplicate.

Reporter

Updated

4 months ago
Duplicate of this bug: 1500385
Assignee

Comment 8

4 months ago

This checkbox is valid for addons on remote runtimes such as Android as well?
As far as I investigate, we can debug any addons on remote even devtools.chrome.enabled and devtools.debugger.remote-enabled config on remote are off.
If the configs are valid for remote runtime as well, it seems we should implement also a mechanism which accepts/rejects the debugging on remote runtime by the configs, I think.

Flags: needinfo?(jdescottes)
Reporter

Comment 9

4 months ago

Ah that's interesting.

devtools.chrome.enabled

It looks like devtools.chrome.enabled is mostly checked on the client side. The only server side usage of this preference is:

https://searchfox.org/mozilla-central/rev/c07aaf12f13037c1f5a343d31f8291549e57373f/devtools/server/actors/storage.js#2014

So for the runtime (=server) turning on this pref will only impact the storage inspector.

For local addon debugging, it is checked on the client side to open the browser toolbox, which is also what we use for local addons. But it looks like we can skip it for remote runtimes.

devtools.debugger.remote-enabled

I have not tested, but I assume that devtools.debugger.remote-enabled is always true when we are debugging a remote device? The purpose of the pref is to enable remote debugging, so it would make sense? https://searchfox.org/mozilla-central/search?q=devtools.debugger.remote-enabled&case=true&regexp=false&path=mobile

This control is only needed because we use the browser toolbox at the moment to debug addons. We should show it only for This-Firefox. For the remote runtimes, the chrome enabled pref is client side only so we don't care here. And if you are connected to the remote runtime, then it means devtools.debugger.remote-enabled is already true. And no need to introduce another way to enable/disable addon debugging in remote runtimes.

This information will be useful to find the good wording we should use for this checkbox.

Flags: needinfo?(jdescottes)
Assignee

Comment 10

4 months ago

Thanks Julian!
Ah, however, we can debug any targets on Android even devtools.debugger.remote-enabled is off...
(But if set devtools.remote.usb.enabled to off, rejects the debugging and closes current connection immediately..)
Okay, I'll file a bug to discuss what should do on remote runtime by devtools.debugger.remote-enabled.

Also, I'll take this bug, and implement only This-Firefox at this time.
Thanks again.

Assignee: nobody → dakatsuka
Status: NEW → ASSIGNED
Priority: P2 → P1
Assignee

Comment 15

4 months ago

Hi Matt, I need your advice!

As Julian said in D18196 at phabricator, we will use a checkbox here.
I have referred from checkboxes of photon. Although it defines the style of checkbox which assumes font-size: 13px, there are no definition for other font-size. Since the font-size of the "Load Temporary Add-on" button which will be at side of this checkbox is 15px, I thought it is better to fit to the font-size though, how should we design for the checkbox and the label?

Flags: needinfo?(mcroud)

Matt, the "learn more" link should look like a link, or like a Photon micro button? Thanks!

Comment 17

4 months ago

@Daisuke
Could you provide an image or screenshot of what it looks like? If I can see the page as a whole with the checkbox in place I can hopefully come up with some suggestions :)

@Belén
It depends where 'Learn more' will be placed. Generally if it is in the main body a link is correct, if this is in a message bar then Photon only indicates that a button should be used. I have not seen a text link within a message bar in use.

Assignee

Comment 18

4 months ago
Posted image prototype.png

Hi Matt, thank you for replying!
I attach the screenshot. If you need additional something, please let me know!

Comment 19

4 months ago

Thank you for the image!
The "Load Temporary Add-on" button will eventually follow the Photon Default button styling: https://design.firefox.com/photon/components/buttons.html#default-2 making the button text 13px. This might make it easier to have the checkbox label match the docs and also be 13px.

My advice would be to have the checkbox label 13px and see how it looks after we change the button to match the docs.

It might be that after we add more content to that page, it might look odd being the only 13px sentence, but we can make that decision once more styling is in place.

Flags: needinfo?(mcroud)
Reporter

Updated

4 months ago
Blocks: 1500381

Comment 20

4 months ago

Just adding the feedback I received from Emanuela on #ux regarding checkbox label size.
Checkbox labels are NOT limited to Photon Body 10 (13px) type, you can use larger text (Body 20 for example) however the checbox size does not change.

Reporter

Updated

3 months ago
Duplicate of this bug: 1500381
Attachment #9040374 - Attachment is obsolete: true

Comment 23

3 months ago
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a1f91d90de1
Add a checkbox to enable addon debugging on this-firefox. r=jdescottes,ladybenko
https://hg.mozilla.org/integration/autoland/rev/b10932876150
Re-layout for runtime controllers. r=ladybenko,jdescottes
https://hg.mozilla.org/integration/autoland/rev/ef94872bfd12
Disable 'Inspect' button for extension when extension debug setting was disabled. r=jdescottes,ladybenko
https://hg.mozilla.org/integration/autoland/rev/dd4aa59c6a12
Add a test for extension debug setting. r=jdescottes,ladybenko

Comment 25

2 months ago

Verified as fixed on Firefox Nightly 68.0a1 (2019-04-01) and Firefox 67.0b6 on Windows 10 x 64, Mac OS X 10.14 and on Ubuntu 16.04 x64.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.