795691 - b2g fixes for the web console actors

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Mihai Sucan [:msucan]]

After bug 768096 we have to do some fixes to allow the Web Console client to connect to a b2g server.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: demo patch

Really quick patch I did for the b2g+web console demo in London. This is a proof of concept.

Things to note:

- b2g browser actors have a bug: they instance global actors for tab actor grips. The Web Console client expects tab actors to live on tab grips.
- ProcessGlobal.js breaks the window.console API.
- the web console actor listeners need to allow all events, from any window. This is the beginning of the Global Console.

Comment 2

[Mihai Sucan [:msucan]]

Screenshot: http://img.i7m.de/show/iuuy5.png

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Mihai Sucan [:msucan] from comment #1)
> Created attachment 666306 [details] [diff] [review]
> demo patch
> 
> Really quick patch I did for the b2g+web console demo in London. This is a
> proof of concept.
> 
> Things to note:
> 
> - b2g browser actors have a bug: they instance global actors for tab actor
> grips. The Web Console client expects tab actors to live on tab grips.

You may want to split this work in multiple patches and land them piecemeal. This typo could be fixed right away.

> - ProcessGlobal.js breaks the window.console API.

Did you figure out why adding another observer breaks it?

> - the web console actor listeners need to allow all events, from any window.
> This is the beginning of the Global Console.

Which will be the successor to the Error Console, right? Awesome.

Do you plan to use the debugger's current (temporary!) approach to a UI (setting WCCP.local according to the menu item that launched the tool) or have you thought about something else? We should probably factor out the connection dialog into a shared utility at some point.

Comment 4

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #3)
> You may want to split this work in multiple patches and land them piecemeal.
> This typo could be fixed right away.

100% agreed. That's the plan.

> > - ProcessGlobal.js breaks the window.console API.
> 
> Did you figure out why adding another observer breaks it?

Demo time. Didn't try to figure out anything, just plowed ahead. :)

> > - the web console actor listeners need to allow all events, from any window.
> > This is the beginning of the Global Console.
> 
> Which will be the successor to the Error Console, right? Awesome.

Yes, but not here, not in this bug.

> Do you plan to use the debugger's current (temporary!) approach to a UI
> (setting WCCP.local according to the menu item that launched the tool) or
> have you thought about something else? We should probably factor out the
> connection dialog into a shared utility at some point.

Yes, for this bug I intend to do a minimal temporary UI for the connection stuff, similar to the debugger one. When we have a common UI we can use that.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 666306 [details] [diff] [review]
demo patch

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

::: b2g/components/ProcessGlobal.js
@@ +50,5 @@
>        let prefix = ('Content JS ' + message.level.toUpperCase() +
>                      ' at ' + message.filename + ':' + message.lineNumber +
>                      ' in ' + (message.functionName || 'anonymous') + ': ');
>        Services.console.logStringMessage(prefix + Array.join(message.arguments,
>                                                              ' '));

For my part I would be happy to delete ProcessGlobal.js if the remote console already display the console outputs. I wonder if some others have a different opinion.. Cc'ing Chris Jones and Fabrice.

Comment 6

[[:fabrice] Fabrice Desré]

I'm absolutely in favor of getting rid of ProcessGlobal.js also. The tools here are so much better!

Comment 7

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Changes:

- rebased on top of the latest fx-team.
- added minimal UI for connection (server, port and tabs list).
- added a Remote Web Console menuitem, hidden under a new pref: devtools.webconsole.remote-enabled.


Asking for reviews as follows:

- Vivien for b2g-related changes. Please let me know if further b2g-related changes are needed. I did not touch ProcessGlobal.js - it seems it works fine now - it looks like there were other problems in my code which caused console API messages to not show. I think you can remove ProcessGlobal.js later on when people start to use the web console more, or I can do it in my patch.

Please note I only tested with B2G Desktop builds on Linux. Please test under different scenarios and let me know if it still works.

- Tim: please review the browser/ changes.

- Panos: please review web console changes and the overall approach. Please let me know if I missed anything.

Questions:

Vivien: given this patch doesn't do too many changes to how the web console works - it only exposes functionality / makes it work with b2g, is this something you would like in Aurora? Are Gaia devs using aurora builds or simply landing this patch in m-c would be sufficient?

Tim: would this patch be landable in Aurora if the b2g/gaia team would like it?


Thank you!

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 670377 [details] [diff] [review]
proposed patch

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

This patch works fine for desktop to desktop debugging, but not for debugging a device. I tested with b2g on a device and I got an error:

error occurred while processing 'startListeners' request: TypeError: this.browser.addProgressListener is not a function

I believe it is from WebConsoleUtils.jsm:2654. We need to make these listeners fail safely, so that they can fail to start, but the rest of the console can continue to function.

Unrelated to this patch, but I just observed it: WCA_onStartListeners is missing a startedListeners.push(listener); for the LocationChange listener. As it is now, when we ask for all of the listeners, we get back that all but this one were started.

::: browser/app/profile/firefox.js
@@ +1100,5 @@
>  
> +// The Remote Web Console.
> +pref("devtools.webconsole.remote-enabled", false);
> +pref("devtools.webconsole.remote-host", "localhost");
> +pref("devtools.webconsole.remote-port", 6000);

I don't think you need different prefs for the web console. We expect to merge the connection dialogs for both the debugger and the console soon, and at that point a single configuration will exist. Furthermore, the remote-enabled pref serves also as a kill-switch for the debugger server, until we are ready to pref it on by default. It doesn't make sense to enable the remote web console feature separately from that, if it is going to connect to that same server.

::: browser/devtools/webconsole/HUDService.jsm
@@ +1080,5 @@
> +
> +    let result = Services.prompt.prompt(null,
> +      l10n.getStr("remoteWebConsolePromptTitle"),
> +      l10n.getStr("remoteWebConsolePromptMessage"),
> +      input, null, check);

I see you didn't add the "don't ask me again" checkbox, is that on purpose? It's not that big a deal either way.

Comment 9

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #8)
> Comment on attachment 670377 [details] [diff] [review]
> proposed patch
> 
> Review of attachment 670377 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch works fine for desktop to desktop debugging, but not for
> debugging a device. I tested with b2g on a device and I got an error:
> 
> error occurred while processing 'startListeners' request: TypeError:
> this.browser.addProgressListener is not a function
> 
> I believe it is from WebConsoleUtils.jsm:2654. We need to make these
> listeners fail safely, so that they can fail to start, but the rest of the
> console can continue to function.

Known issue with the tab actor in b2g - bug 798764. The global consoleActor should work.


> Unrelated to this patch, but I just observed it: WCA_onStartListeners is
> missing a startedListeners.push(listener); for the LocationChange listener.
> As it is now, when we ask for all of the listeners, we get back that all but
> this one were started.

My bad - will fix.

> ::: browser/app/profile/firefox.js
> @@ +1100,5 @@
> >  
> > +// The Remote Web Console.
> > +pref("devtools.webconsole.remote-enabled", false);
> > +pref("devtools.webconsole.remote-host", "localhost");
> > +pref("devtools.webconsole.remote-port", 6000);
> 
> I don't think you need different prefs for the web console. We expect to
> merge the connection dialogs for both the debugger and the console soon, and
> at that point a single configuration will exist. Furthermore, the
> remote-enabled pref serves also as a kill-switch for the debugger server,
> until we are ready to pref it on by default. It doesn't make sense to enable
> the remote web console feature separately from that, if it is going to
> connect to that same server.

Makes sense. I'll use the debugger prefs.


> ::: browser/devtools/webconsole/HUDService.jsm
> @@ +1080,5 @@
> > +
> > +    let result = Services.prompt.prompt(null,
> > +      l10n.getStr("remoteWebConsolePromptTitle"),
> > +      l10n.getStr("remoteWebConsolePromptMessage"),
> > +      input, null, check);
> 
> I see you didn't add the "don't ask me again" checkbox, is that on purpose?
> It's not that big a deal either way.

Yes, that's on purpose. I also don't add the timeout event. I'd like to avoid us having too much code duplication. I'd rather spend time fixing bug 800442.


Thanks for your quick review!

Comment 10

[Mihai Sucan [:msucan]]

Attachment: patch (v0.2)

Updated to address Panos's comments. Changed to use the debugger prefs.

For the error you encountered I will have to fix bug 798764.

If you want more UI smarts implemented, please let me know. Thank you!

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 670483 [details] [diff] [review]
patch (v0.2)

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

(In reply to Mihai Sucan [:msucan] from comment #7)
> Created attachment 670377 [details] [diff] [review]
> proposed patch
> 
> Changes:
> 
> Asking for reviews as follows:
> 
> - Vivien for b2g-related changes. Please let me know if further b2g-related
> changes are needed. I did not touch ProcessGlobal.js - it seems it works
> fine now - it looks like there were other problems in my code which caused
> console API messages to not show. I think you can remove ProcessGlobal.js
> later on when people start to use the web console more, or I can do it in my
> patch.

With the current b2g/ changes I feel like Panos review is sufficient. This is code he wrote :)

If ProcessGlobal.js works now I'm fine to remove it in a different bug.

> 
> Questions:
> 
> Vivien: given this patch doesn't do too many changes to how the web console
> works - it only exposes functionality / makes it work with b2g, is this
> something you would like in Aurora? Are Gaia devs using aurora builds or
> simply landing this patch in m-c would be sufficient?

I feel like having such a tool in the runtime that will be shipped will be a great help for web authors. That would make their life much easier and this can be part of the daily tool they used to build web apps. 
Now I don't know about the Firefox impact of those changes and I will conform to Tim's answer.
 
> Tim: would this patch be landable in Aurora if the b2g/gaia team would like
> it?
> 
> 
> Thank you!

I think I can't express how happy I am to see the devtools evolve in such a way. So thank you Panos and Mihai for all the hard work you're doing here with the remote debugger and the console!

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 670483 [details] [diff] [review]
patch (v0.2)

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

Sorry, that it took me so long. Forgot a little about this :/ LGTM!

Comment 13

[Rob Campbell [:rc] (:robcee)]

landable?

Comment 14

[Mihai Sucan [:msucan]]

Landed:
https://hg.mozilla.org/integration/fx-team/rev/0743f910f5a5

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0743f910f5a5

Comment 16

[Mihai Sucan [:msucan]]

Comment on attachment 670483 [details] [diff] [review]
patch (v0.2)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A.
User impact if declined: comment 11 sums up this really well: having this patch in Firefox 18 would match the base of the runtime we will ship (b2g). Web devs will be able to connect from Firefox for desktop to their Firefox OS device/builds, using the Web Console.
Testing completed (on m-c, etc.): green try runs, landed in fx-team and in m-c.
Risk to taking this patch (and alternatives if risky): minimal risk. This patch only adds minimal UI that allows the remote connection.
String or UUID changes made by this patch: add news strings in browser.dtd and webconsole.properties.

Thank you!

Comment 17

[bhavana bajaj [:bajaj]]

(In reply to Mihai Sucan [:msucan] from comment #16)
> Comment on attachment 670483 [details] [diff] [review]
> patch (v0.2)
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): N/A.
> User impact if declined: comment 11 sums up this really well: having this
> patch in Firefox 18 would match the base of the runtime we will ship (b2g).
> Web devs will be able to connect from Firefox for desktop to their Firefox
> OS device/builds, using the Web Console.
> Testing completed (on m-c, etc.): green try runs, landed in fx-team and in
> m-c.
> Risk to taking this patch (and alternatives if risky): minimal risk. This
> patch only adds minimal UI that allows the remote connection.
> String or UUID changes made by this patch: add news strings in browser.dtd
> and webconsole.properties.
> 
> Thank you!

Since Aurora is a Freeze period for string changes, can you please email on the l10n list to make sure the localizers are ok with this change ?

Comment 18

[bhavana bajaj [:bajaj]]

Or can we prevent the string changes from being uplifted while maintaining the functionality?

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

I think one option would be to uplift only the two one-liner actor changes to aurora, so that B2G gets them, but we require developers to use Nightly for debugging B2G with the web console.

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Panos Astithas [:past] from comment #19)
> I think one option would be to uplift only the two one-liner actor changes
> to aurora, so that B2G gets them, but we require developers to use Nightly
> for debugging B2G with the web console.

What I meant to write is that we require developers to use Nightly *desktop* as their debugging tool.

Comment 21

[Mihai Sucan [:msucan]]

Panos's idea is good - I like that. Would that be acceptable?

Another idea:

- I could reuse the debugger strings where possible.
- Where I don't have reusable strings: no localization in aurora. Let them be English.

Why I'd prefer to do this:

- Aurora is in strings freeze and people from #l10n are not happy at all if we change strings.
  - So we don't do that.
- I really want to offer mobile devs the UI. Makes things much easier.
  - We've talked about devtools not necessarily needing l10n since they are technical and most of the devs out there use these tools in English anyway. (there are far fewer cases where non-English dev tools are used).
- This stuff is behind a pref.
- I hope people will not be upset if a couple of strings show in English, only when a very new feature is enabled in about:config.
- This will be only one cycle.

Thoughts? Would my idea be acceptable? Let's pick one approach and I'll do a patch for aurora.

Comment 22

[Alex Keybl [:akeybl]]

Comment on attachment 670483 [details] [diff] [review]
patch (v0.2)

Comment 19 seems like the way to go, given this is a non-critical string change on Aurora otherwise.

Comment 23

[Mihai Sucan [:msucan]]

Attachment: patch for aurora

This is the patch with no UI changes.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A.
User impact if declined: users will not be able to connect to b2g using the remote web console.
Testing completed (on m-c, etc.): these changes have landed in m-c for quite some time.
Risk to taking this patch (and alternatives if risky): minimal to none.
String or UUID changes made by this patch: none.

Thanks!

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1e93f23070ab

Comment 25

[Axel Hecht [:Pike]]

Ryan, you didn't land the patch for aurora (without strings), but instead the patch with string changes on aurora.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Whoops, sorry about that!

Backed out:
https://hg.mozilla.org/releases/mozilla-aurora/rev/59f2d425581b

Re-landed:
https://hg.mozilla.org/releases/mozilla-aurora/rev/02bf38dae079

Comment 27

[Mihai Sucan [:msucan]]

Thank you Axel and Ryan!