823097 - Add support for %c style formatting in console

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Pedro Alves]

Style formatting is a very useful feature in firebug (and chrome >=24). Would be great to have it in console.

Link:

https://getfirebug.com/wiki/index.php/Console_API#console.log.28object.5B.2C_object.2C_....5D.29

Comment 1

[Sebastian Zartner [:sebo]]

Being compatible to each other would also avoid hacks to get to know which tool supports formatting console logs[1].

Sebastian

[1] http://code.google.com/p/fbug/issues/detail?id=4772#c18

Comment 2

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

I would like to know how many developers actually use this feature and what the use cases are.

Comment 3

[Sebastian Zartner [:sebo]]

Use cases I can think of are:
- Emphasizing specific log messages
- Styling messages depending on the module they are in
- Formatting parts of a message differently

Unfortunately we don't have exact numbers of people using this feature.

Sebastian

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I have been also blogging about the feature here:
http://www.softwareishard.com/blog/firebug/firebug-tip-styled-logging/

Honza

Comment 5

[Pedro Alves]

I use this in pentaho's CDF framework. We have the concept of components there, and each has it's own lifecycle.

I use a small routine to give each component a different colour so that in debug mode it's easy to visually identify individual component's lifecycle.

A more trivial usecase would be a logical layer of severe / warn / info that would be separated from the console logging levels

Comment 8

[Peter deHaan [:pdehaan]]

Saw this today on https://twitter.com/cubiq/status/426691158190731264 and was very sad to see that it isn't supported in Firefox 29 yet (especially since it looks like it was originally reported in Dec 2012).

Comment 9

[Mihai Sucan [:msucan]]

(In reply to Peter deHaan [:pdehaan] from comment #8)
> Saw this today on https://twitter.com/cubiq/status/426691158190731264 and
> was very sad to see that it isn't supported in Firefox 29 yet (especially
> since it looks like it was originally reported in Dec 2012).

I believe you mean something like this: http://imgur.com/TtOvVAt

With the fix for bug 843004 it's much easier to fix this bug. Going to attach a quick patch I did last evening.

Comment 10

[Mihai Sucan [:msucan]]

Attachment: wip1

This is a wip patch I did last evening, it seems to be working well. I only need to write a test and clean it up.

Comment 12

[Mihai Sucan [:msucan]]

Attachment: bug823097-2.diff

Updated the patch, this should be ready for reviews.

Got delayed by the work on netmonitor support for b2g and e10s, other console output work, and the console API rewrite from bug 965860.

bz: please review the changes from dom/base/ and dom/webidl/. As usual, apologies for any silly mistakes - I rarely write C++ code.

past: please review the rest of the changes.

Thank you!

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8402861 [details] [diff] [review]
bug823097-2.diff

At least document that in practice this is a sequence of strings, but that it's simpler for us to send them across as Values instead of nsAString?

I don't think you need the SequenceRooter there, and I'm not sure why the existing one is there: the sequence is part of a RootedDictionary.

Why do you need the makeDebuggeeValue bits if all we have are strings?  And if they're strings calling them aObj is a bit odd.

But most importantly, have you tests what happens if the styles include a -moz-binding to some https:// or http:// URL?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Or more generally, how do we make sure web pages can't use this to totally mess with your browser UI?  Does Firebug really support arbitrary styles here (think fixed positioning)?

Comment 15

[Sebastian Zartner [:sebo]]

> Does Firebug really support arbitrary styles here (think fixed positioning)?
No, the allowed styles are hard-coded.[1]

Sebastian

[1] https://github.com/firebug/firebug/blob/f517e30ccab54092d4ceff73fca787d9f30027e8/extension/content/firebug/console/consolePanel.js#L42

Comment 16

[Mihai Sucan [:msucan]]

(In reply to Boris Zbarsky [:bz] from comment #13)
> Comment on attachment 8402861 [details] [diff] [review]
> bug823097-2.diff
> 
> At least document that in practice this is a sequence of strings, but that
> it's simpler for us to send them across as Values instead of nsAString?

How can I make mStyles a sequence of strings? would sequence<string> or sequence<DOMString> work in the webidl? In cpp can I use Sequence<JSString>? Most importantly, are there any docs I could look into about this stuff?


> I don't think you need the SequenceRooter there, and I'm not sure why the
> existing one is there: the sequence is part of a RootedDictionary.

Should I remove the two lines then?

> Why do you need the makeDebuggeeValue bits if all we have are strings?  And
> if they're strings calling them aObj is a bit odd.

Good point. This is a remnant of the previous patch. Will fix.

> But most importantly, have you tests what happens if the styles include a
> -moz-binding to some https:// or http:// URL?

Tested with http and data URIs. The cleanupStyles() method specifically botches urls. I will look into -moz-binding as well.


(In reply to Boris Zbarsky [:bz] from comment #14)
> Or more generally, how do we make sure web pages can't use this to totally
> mess with your browser UI?  Does Firebug really support arbitrary styles
> here (think fixed positioning)?

My approach was that this is a matter of common sense. You can spam the output with plain console.log() calls, or not, if you want it useful. Same goes with custom styling. Maybe I am wrong. My only concern was, really, with external URLs.

Should we do this using a whitelist of properties? Like firebug does.

Thanks for your comments.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> How can I make mStyles a sequence of strings?

It would be sequence<DOMString> in the IDL and Sequence<nsString> in the C++.  But you'd have to do manual conversions from JS::Value to nsString, which doesn't seem worth it in this case; it's actually more work than what your patch ends up doing.

> are there any docs I could look into about this stuff?

https://developer.mozilla.org/en/Mozilla/WebIDL_bindings for the stuff specific to to Mozilla's implementation and the WebIDL spec itself (<http://heycam.github.io/webidl/>) for things like "what is the string type called".

> Should I remove the two lines then?

I think so, yes.

> Maybe I am wrong.

That depends.  Is the console stuff in its own subframe?  Or can the log output effectively overlay itself over parts of the browser UI outside the log output itself with the patch as it stands?

> Should we do this using a whitelist of properties? 

I suspect that's a good idea, yes.

Comment 18

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

Comment on attachment 8402861 [details] [diff] [review]
bug823097-2.diff

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

Sounds like you'll have an updated patch soon, so I'll wait for that. And I do agree about making sure only string values are allowed.

Comment 19

[Mihai Sucan [:msucan]]

(In reply to Boris Zbarsky [:bz] from comment #17)
> > are there any docs I could look into about this stuff?
> 
> https://developer.mozilla.org/en/Mozilla/WebIDL_bindings for the stuff
> specific to to Mozilla's implementation and the WebIDL spec itself
> (<http://heycam.github.io/webidl/>) for things like "what is the string type
> called".

Thanks!

> > Should I remove the two lines then?
> 
> I think so, yes.

Will do.

> > Maybe I am wrong.
> 
> That depends.  Is the console stuff in its own subframe?  Or can the log
> output effectively overlay itself over parts of the browser UI outside the
> log output itself with the patch as it stands?

The webconsole lives in its own subframe within the toolbox. Custom styles cannot overlay browser UI, nor toolbox UI. They can overlay the console toolbar, JS input and the object inspector.


> > Should we do this using a whitelist of properties? 
> 
> I suspect that's a good idea, yes.

Will do. Thanks for your comments.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Custom styles cannot overlay browser UI, nor toolbox UI. 

OK.  That's a lot better than I was afraid of.

I'd still aim for locking down by default; loosening up later is easier than locking down later...

Comment 21

[Mihai Sucan [:msucan]]

Attachment: bug823097-3.diff

Updated to address comments. Now there's a whitelist of css properties, and more tests.

Comment 22

[Mihai Sucan [:msucan]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=c09d5cc49669

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8404667 [details] [diff] [review]
bug823097-3.diff

In cleanupStyle, it might be better to save the names of all the props you don't want, then removeProperty them all, then .cssText to get the string.

>+              aStyles.AppendElement(JSVAL_NULL);

s/JSVAL_NULL/JS::NullValue()/

>+  sequence<any> styles;

Please do document that these are string-or-null in practice?

r=me with that

Comment 24

[Mihai Sucan [:msucan]]

(In reply to Boris Zbarsky [:bz] from comment #23)
> Comment on attachment 8404667 [details] [diff] [review]
> bug823097-3.diff
> 
> In cleanupStyle, it might be better to save the names of all the props you
> don't want, then removeProperty them all, then .cssText to get the string.

I had that in a local iteration of the patch, actually... and it seemed weird for no obvious reasons. 


> >+              aStyles.AppendElement(JSVAL_NULL);
> 
> s/JSVAL_NULL/JS::NullValue()/
> 
> >+  sequence<any> styles;
> 
> Please do document that these are string-or-null in practice?

Will do.

Thanks for the review. I will update the patch once Panos also has a chance to review it.

Comment 25

[Mihai Sucan [:msucan]]

Green try push: https://tbpl.mozilla.org/?tree=Try&rev=114b3a002f61

Comment 26

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

Comment on attachment 8404667 [details] [diff] [review]
bug823097-3.diff

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

::: browser/devtools/webconsole/console-output.js
@@ +1263,5 @@
> +      }
> +
> +      let piece = this._messagePieces[i];
> +      let style = this._styles[i];
> +      if (style && typeof style == "string" /* no long string support */) {

Could you move the comment above the test? Reading the code is harder this way.

@@ +1296,5 @@
> +    return result;
> +  },
> +
> +  /**
> +   * Given a style attribute value cleanup the string such that:

"Given a style attribute value, return a cleaned up version of the string such that:"

::: browser/devtools/webconsole/test/browser_webconsole_console_custom_styles.js
@@ +23,5 @@
> +             backgroundImage: false },
> +    sameStyleExpected: false,
> +  }, {
> +    // check that some properties are not allowed
> +    style: "color:pink;position:absolute:top:10px",

I assume you meant to use a semicolon, not a colon, after "absolute" or is it a weird error case you're testing?

Comment 27

[Mihai Sucan [:msucan]]

(In reply to Panos Astithas [:past] from comment #26)
> browser/devtools/webconsole/test/browser_webconsole_console_custom_styles.js
> @@ +23,5 @@
> > +             backgroundImage: false },
> > +    sameStyleExpected: false,
> > +  }, {
> > +    // check that some properties are not allowed
> > +    style: "color:pink;position:absolute:top:10px",
> 
> I assume you meant to use a semicolon, not a colon, after "absolute" or is
> it a weird error case you're testing?

Hah, that's a typo. Credits go to MyEyes Pre-Alpha build. Good catch. :)

Comment 28

[Mihai Sucan [:msucan]]

Attachment: bug823097-4.diff

Updated to address review comments from bz and Panos. Thanks for your reviews.

Landed: https://hg.mozilla.org/integration/fx-team/rev/74e475d14c01

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/74e475d14c01

Comment 30

[Will Bamberg [:wbamberg]]

Mihai, is this good enough: https://developer.mozilla.org/en-US/docs/Tools/Web_Console#Styling_messages ? I added the same thing in the console API page: https://developer.mozilla.org/en-US/docs/Web/API/console#Styling_console_output.

Comment 31

[Mihai Sucan [:msucan]]

(In reply to Will Bamberg [:wbamberg] from comment #30)
> Mihai, is this good enough:
> https://developer.mozilla.org/en-US/docs/Tools/Web_Console#Styling_messages
> ? I added the same thing in the console API page:
> https://developer.mozilla.org/en-US/docs/Web/API/
> console#Styling_console_output.

Looks good to me, thank you! We may want to mention somewhere that it accepts CSS properties and URLs are not allowed - not sure where...