850296 - Connect our console module to the devtool console

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Alexandre Poirot [:ochameau]]

Currently our console API goes to stdout and into the old fashion JS console.
It has many drawbacks and we would gain from using the upcoming global browser console that would allow easy discovery of console messages, advanced filtering support and be able to inspect ojects instead of printing "[Object object]".

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 850

WIP, needs to be tuned to included filename and line number.

Comment 2

[Jeff Griffiths (:canuckistani) (:⚡︎)]

This is a P1 bug, it needs a new owner.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 1019

Here is patch that depends on bug 879228.
It allows to delegate most of the console/stdout work to devtool code.
Console.jsm handles printing nicely to stdout (i.e. prevent printing just [Object object]), and also pipe console calls to the Browser console.

Unfortunately, we hit bug 861338. console calls made during addon startup only show up in stdout and not in the browser console. It won't be a big issues for developers using cfx, but it may be one for others on addon builder.

This patch would require a bunch of work to modify unittests that were checking console output as we not longer can out the very final stdout ouput that is delegate to devtool code. But we no longer need to check such details as browser console code has unit tests.

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 757916 [details]
Pull request 1019

I think this is right direction to be going as long as CFX (which will attempt to read from file) and add-on builder will be able to handle all of this. Also I remember Jeff wanted to log everywhere stdout, old console, new console. I presume that's what devtools console is doing if not we should probably do it.

I also think instead of wrapping created instance of devtools console and if there are some reasons why we can't we should probably fix that in devtools.

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 757916 [details]
Pull request 1019

I moved more logic to Console.jsm to avoid having to wrap the console.
I've updated the unit test to match the new, quite different, stdout output.
I still have once issue to solve, but I think it is already worth starting reviewing it. The new console put commas between each console.log arguments.
So that: 
  console.log(1, 2)
is going to output:
  console.log: addon-id: 1, 2

It ends up messing with unit test output, where we use multiple console.info arguments and do not expect commas between them...

Comment 7

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #5)
...
> I think this is right direction to be going as long as CFX (which will
> attempt to read from file) and add-on builder will be able to handle all of
> this. Also I remember Jeff wanted to log everywhere stdout, old console, new
> console. I presume that's what devtools console is doing if not we should
> probably do it.

The browser console is replacing the error console in 24 anyway ( the key binding is switched over ). My expectation as a developer would be:

1. I see console.log ending up in the browser console with an add-on installed normally and the right loglevel set.

2. if I use cfx run, I additionally see stdout

3. for old school compatibility, print text into the error console, sure. I expect once developers see the browser console, they will prefer it.

We've fixed the issue in Addon Builder helper to set the loglevel? One additional change we could make in addon builder helper is to launch the browser console instead of the error console from the 'Error Console' toolbar button, i assume this is done via the helper add-on:

https://www.evernote.com/shard/s1/sh/c7242174-cc5a-4cec-8890-1b1bb9893a2e/f584e9799824dc71b8810cc0975ace91

Comment 8

[Alexandre Poirot [:ochameau]]

We the current patches, the two first items are expected to work.
The 3rd isn't, we no longer pipe messages to the old console service, but I don't think that's a real issue. We should communicate about this, but the simple fact that the key binding open the browser console makes the old console almost removed.

And yes, we will have to spawn another builder helper version to open the Browser console. i.e. replace this code:
https://github.com/mozilla/addon-builder-helper/blob/master/lib/addons-builder-helper.js#L19-L55
With something like:
let {gDevtoolsBrowser} = Cu.import("resource:///modules/devtools/gDevTools.jsm");
gDevtoolsBrowser.toggleBrowserToolboxCommand();

Comment 9

[Alexandre Poirot [:ochameau]]

Jeff, I'm wondering if we should make this work depend on bug 861338, and block on it before landing this patch? 

To make it short: messages being sent before Browser console is opened are ignored and will only print to stdout.

That's a pretty significant behavior change compared to the old console. Having said that it won't change much for developers using cfx and used to look at the stdout. But it can be more disturbing for developers used to look at the Error console -or- addon builder users. Also, we won't be able to use the Browser console to debug addons during Firefox startup, as we can't open it on startup (bug 860672).

Comment 10

[Jeff Griffiths (:canuckistani) (:⚡︎)]

While I see that bug 861338 & bug 860672 are important for a good user experience, I don't see why this should block on that. This bug isn't a meta bug, it's just a bug to get our source connected to their destination.

Comment 11

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Getting read of needinfo here

Comment 12

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 757916 [details]
Pull request 1019

Few things that I'm not sure this handles properly that I'd like to check before r+ this:

1. This change does not breaks add-on builder.
2. Since it depends on Console.jsm and more importantly on changes you made to
   it I presume it may not work for our integration tests that run against release
   and aurora. If we should find some solution, maybe bring up on weekly or do some
   workaround.
3. I don't see any changes to CFX, which is surprising since I was under impression
   it reads from file that we used to write to and I don't really see how
   Console.jsm would guess which file to write output to.
4. Are there any implications on Fennec ?

Comment 13

[Alexandre Poirot [:ochameau]]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #12)
> Comment on attachment 757916 [details]
> Pull request 1019
> 
> Few things that I'm not sure this handles properly that I'd like to check
> before r+ this:
> 
> 1. This change does not breaks add-on builder.

It does as described in comment 8. We need to release a new helper version.

> 2. Since it depends on Console.jsm and more importantly on changes you made
> to
>    it I presume it may not work for our integration tests that run against
> release
>    and aurora. If we should find some solution, maybe bring up on weekly or
> do some
>    workaround.

There is no possible workaround here. We should stop running test of sdk master against anything else other than mozilla-central.

> 3. I don't see any changes to CFX, which is surprising since I was under
> impression
>    it reads from file that we used to write to and I don't really see how
>    Console.jsm would guess which file to write output to.

Hum, I think I broke this logFile feature over here:
https://github.com/mozilla/addon-sdk/pull/1019/files#L2L579
But that change isn't really justified. We can keep passing `print`,
which ends up being require(system).stdout.write.
And I modified Console.jsm in order to be able to use a custom print method,
so that we will continue writing to this logFile.

But it works without it? I don't remember exactly the usefullness of this logFile thing. I think it was breaking stdout on windows, but it may have been made unecessary since I landed my windows platform fixes around stdout...

> 4. Are there any implications on Fennec ?

Good question!

Comment 14

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

(In reply to Alexandre Poirot (:ochameau) from comment #13)
> There is no possible workaround here. We should stop running test of sdk
> master against anything else other than mozilla-central.
We can hide the non-central tests easily enough. Then we'd just need to file a bug to actually stop running those tests to free up build/test machine time.

Comment 15

[Dave Townsend [:mossop]]

(In reply to Wes Kocher (:KWierso) from comment #14)
> (In reply to Alexandre Poirot (:ochameau) from comment #13)
> > There is no possible workaround here. We should stop running test of sdk
> > master against anything else other than mozilla-central.
> We can hide the non-central tests easily enough. Then we'd just need to file
> a bug to actually stop running those tests to free up build/test machine
> time.

Let's do that, I don't think we have a need for them anymore.

Comment 16

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

(In reply to Dave Townsend (:Mossop) from comment #15)
> Let's do that, I don't think we have a need for them anymore.

Non-m-c builders are hidden, and I filed bug 880506 about shutting down the now-hidden tests.

Comment 17

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Alexandre Poirot (:ochameau) from comment #13)
> (In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from
> comment #12)
> > Comment on attachment 757916 [details]
> > Pull request 1019
> > 
> > Few things that I'm not sure this handles properly that I'd like to check
> > before r+ this:
> > 
> > 1. This change does not breaks add-on builder.
> 
> It does as described in comment 8. We need to release a new helper version.


Do you plan on providing patch for that as well ?


> 
> > 2. Since it depends on Console.jsm and more importantly on changes you made
> > to
> >    it I presume it may not work for our integration tests that run against
> > release
> >    and aurora. If we should find some solution, maybe bring up on weekly or
> > do some
> >    workaround.
> 
> There is no possible workaround here. We should stop running test of sdk
> master against anything else other than mozilla-central.

I think it's already a case now, so we can probably ignore this one.

> 
> > 3. I don't see any changes to CFX, which is surprising since I was under
> > impression
> >    it reads from file that we used to write to and I don't really see how
> >    Console.jsm would guess which file to write output to.
> 
> Hum, I think I broke this logFile feature over here:
> https://github.com/mozilla/addon-sdk/pull/1019/files#L2L579
> But that change isn't really justified. We can keep passing `print`,
> which ends up being require(system).stdout.write.
> And I modified Console.jsm in order to be able to use a custom print method,
> so that we will continue writing to this logFile.
> 
> But it works without it? I don't remember exactly the usefullness of this
> logFile thing. I think it was breaking stdout on windows, but it may have
> been made unecessary since I landed my windows platform fixes around
> stdout...


As far as I know only reason for that was to workaround stdout issue on windows, if tests pass and progress is logged on windows as it used to, I don't really care for that feature, in fact I'd rather prefer to remove that feature.

> 
> > 4. Are there any implications on Fennec ?
> 
> Good question!



Alex do you need some help to run this to a completion, I'd like this change to land. Thanks!

Comment 18

[Alexandre Poirot [:ochameau]]

Any help would be much welcomed!

The current status is that I need to address some final comments in the platform patch. This time the API exposed by the platform shouldn't change.
So we can already take a look at what is going on on Fennec, but it requires to build it with the patch applied.

I should be able to spend the necessary cycle next week to address platform patch, push it to try and eventually land it.
In the meantime if you can release the browser console modification for the helper and/or test these patches against fennec, it would help landing the jetpack patch sooner.

Comment 19

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 757916 [details]
Pull request 1019

Clearing out review request until there's patch to review.

Comment 20

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Jordan, do you think you can help out with fennec related parts since you're looking into fennec tests already ?

Comment 21

[Jordan Santell [:jsantell] [@jsantell]]

8 tests fail with this change (37 of 45 pass):
tests/test-system-events.test emit to nsIObserverService observers: failure
tests/test-system-events.test handle nsIObserverService notifications: failure

Comment 22

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: PR 1106

Comment 23

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 757916 [details]
Pull request 1019

Reference latest attachment

Comment 24

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 775933 [details]
PR 1106

I'll conveniently reassign this to Alex he knows more about this than me.

Comment 25

[Alexandre Poirot [:ochameau]]

Comment on attachment 775933 [details]
PR 1106

Thanks for having picked that bug!!

Comment 26

[Jordan Santell [:jsantell] [@jsantell]]

Awesome, thanks alex!

Comment 27

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/285203233d7f05dab3a97b0d67345653994e1abb
Bug 850296 - Connect our console module to the devtool console

https://github.com/mozilla/addon-sdk/commit/954e27e2d3ade5241c02af2e110b533d66e7aed1
Bug 850296 Ignore all console events in system event tests

https://github.com/mozilla/addon-sdk/commit/1fc28fd7b70249684a38da02e4ccedb6dea2f0ce
Merge pull request #1106 from jsantell/850296-use-devtool-console

Bug 850296 use devtool console, r+=@ochameau