888533 - Add minimum for remote debugging UI v1

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Chenxia Liu [:liuche]]

The minimum for landing remote debugging should be:
- UI for toggling devtools.debugger.remote-enabled
- either make restartless possible, or add UI for restarting

Comment 1

[Chenxia Liu [:liuche]]

Update: after adding some debug printouts, I realized that this has actually been completely working! - except that the dialog for accepting the remote debug connection is not being displayed above the preferences activity, which gave me the impression that a restart was necessary.

Comment 2

[Chenxia Liu [:liuche]]

Dialogs are tied to activities, not applications, so since we need a dialog to display over whichever Fennec Activity is currently running (because you may start remote debugging anywhere in the application), I'm going to take the approach of making a translucent dialog activity that displays a dialog. Unfortunately, this is kind of hacky.

The other alternative is to use a FragmentDialog + FragmentManager with transactions, but this relies on everything being fragments, which we currently don't use in pre-v11 Android.

Comment 3

[Chenxia Liu [:liuche]]

Attachment: Part 1: Strings v1

Lucas, since you did some of the remote debugging stuff, pushing this to you for review - feel free to re-gift it though.

Comment 4

[Chenxia Liu [:liuche]]

Attachment: Part 2: Advanced prefs xml layouts v1

Comment 5

[Chenxia Liu [:liuche]]

Attachment: Part 3: Surface browser activity + dialog v1

Comment 6

[Chenxia Liu [:liuche]]

Attachment: Part 4: Add "More information" link to remote-debugging

Comment 7

[Chenxia Liu [:liuche]]

Attachment: Part 4: Add link to MDN remote-debugging in Advanced settings v2

Changed string.

Comment 8

[Chenxia Liu [:liuche]]

Attachment: Screenshot: advanced screen - tablet

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 777182 [details] [diff] [review]
Part 1: Strings v1

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

Nice. I would probably merge this patch with the patch that adds code to use these strings.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +74,5 @@
>  <!ENTITY pref_category_vendor "&vendorShortName;">
>  <!ENTITY pref_category_datareporting "Data choices">
>  <!ENTITY pref_category_installed_search_engines "Installed search engines">
> +<!ENTITY pref_category_advanced "Advanced">
> +<!ENTITY pref_category_devtools "Developer tools">

"Developer tools" or "Developer Tools"? This is consistent with the existing strings. Just wondering if that's the intended form. I'm assuming this is as per ibarlow's spec.

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 777183 [details] [diff] [review]
Part 2: Advanced prefs xml layouts v1

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

Cool.

::: mobile/android/base/resources/xml-v11/preference_headers.xml
@@ +30,5 @@
>      <header android:fragment="org.mozilla.gecko.GeckoPreferenceFragment"
>              android:title="@string/pref_header_vendor">
>          <extra android:name="resource"
>                 android:value="preferences_vendor"/>
>      </header>

nit: add empty line here.

::: mobile/android/base/resources/xml/preferences_advanced.xml
@@ +10,5 @@
> +
> +   <PreferenceCategory android:title="@string/pref_category_devtools">
> +
> +       <CheckBoxPreference android:key="devtools.debugger.remote-enabled"
> +                           android:title="@string/pref_developer_remotedebugging" />

nit: add empty line here.

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 777212 [details] [diff] [review]
Part 4: Add link to MDN remote-debugging in Advanced settings v2

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

Nice.

::: mobile/android/base/resources/xml/preferences_advanced.xml
@@ +13,5 @@
>         <CheckBoxPreference android:key="devtools.debugger.remote-enabled"
>                             android:title="@string/pref_developer_remotedebugging" />
> +
> +       <org.mozilla.gecko.AlignRightLinkPreference android:title="@string/pref_developer_remotedebugging_docs"
> +                                                   url="https://developer.mozilla.org/docs/Tools/Debugger#Remote_debugging?" />

nit: add empty line here.
Does this URL need to be localized somehow?

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 777212 [details] [diff] [review]
Part 4: Add link to MDN remote-debugging in Advanced settings v2

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

Nice.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +76,5 @@
>  <!ENTITY pref_category_installed_search_engines "Installed search engines">
>  <!ENTITY pref_category_advanced "Advanced">
>  <!ENTITY pref_category_devtools "Developer tools">
>  <!ENTITY pref_developer_remotedebugging "Remote debugging">
> +<!ENTITY pref_developer_remotedebugging_docs "Getting started">

I wonder if "Getting started" is the right wording here. It gives the impression that this will take you to a wizard/tour type of thing. I'd probably use something more explicit like "Learn more" or "Read more about it". Maybe double-check with ibarlow?

::: mobile/android/base/resources/xml/preferences_advanced.xml
@@ +13,5 @@
>         <CheckBoxPreference android:key="devtools.debugger.remote-enabled"
>                             android:title="@string/pref_developer_remotedebugging" />
> +
> +       <org.mozilla.gecko.AlignRightLinkPreference android:title="@string/pref_developer_remotedebugging_docs"
> +                                                   url="https://developer.mozilla.org/docs/Tools/Debugger#Remote_debugging?" />

nit: add empty line here.
Does this URL need to be localized somehow?

Comment 13

[Lucas Rocha (:lucasr)]

(In reply to Chenxia Liu [:liuche] from comment #2)
> Dialogs are tied to activities, not applications, so since we need a dialog
> to display over whichever Fennec Activity is currently running (because you
> may start remote debugging anywhere in the application), I'm going to take
> the approach of making a translucent dialog activity that displays a dialog.
> Unfortunately, this is kind of hacky.
> 
> The other alternative is to use a FragmentDialog + FragmentManager with
> transactions, but this relies on everything being fragments, which we
> currently don't use in pre-v11 Android.

We use the Fragment API from Android's support library, which is available pre-v11. I'd prefer using a FragmentDialog if possible.

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 777184 [details] [diff] [review]
Part 3: Surface browser activity + dialog v1

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

If the idea is to show a dialog even if Fennec is not visible on screen, Using a DialogFragment is probably not an option here.

This looks good but I'd like to get a better understanding of the implemented behaviour before giving r+.

::: mobile/android/base/BrowserApp.java
@@ +519,5 @@
> +    @Override
> +    public void onPause() {
> +        super.onPause();
> +        // Register for Prompt:Show so we can foreground this activity if it's hidden.
> +        registerEventListener("Prompt:Show");

I'm assuming this is needed in order to make the prompt dialog visible in case remote debugging is triggered while fennec is in background?

@@ +1140,5 @@
> +                // Bring this activity to front so the prompt is visible..
> +                Intent bringToFrontIntent = new Intent();
> +                bringToFrontIntent.setClassName(AppConstants.ANDROID_PACKAGE_NAME, AppConstants.BROWSER_INTENT_CLASS);
> +                bringToFrontIntent.setFlags(Intent.FLAG_ACTIVITY_REORDER_TO_FRONT);
> +                startActivity(bringToFrontIntent);

How does this look when Prompt:Show is emitted while Fennec is in background (not visible)? You only see the dialog without Fennec behind it?

Comment 15

[Lucas Rocha (:lucasr)]

Ian, do you approve the screenshot? Just wondering if "Getting started" is the best wording here.

Comment 16

[Chenxia Liu [:liuche]]

(In reply to Lucas Rocha (:lucasr)
> Comment on attachment 777182 [details] [diff] [review]
> Part 1: Strings v1
> 
> Review of attachment 777182 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice. I would probably merge this patch with the patch that adds code to use
> these strings.

No problem, I think I will actually merge all of this into one patch to land, but I split it up for ease of reviewing.

> ::: mobile/android/base/locales/en-US/android_strings.dtd
> "Developer tools" or "Developer Tools"? This is consistent with the existing
> strings. Just wondering if that's the intended form. I'm assuming this is as
> per ibarlow's spec.

I believe Android convention is to capitalize the first word and not subsequent words, correct me if I'm wrong Ian - happy to fix it.

> Does this URL need to be localized somehow?

I checked in #l10n, and removing the /en-US/ from the url causes MDN to do localization redirects server-side. (No guarantees that the doc actually exists in other languages, but that can't be helped.) No additional localization necessary.

> I wonder if "Getting started" is the right wording here. It gives the impression 
> that this will take you to a wizard/tour type of thing. I'd probably use 
> something more explicit like "Learn more" or "Read more about it". Maybe 
> double-check with ibarlow?

I like the idea of "Learn more" - I stuck that in as a placeholder, since I couldn't think up a good string at the time.

Addressing the following two points together:

> We use the Fragment API from Android's support library, which is available 
> pre-v11. I'd prefer using a FragmentDialog if possible.

(and re: Prompt:Show listener in BrowserApp.java)
> I'm assuming this is needed in order to make the prompt dialog visible in case 
> remote debugging is triggered while fennec is in background?

I went with a solution that's simpler than having a Fragment/TransactionManager, where .App just brings itself to the front if it's in the back stack. From Intent.FLAG_ACTIVITY_REORDER_TO_FRONT:

> If set in an Intent passed to Context.startActivity(), this flag will cause the 
> launched activity to be brought to the front of its task's history stack if it is 
> already running.

That solves the problem of a remote-debugging request appearing when you have Settings opened (e.g., after enabling remote debugging) where you can't see it, by bringing BrowserApp to the front, where the dialogs are shown.

The main cases I addressed are:

1) Fennec isn't even running - I assume the request won't even be registered, so nothing happens.

2) Fennec is hidden but running - remote debugging request will show up in Fennec when Fennec is resumed. (Dialogs are associated with an activity, and can't be popped up independently.)

3) Settings is open on top of the browser, as in the case when you've just enabled remote-debugging from the UI - remote debugging request dialog will be launched in .App activity and the .App activity will be brought to the front. Settings won't be present in the stack anymore.

Thanks for the review! And don't worry, my feelings won't be hurt if you prefer using r- :) I'll upload new patches for review shortly.

Comment 17

[Chenxia Liu [:liuche]]

Try build w/ tests: https://tbpl.mozilla.org/?tree=Try&rev=8d37cf48709f

(Grab a build and try it out with a remote-enabled desktop FF.)

Comment 18

[Chenxia Liu [:liuche]]

Attachment: Part 2: Advanced prefs xml layouts v2

Newline nits addressed.

Comment 19

[Chenxia Liu [:liuche]]

Attachment: Part 4: Add docs link to remote debugging v2

Changed string to "Learn more".

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 778166 [details] [diff] [review]
Part 2: Advanced prefs xml layouts v2

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

Nice.

Comment 21

[Ian Barlow (:ibarlow)]

Lucas and I were just chatting and he raised an interesting point that gives me second thoughts about using "Advanced" as a top level heading. 

Partly because Developer Tools are the only thing we have in that menu for now, and partly because we are hiding the developer tools in a page where developers may or may not look. On desktop, at least, we show Web Developer options in-line with other less advanced features in the Tools menu. 

I wonder if we should just bubble them up to be a top level Setting for now. So it would be:

Sync
Customize
Display
Privacy
Mozilla
Developer

(or 'Developer tools')

What do you folks think?

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Ian Barlow (:ibarlow) from comment #21)
> Lucas and I were just chatting and he raised an interesting point that gives
> me second thoughts about using "Advanced" as a top level heading. 

> Developer (or 'Developer tools')
> 
> What do you folks think?

Android itself uses "Developer options". Desktop Firefox does refer to these as "tools", so I'd be fine with "Developer tools".

Comment 23

[Chenxia Liu [:liuche]]

Attachment: Part 1: Strings v2

Change "Advanced" => "Developer tools"

Comment 24

[Chenxia Liu [:liuche]]

Attachment: Part 2: Advanced prefs xml layouts v3

Incorporate new strings.

Comment 25

[Chenxia Liu [:liuche]]

Attachment: Screenshot: devtools (tablet)

Comment 26

[Chenxia Liu [:liuche]]

Attachment: Screenshot: menu (phone)

Comment 27

[Chenxia Liu [:liuche]]

Attachment: Screenshot: devtools (phone)

Comment 28

[Chenxia Liu [:liuche]]

Ian, how do these screenshots look?

Comment 29

[Chenxia Liu [:liuche]]

Comment on attachment 777184 [details] [diff] [review]
Part 3: Surface browser activity + dialog v1

Did my earlier commments/try build help clarify what the behavior is?

Comment 30

[Ian Barlow (:ibarlow)]

(In reply to Chenxia Liu [:liuche] from comment #28)
> Ian, how do these screenshots look?

Looks great, ship it :)

Comment 31

[Lucas Rocha (:lucasr)]

Comment on attachment 777184 [details] [diff] [review]
Part 3: Surface browser activity + dialog v1

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

Looks generally good, with the described intent in mind. However, I feel like the implemented behaviour (bring Fennec to top) should only apply to the remote debugging prompts, not for any type of prompt. It's only ok to do that for remote debugging because this is an action explicitly triggered by the user (by requesting a remote debug connection from desktop). The same doesn't apply to all other types of prompts which are usually triggered by the pages.

::: mobile/android/base/BrowserApp.java
@@ +512,5 @@
>  
> +    @Override
> +    public void onResume() {
> +        super.onResume();
> +        unregisterEventListener("Prompt:Show");

Doesn't this break somehow if you haven't registered a listener for "Prompt:Show" before?

Comment 32

[Chenxia Liu [:liuche]]

Attachment: Part 3: Bring to front browser for dialog v4

> Looks generally good, with the described intent in mind. However, I feel
> like the implemented behaviour (bring Fennec to top) should only apply to
> the remote debugging prompts, not for any type of prompt. It's only ok to do
> that for remote debugging because this is an action explicitly triggered by
> the user (by requesting a remote debug connection from desktop). The same
> doesn't apply to all other types of prompts which are usually triggered by
> the pages.
> 

I talked to Wes about this, and he mentored through adding a Prompt:ShowTop message for high priority dialogs and also modifying Prompt to handle priority. There is a little ugliness making the prompt synchronous, but I'd be happy to file a follow-up for that, or might just file a devtools bug to accept an async callback (because that makes a lot more sense).

> Doesn't this break somehow if you haven't registered a listener for
> "Prompt:Show" before?

Gecko logs a warning that there aren't any listeners to register, but nothing breaks. I think it's cleaner than always checking for the listener first.

Comment 33

[Chenxia Liu [:liuche]]

Attachment: Part 5: toast reminder to set up port forwarding v1

I ended up spending a lot of getting bitten by this, so I just added a toast.

Comment 34

[Lucas Rocha (:lucasr)]

Comment on attachment 783699 [details] [diff] [review]
Part 3: Bring to front browser for dialog v4

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

Looks good (with suggested changes).

::: mobile/android/chrome/content/browser.js
@@ +6936,4 @@
>     *
>     * @return true if the connection should be permitted, false otherwise
>     */
> +  _showPrompt: function rd_showPrompt() {

_showConnectionPrompt for clarity?

@@ +6948,5 @@
> +      window: null,
> +      title: title,
> +      message: msg,
> +      buttons: [ agree, cancel, disable ],
> +      priority: "1"

Why not using a plain int here?

@@ +6954,5 @@
> +
> +    // The debugger server expects a synchronous response, so spin on result since Prompt is async.
> +    let result = null;
> +
> +    console.log("showing prompt");

Still necessary?

@@ +6961,5 @@
> +    });
> +
> +    // Spin this thread while we wait for a result.
> +    let thread = Services.tm.currentThread;
> +    while (result == null)

I assume that tapping outside the prompt dialog does not dismiss it without a defined result?

@@ +6973,3 @@
>      }
>      return false;
> +  }.bind(this),

I think calling bind when passing _showPrompt() to init() would look cleaner and more visible.

@@ +6981,5 @@
>  
>    _start: function rd_start() {
>      try {
>        if (!DebuggerServer.initialized) {
> +        DebuggerServer.init(this._showPrompt);

this._showPrompt.bind(this) instead.

::: mobile/android/modules/Prompt.jsm
@@ +15,5 @@
>  }
>  
>  function Prompt(aOptions) {
>    this.window = "window" in aOptions ? aOptions.window : null;
>    this.msg = { type: "Prompt:Show", async: true };

nit: add empty line here.

@@ +16,5 @@
>  
>  function Prompt(aOptions) {
>    this.window = "window" in aOptions ? aOptions.window : null;
>    this.msg = { type: "Prompt:Show", async: true };
> +  if ("priority" in aOptions && aOptions.priority != null) {

The combination of if and switch here seems a bit overdone given that you only really care about the priority = 1 case. This would look simpler to read:

this.msg = { async: true };
if (aOptions.priority === 1)
    msg.type = "Prompt:ShowTop";
else
    msg.type = "Prompt:Show";

@@ +19,5 @@
>    this.msg = { type: "Prompt:Show", async: true };
> +  if ("priority" in aOptions && aOptions.priority != null) {
> +    switch (aOptions.priority) {
> +      case "1":
> +        // Top priority, sent a Prompt:ShowTop message.

sent -> send

@@ -108,5 @@
>    },
>  
>    show: function(callback) {
>      this.callback = callback;
> -    log("Sending message");

Unrelated?

@@ -118,5 @@
>      this.bridge.handleGeckoMessage(JSON.stringify(this.msg));
>    },
>  
>    observe: function(aSubject, aTopic, aData) {
> -    log("observe " + aData);

Ditto.

Comment 35

[Lucas Rocha (:lucasr)]

Comment on attachment 783701 [details] [diff] [review]
Part 5: toast reminder to set up port forwarding v1

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

What's the background for this patch? Just wondering where it's coming from.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +342,5 @@
>  <!ENTITY webapp_generic_name "App">
>  
>  <!ENTITY searchable_description "Bookmarks and history">
>  
> +<!ENTITY devtools_remote_debugging_forward "Don\'t forget to set up port forwarding!">

Sounds a bit aggressive :-) But I can't think of a better way to phrase it...

Comment 36

[Chenxia Liu [:liuche]]

> > +
> > +    // Spin this thread while we wait for a result.
> > +    let thread = Services.tm.currentThread;
> > +    while (result == null)
> 
> I assume that tapping outside the prompt dialog does not dismiss it without
> a defined result?
> 

Yep, dismiss from outside returns -1.

> >    show: function(callback) {
> >      this.callback = callback;
> > -    log("Sending message");
> 
> Unrelated?
> 
> @@ -118,5 @@
> >      this.bridge.handleGeckoMessage(JSON.stringify(this.msg));
> >    },
> >  
> >    observe: function(aSubject, aTopic, aData) {
> > -    log("observe " + aData);
> 
> Ditto.

Yep, both of these are previously existing statements. I'll remove the second one since it's not very useful, but up to you if I remove the first one, or make it more specific.

> 
> What's the background for this patch? Just wondering where it's coming from.
> 
> ::: mobile/android/base/locales/en-US/android_strings.dtd
> @@ +342,5 @@
> >  <!ENTITY webapp_generic_name "App">
> >  
> >  <!ENTITY searchable_description "Bookmarks and history">
> >  
> > +<!ENTITY devtools_remote_debugging_forward "Don\'t forget to set up port forwarding!">
> 

Setting up remote debugging is really easy now, but I oftentimes forget to start adb forward, because it's something separate from the two Firefox instances I'm linking. It's possible that this isn't actually the case, because you'll probably just turn on remote-debugging once, but mostly only devs will see it, and I think I'd appreciate the reminder. And I'd rather have the Toast and the strings in rather than wish I had it but get blocked by Aurora string freeze.

> Sounds a bit aggressive :-) But I can't think of a better way to phrase it...

It actually seems a lot friendlier when it's coming in Toast form from Android, but maybe that's just me.

Comment 37

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Developer port forwarding toast

(See second half of comment #37 for context)

Comment 38

[Ian Barlow (:ibarlow)]

(In reply to Chenxia Liu [:liuche] from comment #36)

> > Sounds a bit aggressive :-) But I can't think of a better way to phrase it...
> 
> It actually seems a lot friendlier when it's coming in Toast form from
> Android, but maybe that's just me.

I tend to agree, it seems more like a friendly reminder than something aggressive. I like it

Comment 39

[Chenxia Liu [:liuche]]

Attachment: Part 3: Bring to front browser for dialog v5

Addressed comments and nits.

Comment 40

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7d29572f9df
https://hg.mozilla.org/integration/mozilla-inbound/rev/14fccad8c36c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed419e36e05
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c5c81f1ebfe

Landed on inbound in 4 parts because we may want to remove toasts at some point, and there is a heisenbug in part 2, where synchronous loop takes a little while to return the result to the DebuggerServer. This includes all behavior and strings specified by v1.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ed419e36e05
https://hg.mozilla.org/mozilla-central/rev/0c5c81f1ebfe

Comment 42

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

https://hg.mozilla.org/mozilla-central/rev/d7d29572f9df
https://hg.mozilla.org/mozilla-central/rev/14fccad8c36c

Comment 43

[Ed Morley [:emorley]]

Please can bugs changing these parts of the tree land on fx-team in the future to help balance load & avoid conflicts (for more info see Gavin's newsgroup post: https://mail.mozilla.org/pipermail/firefox-dev/2013-July/000618.html). Thank you :-)