Web Console and Firebug Console panel should not display 'undefined' for simple console.* API calls

RESOLVED WONTFIX

Status

P3
normal
RESOLVED WONTFIX
7 years ago
3 months ago

People

(Reporter: Honza, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [has-patch] [console-papercuts][polish-backlog] )

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
There is no simple way how Firebug's Console panel could distinguish 'undefined' results coming from an expression (entered e.g. on the command line) from 'undefined' returned by e.g. console.* APIs (or from simple variable assignments expressions)

From this reason Firebug now returns a string key "_firebugIgnore" that is caught by command line and console hooks and not producing any 'undefined' results.

However, this solutions doesn't work in case where the console.* APIs are used within built-in WebConsole, since the web-console doesn't know anything about "_firebugIgnore" and displays it as as string.

See http://code.google.com/p/fbug/issues/detail?id=4730 reported in Firebug issue list.

One option would be to settle on a common key like "ignore-undefined-result" that would be handled by all console.* API handlers (ie. Firebug and WebConsole now)

Thoughts?

Honza

Comment 1

7 years ago
I'll just mention Victor's idea from the other day: looking for explicit "return" in the function called.
Something we can quickly implement:

Generate a browser-session random string at runtime in ConsoleAPI.jsm and use that for our console.*() methods as the return value. Expose said string so that the Web Console code and Firebug can reuse it.

Simple and quick. (hopefully better than a well known "_firebugIgnore" string)

Thoughts?
(Reporter)

Comment 3

7 years ago
(In reply to Kevin Dangoor from comment #1)
> I'll just mention Victor's idea from the other day: looking for explicit
> "return" in the function called.
Not sure if I understand the idea correctly, but would it also solve the case where a function has more 'return' paths and one of them is not returning a proper value by mistake in the implementation? In such case, the developer should see 'undefined' in the console to be informed about possible bug.

Honza
(Reporter)

Comment 4

7 years ago
(In reply to Mihai Sucan [:msucan] from comment #2)
> Something we can quickly implement:
> 
> Generate a browser-session random string at runtime in ConsoleAPI.jsm and
> use that for our console.*() methods as the return value. Expose said string
> so that the Web Console code and Firebug can reuse it.
> 
> Simple and quick. (hopefully better than a well known "_firebugIgnore"
> string)
> 
> Thoughts?
Yep, I tend to like the idea of having some kind of a 'key' that is reused by dev tools and represents an alternative to JS 'undefined'.

What is the advantage of having a random generated browser-session string instead of a fixed string? I think that fixed string could be easier to reuse, no?

Honza
I would like us to *always* display undefined when user functions are executed. Adding any kind of heuristics that doesn't display undefined at times is, in my personal opinion, ugly. We should not promote anything otherwise. Developers who call their functions need to always know the exact return value.

The idea I had about the random string was to avoid other user functions from returning the same value to get the same effect. But that won't work because web devs can do |foo = console.log()| and get the random string into |foo|.

My main point here is I'd like us to make a clear distinction from chrome privileged functions over unprivileged ones.

Perhaps we need a solution similar to what we did for JSTermHlpers functions. That is... turn a flag on before executing the code in the sandbox, then off. This flag could be used to determine if the undefined value is displayed or not.
Created attachment 566280 [details] [diff] [review]
quick wip

Proposed approach, quick wip - missing tests and comments.

Basic idea: set console.__returnValue__ to tell the value you want the methods to return, and you'll be ignoring that specific value in the Web Console output, and in Firebug.
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Attachment #566280 - Flags: feedback?(rcampbell)
Comment on attachment 566280 [details] [diff] [review]
quick wip

yeah, this should work. The Date.now() is a little funny, but I guess you just need "any sort of random number/string".

All the extra returns in the catch blocks are weird. 

+
+XPCOMUtils.defineLazyGetter(this, "ConsoleAPIStorage", function () {
+  let obj = {};
+  Cu.import("resource://gre/modules/ConsoleAPIStorage.jsm", obj);
+  return obj.ConsoleAPIStorage;
+});
+
+XPCOMUtils.defineLazyGetter(this, "isNonNativeGetter", function () {
+  let obj = {};
+  Cu.import("resource:///modules/PropertyPanel.jsm", obj);
+  return obj.isNonNativeGetter;
+});
 
not really needed for this patch, is it?

 
+    let options = {
+      innerID: innerID,
+      outerID: outerID,
+    };
+

or this?

+                                 "console-api-log-event", aOptions.outerID);
+
+    return result;
   },
 
(from the notifyObservers method) Is this return needed?
Attachment #566280 - Flags: feedback?(rcampbell) → feedback+
(Reporter)

Comment 8

7 years ago
(In reply to Mihai Sucan [:msucan] from comment #5)
> I would like us to *always* display undefined when user functions are
> executed. Adding any kind of heuristics that doesn't display undefined at
> times is, in my personal opinion, ugly. We should not promote anything
> otherwise. Developers who call their functions need to always know the exact
> return value.
Yep, that was my point too, agree.

> The idea I had about the random string was to avoid other user functions
> from returning the same value to get the same effect. But that won't work
> because web devs can do |foo = console.log()| and get the random string into
> |foo|.
So, sounds like we don't need the random string, no?


> My main point here is I'd like us to make a clear distinction from chrome
> privileged functions over unprivileged ones.
> 
> Perhaps we need a solution similar to what we did for JSTermHlpers
> functions. That is... turn a flag on before executing the code in the
> sandbox, then off. This flag could be used to determine if the undefined
> value is displayed or not.
How Firebug should get the current value of |_ignoreReturnValue| ?
(if it isn't a random string the problem is gone)

Honza
(In reply to Jan Honza Odvarko from comment #8)
> (In reply to Mihai Sucan [:msucan] from comment #5)
> > I would like us to *always* display undefined when user functions are
> > executed. Adding any kind of heuristics that doesn't display undefined at
> > times is, in my personal opinion, ugly. We should not promote anything
> > otherwise. Developers who call their functions need to always know the exact
> > return value.
> Yep, that was my point too, agree.
> 
> > The idea I had about the random string was to avoid other user functions
> > from returning the same value to get the same effect. But that won't work
> > because web devs can do |foo = console.log()| and get the random string into
> > |foo|.
> So, sounds like we don't need the random string, no?

We don't, in the way I initially thought of.

> > My main point here is I'd like us to make a clear distinction from chrome
> > privileged functions over unprivileged ones.
> > 
> > Perhaps we need a solution similar to what we did for JSTermHlpers
> > functions. That is... turn a flag on before executing the code in the
> > sandbox, then off. This flag could be used to determine if the undefined
> > value is displayed or not.
> How Firebug should get the current value of |_ignoreReturnValue| ?
> (if it isn't a random string the problem is gone)

You don't need to. In the Firebug code, when you need to evaluate code just set console.__returnValue__ to anything you want (random or not). All of the console.*() API methods will return the given value that you can store somewhere and compare when it's time to display the eval output. Skip that return value.

This patch doesn't make it a requirement to have a random value, it's up to the client that evaluates the console.*() methods to decide what return value to skip.

This works only if Firebug doesn't replace our Console API implementation. So, for the Firebug console API implementation you need to do the same. Add code to the console.*() methods that reads the console.__returnValue__ property and returns it.
(Reporter)

Comment 10

7 years ago
> You don't need to. In the Firebug code, when you need to evaluate code just
> set console.__returnValue__ to anything you want (random or not). All of the
> console.*() API methods will return the given value that you can store
> somewhere and compare when it's time to display the eval output. Skip that
> return value.
> 
> This patch doesn't make it a requirement to have a random value, it's up to
> the client that evaluates the console.*() methods to decide what return
> value to skip.
I see, that's good.

> This works only if Firebug doesn't replace our Console API implementation.
Note that Firebug does replace the console object if the Console panel is enabled (there is more APIs in Firebug's console at the present).

> So, for the Firebug console API implementation you need to do the same. Add
> code to the console.*() methods that reads the console.__returnValue__
> property and returns it.
Yep, this is already done.

So, Let's say Firebug is using "_firebugIgnore" key in its |console| object and ignoring any return value that is equal to this key. The web console should consequently use the same key that belongs to the current |console| object and also ignore equal return value - in cases where the user types e.g. "console.log()" in web-console's command line (where in fact Firebug's |console| object is actually used).

Here is what I mean (HUDService.jsm):

    if (this._ignoreReturnValue && !window.console.__returnValue__) {
      window.console.__returnValue__ = this._ignoreReturnValue;
    }
    else if (window.console.__returnValue__) {
        this._ignoreReturnValue = window.console.__returnValue__;
    }

Honza
(In reply to Jan Honza Odvarko from comment #10)
> > This works only if Firebug doesn't replace our Console API implementation.
> Note that Firebug does replace the console object if the Console panel is
> enabled (there is more APIs in Firebug's console at the present).

Sure.

> > So, for the Firebug console API implementation you need to do the same. Add
> > code to the console.*() methods that reads the console.__returnValue__
> > property and returns it.
> Yep, this is already done.
> 
> So, Let's say Firebug is using "_firebugIgnore" key in its |console| object
> and ignoring any return value that is equal to this key. The web console
> should consequently use the same key that belongs to the current |console|
> object and also ignore equal return value - in cases where the user types
> e.g. "console.log()" in web-console's command line (where in fact Firebug's
> |console| object is actually used).
> 
> Here is what I mean (HUDService.jsm):
> 
>     if (this._ignoreReturnValue && !window.console.__returnValue__) {
>       window.console.__returnValue__ = this._ignoreReturnValue;
>     }
>     else if (window.console.__returnValue__) {
>         this._ignoreReturnValue = window.console.__returnValue__;
>     }

The Web Console generates its own __returnValue__ and gives it to your console object implementation by setting window.console.__returnValue__. What you ask here is for the Web Console to read your __returnValue__ and to ignore that value, which, sure, we can do, but I don't see the benefit.

Why can't firebug use the console.__returnValue__ given by Web Console before the user code is evaluated?

(This patch doesn't attempt to work with the current Firebug implementation. As explained, Firebug's console API methods need to be updated to return console.__returnValue__. Consequently this patch is only about fixing the problem in the Web Console + Console API code, while allowing it to inter-operate with Firebug.)
(Reporter)

Comment 12

7 years ago
I have just committed a (Firebug) patch that fixes the current Firebug implementation. Firebug uses the console.__returnValue__ set by Web Console. If the __returnValue__ is not available, Firebug is using "_firebugIgnore" (as before).

Tested with Mihai's patch and works well!

Honza
(In reply to Jan Honza Odvarko from comment #12)
> I have just committed a (Firebug) patch that fixes the current Firebug
> implementation. Firebug uses the console.__returnValue__ set by Web Console.
> If the __returnValue__ is not available, Firebug is using "_firebugIgnore"
> (as before).
> 
> Tested with Mihai's patch and works well!

Great!

I'll get to update this patch such that we can land it ... as soon as possible. There's, at the moment, lots of other stuff to work on...
filter on pegasus
Priority: -- → P3
Whiteboard: [has-patch]

Comment 15

6 years ago
triage. Filter on PINKISBEAUTIFUL
Component: Developer Tools → Developer Tools: Console

Comment 16

6 years ago
Looks like the Firebug console API and the WebConsole one do not interact each other anymore. 

I have experimented the removal of console.__returnValue__ [1] and I could not reproduce the Firebug issue 4730 [2].

So is console.__returnValue__ still relevant? 

Florent

[1] https://github.com/fflorent/firebug/commit/15ab9b7b4def100f84adae64ac0a760fbeeb5508#L3R455
[2] http://code.google.com/p/fbug/issues/detail?id=4730
Florent: I haven't checked, but this is not a high priority bug for us. Also, I'm not sure that we shouldn't display |undefined| for console API calls. I mean, we are special-casing for specific function calls -- that's not nice. Chrome's devtools show undefined too.

De-assigning myself from the bug - I'll not work on this soon. If someone wants to take it, feel free to do so. Thanks!
Assignee: mihai.sucan → nobody
Status: ASSIGNED → NEW
OS: Windows Vista → All
Hardware: x86 → All
Whiteboard: [has-patch] → [console-papercuts]
Whiteboard: [console-papercuts] → [has-patch] [console-papercuts]
I'm skeptical that we should do this at all. we & chrome diverge from firebug here, and ours ( and chrome's ) approach of printing 'undefined' is technically correct if a little busier. I think people are used to it.

Comment 19

4 years ago
I think a person who prints "console.log(...)" would expect just one value, not two.

Though that should be rather up to the users to tell what they expect (our point of view is biased).

Anyway, maybe that could be part of the specificities of Firebug 3?

Florent
console.* API calls are no more special than, say, alert() calls, which also display their return value. In fact the only calls where we hide the return value of |undefined| are those provided by the web console itself (JSTermHelpers), like clear(), cd() or inspect(). I think Firebug's behavior stems from the distant past when it pioneered the console API by injecting these helper functions into the DOM, but now that the console API is a de-facto DOM API and provided by the browser, I don't think this special-casing is warranted any more.
Whiteboard: [has-patch] [console-papercuts] → [has-patch] [console-papercuts][devedition-40]
(In reply to fayolle-florent from comment #19)
> I think a person who prints "console.log(...)" would expect just one value,
> not two.

The thing is that the console input isn't just for console.log() but any JS code, you could also have `console.log(...); something()`
It can be unexpected to sometime display a value and some other times do nothing.
It sounds more scientifically right to always display the returned value of the input.

But Honza is integrating Firebug within Firefox, so I'm tempted to say to WONTFIX this while allowing Firebug to do such filtering. Do you still need some platform support to do the filtering?
Flags: needinfo?(odvarko)
(Reporter)

Comment 22

4 years ago
(In reply to Alexandre Poirot [:ochameau] from comment #21)
> It sounds more scientifically right to always display the returned value of
> the input.
> 
> But Honza is integrating Firebug within Firefox, so I'm tempted to say to
> WONTFIX this while allowing Firebug to do such filtering.
Sounds good to me.

> Do you still need some platform support to do the filtering?
I don't know, but if yes, it can be reported separately.

Closing

Honza
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(odvarko)
Resolution: --- → WONTFIX
Whiteboard: [has-patch] [console-papercuts][devedition-40] → [has-patch] [console-papercuts][polish-backlog]

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.