Closed Bug 989619 Opened 6 years ago Closed 4 years ago

console.log must be bound to console object now, when it previously didn't

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: fitzgen, Assigned: baku)

References

Details

(Keywords: DevAdvocacy, Whiteboard: [console-spec])

Attachments

(3 files, 2 obsolete files)

STR:

[1,2,3].forEach(console.log)

ER:

logs 3 messages in the console

AR:

Error: 'log' called on an object that does not implement interface Console.

Suspect this was caused by https://bugzilla.mozilla.org/show_bug.cgi?id=965860
Depends on: 965860
[1,2,3].forEach(console.log) doesn't seem to work in other browsers either.
Point taken, but I don't think it really matters. Here's why we should fix it:

* It used to work

* It isn't spec'd either way so we aren't doing nonstandard things

* It's really annoying to always have to bind
FWIW, node's |console.log| doesn't need to be bound either.
If and when Console interface is spec'ed it will most probably follow the normal interface handling which is defined in the WebIDL spec - and we have now that behavior.
But sure, Console API isn't spec'ed atm.
And in case of log I can see [1,2,3].forEach(console.log) being useful.

I wonder if there is any WebIDL magic which could be used for this.
This was a purposeful behavior change.  See 965860 comment 59 through bug 965860 comment 61.

> but I don't think it really matters. 

It does to some extent.  If it doesn't work in other browsers, then there is no strong web compat need for it.

> * It used to work

True.  Except in other browsers.

> * It isn't spec'd either way so we aren't doing nonstandard things

But we _do_ want to get it specced, and all other instance methods in the platform aren't bound methods.  Why should console be special?

More to the point, if we don't think other browsers would be willing to implement the pre-bound behavior, then we shouldn't be shipping it, because it won't be able to be standardized.

> * It's really annoying to always have to bind

You only have to bind if you try to use the method separately from the console object...

We can try to add a bunch of complexity to handle this, but we need some indication that other UAs are willing to get on board with the idea first.
Blocks: 965860
No longer depends on: 965860
Issue on DeveloperToolsWG where hopefully people from the Chrome team will comment: https://github.com/DeveloperToolsWG/console-object/issues/27

> Why should console be special?

One possible argument: console is a singleton, so the "this" value should have no meaning (modulo cross-window stuff that doesn't come up in practice). This matches "Math" at least.
(In reply to Simon Lindholm from comment #7)
> > Why should console be special?
> 
> One possible argument: console is a singleton, so the "this" value should
> have no meaning (modulo cross-window stuff that doesn't come up in
> practice). This matches "Math" at least.
Ignoring that console has tab-dependent side-effects while Math functions are stateless (minus random), of course :-p

I'd love to see console.log being bound by default too. But at the same time, it's not too hard to bind by default (must be a 10-line script at most).
Another track I've been looking at is binding destructuring (via a different syntax). Maybe a promising lead? 
https://bugs.ecmascript.org/show_bug.cgi?id=1183
That could eventually look like:
    const ::{log, warn} = console;
    [1,2,3].forEach(log);
what if console was the interface name and methods would be static?
> One possible argument: console is a singleton

No more so than "window", "navigator", "crypto", "location", "history", "external", "applicationCache", "performance", and probably other things on Window.  In fact less than some of them.

> modulo cross-window stuff that doesn't come up in practice

Why do you think it doesn't come up in practice?  I'd expect that it does, in fact.

> This matches "Math" at least.

If you want to match Math, you should probably have a Console.log, since that's how non-object-dependent stuff is typically exposed on the web.  The fact that console's methods acted bound (to the window, in fact) in Gecko was basically an artifact of how it happened to be implemented as not a normal DOM object...

We could name the interface "console", I guess; since the methods still have an attached window it would all work out.  It would be pretty bizarre, but that's what we get with legacy nonstandard stuff.  :(
console + static methods would give similar behavior what CSS interface has.
Yeah, the bizarre part is having a lowercased class/interface/namespace name.
See Also: → 992644
Hmm... Might break Gecko-only code like FirefoxOS apps indeed. Everything.me will update, but it's unclear whether all apps will have the same chance.

Symbolically setting the everything.me bug as blocking.
Blocks: 992644
> No more so than "window", "navigator", "crypto", "location", "history", "external",
> "applicationCache", "performance", and probably other things on Window.  In fact less
> than some of them.

To be fair, lots of things on "window" are [ImplicitThis], and as a web author I use methods of the other ones so seldom that the question of aliases doesn't come up. But I see your point--I wasn't aware the web platform was so consistent in this regard. I'll let you be the judge of whether consistency is worth more than developer convenience here.

> Why do you think it doesn't come up in practice?  I'd expect that it does, in fact.

The case where "this" would matter that I can think of is `winA.console.log.apply(winB.console, args)`, which seems like a very roundabout way of logging things, and in most cases would go to the same console anyway.
> winA.console.log.apply(winB.console, args)

Yes, I think so.  

> and in most cases would go to the same console anyway.

Are the timing functions on console per-window or per-console?
> Yes, I think so.

It strikes me as a very weird thing to do, but OK.

> Are the timing functions on console per-window or per-console?

I don't know, it probably varies between devtools. But sure, there are differences in that vein (another thing I know of is that winA.console.clear only clears messages from winA's frame subtree in some devtools), they just feel unlikely to be noticeable in practice. In any case, it shouldn't be a large compatibility hazard.
So is there any resolution?  Is there interest in seeing what agreements we can get from other vendors?  There is an open issue here https://github.com/DeveloperToolsWG/console-object/issues/27 if anyone would like to send a proposed pull request or make comments and we can see what we can do to get agreements?
Looks like a discussion about this has restarted at https://github.com/whatwg/console/issues/3#issuecomment-172285367
It looks like WebKit has changed[1] to auto-bind console.log and
friends so you could save them off to variable without first binding
them to the console object.

There seems to be general agreement on our mailing list that we should do this as well.

[1]: https://github.com/whatwg/console/issues/3#issuecomment-216417014
I and bz have a plan about this. I'll assign this bug to myself.
Assignee: nobody → amarchesini
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [console-spec]
Attached patch console.patch (obsolete) — Splinter Review
I guess this is the first part.
But doing this we break [1,2,3].forEach(console.bind.log)
Attachment #8749536 - Flags: review?(bzbarsky)
> But doing this we break [1,2,3].forEach(console.bind.log)

I'm not sure what you mean.  Do you mean [1,2,3].forEach(console.log.bind(console))?
My thing from comment 22 works fine.

What doesn't work fine is the hasNativeConsoleAPI function in http://mxr.mozilla.org/mozilla-central/source/devtools/server/actors/webconsole.js because it tries to use instanceof aWindow.Console, which of course fails now.  We need a replacement for that.  For now, we can wrap back into an Xray and then examine the Object.prototype.toString bits, but the spec is pushing for that to claim just "[object Object]"...

Most simply, we could have a chromeonly isRealConsole boolean constant on the interface, and check _that_ after rewrapping into Xrays.
Attached patch console.patchSplinter Review
Attachment #8749536 - Attachment is obsolete: true
Attachment #8749536 - Flags: review?(bzbarsky)
Attachment #8749668 - Flags: review?(bzbarsky)
Comment on attachment 8749668 [details] [diff] [review]
console.patch

>+      isNative = aWindow.wrappedJSObject.console.IS_NATIVE_CONSOLE;

No, this is wrong. aWindow.wrappedJSObject.console is an Xray waiver, so can spoof IS_NATIVE_CONSOLE.  You need to leave the comment in place (because it explains why we .wrappedJSObject) here, and then once you get the .console you need to re-wrap the resulting object in an Xray before examining .IS_NATIVE_CONSOLE.

Probably worth adding a test that IS_NATIVE_CONSOLE is not visible from a web page, and another that it _is_ visible via Xrays.

>+Console::ProfileMethodInternal(JSContext* aCx, const nsAString& aAction,
>+                       const Sequence<JS::Value>& aData)

Fix indent, please.

r=me.  That was ... a lot simpler than I expected.  ;)
Attachment #8749668 - Flags: review?(bzbarsky) → review+
Attached patch console2.patchSplinter Review
Attachment #8750704 - Flags: review?(peterv)
Note that this approach, especially when combined with bug 1270601, may not be web-compatible due to people poking console.__proto__.  See also https://github.com/whatwg/console/issues/3#issuecomment-219549472
Comment on attachment 8750704 [details] [diff] [review]
console2.patch

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

::: dom/bindings/Codegen.py
@@ +6019,5 @@
>          return "JS::NumberValue(%sU)" % (value.value)
>      if tag in [IDLType.Tags.int64, IDLType.Tags.uint64]:
>          return "JS::CanonicalizedDoubleValue(%s)" % numericValue(tag, value.value)
>      if tag == IDLType.Tags.bool:
> +        return "JS::BooleanValue(true)" if value.value else "JS::BooleanValue(false)"

I'd just do |return "JS::BooleanValue(%s)" % toStringBool(value.value)|
Attachment #8750704 - Flags: review?(peterv) → review+
Also, please add a test to dom/bindings/test/TestCodeGen.webidl of a static method with a reserved C++ keyword as the name.
Note that you will probably want to add [ProtoObjectHack] to console to address the issue I mention in comment 27.
And please add a test for the proto thing....
Flags: needinfo?(amarchesini)
Attached patch patch 3 - console.__proto__ (obsolete) — Splinter Review
bz, do you mind to take a quick look at this test?
Flags: needinfo?(amarchesini) → needinfo?(bzbarsky)
Comment on attachment 8759631 [details] [diff] [review]
patch 3 - console.__proto__

I expect this test as written passes both with and without the ProtoObjectHack thing, no?  So it's not really a useful test for checking that the proto is set up correctly.

What you need to test is that Object.getPrototypeOf(console) != Object.prototype, but Object.prototypeOf(Object.prototypeOf(console)) == Object.prototype.
Flags: needinfo?(bzbarsky)
Attachment #8759631 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Comment on attachment 8759649 [details] [diff] [review]
patch 3 - console.__proto__

r=me, but I think the pointless for-in loop could also be replaced with:

  is(Object.getOwnPropertyNames(Object.getPrototypeOf(console)).length, 0,
     "Should have empty prototype object");

which again would have failed without the ProtoObjectHack bit.
Flags: needinfo?(bzbarsky)
Attachment #8759649 - Flags: review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c54d1374d42a
patch 1 - Codegen should allow static methods with a reserved C++ keywords as the name, r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4c1500759bf
patch 2 - console API should use webIDL namespaces, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fac4674fdfa
patch 3 - console API should use [ProtoObjectHack], r=bz
Depends on: 1278630
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.