722267 - Option to enable/disable timestamps for messages in the console

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Paul Rouget [:paul]]

Timestamps are not always useful. We need a way to toggle them.

Comment 1

[Alex Limi (:limi) — Firefox UX Team]

Stephen hopefully knows what the context here is. :)

Comment 2

[Stephen Horlander [:shorlander]]

Paul and I talked about adding a toggle button to the toolbar. A contextual menu item is another option if this feels too visible.

 +---------------------------------------------
 |+---+  +----------+ +-----------+ +----------
 || < |  |   Net    | |    CSS    | |     JS
 |+---+  +----------+ +-----------+ +----------
 +-------------+-------------------------------
 |16:40:01.350 |
 |16:40:02.350 |
 |16:40:03.350 |
 |16:40:04.350 |
 |16:40:05.350 |
 |16:40:06.350 |
 |16:40:07.350 |
 |16:40:08.350 |
 |16:40:09.350 |
 |16:40:10.350 |
 |16:40:11.350 |
 |16:40:12.350 |
 |16:40:13.350 |
 |16:40:14.350 |
 |16:40:15.350 |
 +-------------+-------------------------------

Comment 3

[Paul Rouget [:paul]]

A "clock" icon would work I think.

Comment 4

[Mihai Sucan [:msucan]]

I doubt shorlander is working on this now. Please reassign to whoever works on this.

Thanks for the bug report and for the ideas. This is good stuff for the upcoming console output rewrite.

Comment 6

[Mihai Sucan [:msucan]]

Paul, thanks for taking the bug! Please base your work on top of bug 760876 - we currently have some important patches in reviews, lots of output work is happening.

Comment 7

[Paul Rouget [:paul]]

Is it ok if we should the timestemps only if a checkbox is checked in the option panel?

Comment 8

[Paul Rouget [:paul]]

s/should/show

Comment 9

[Mihai Sucan [:msucan]]

(In reply to Paul Rouget [:paul] from comment #7)
> Is it ok if we should the timestemps only if a checkbox is checked in the
> option panel?

Yes.

Comment 10

[Andrei Oprea [:andreio]]

I am planning to add it in the Devtools Toolbox Option under Web Console

Comment 11

[Andrei Oprea [:andreio]]

The checkbox
http://i.imgur.com/lF5sjtc.png

Web Console with and without timestamps
http://i.imgur.com/i8DXdBU.png

It works. Let me know if there is anything I should change before I submit my patch.

I had a question about the .xul format. I've added a checkbox in toolbox-options.xul and I want timestamps to be enabled by default so I've added a checked=true attribute to the checkbox. Is this necessary ? The reason I'm confused is because I have had to set the pref in profile/firefox.js of that checkbox to true before it worked.

Comment 12

[Mihai Sucan [:msucan]]

Andrei: you don't need to add the checked=true attribute to the checkbox. The 'checked' attribute is set automatically during runtime, based on the pref.

Thanks for your work!

Comment 13

[Paul Rouget [:paul]]

(In reply to Andrei Oprea [:andreio] from comment #11)
> The checkbox
> http://i.imgur.com/lF5sjtc.png
> 
> Web Console with and without timestamps
> http://i.imgur.com/i8DXdBU.png

Looks nice! Can you submit the patch and flag it "f?" with me (:paul) or Mihai (:msucan)?

Comment 14

[Andrei Oprea [:andreio]]

Attachment: timestamp.patch

Comment 15

[Paul Rouget [:paul]]

Comment on attachment 813629 [details] [diff] [review]
timestamp.patch

I'm wondering if it would not be better to use CSS for that. Mihai, what do you think?

Comment 16

[Andrei Oprea [:andreio]]

Like using a "hidden" class on the timestamp element ? 

I was debating this myself, depends on what you expect the feature to do. It could enable/disable timestamps. Or toggle them, in which case you could hide/show them depending on the options.

Comment 17

[Mihai Sucan [:msucan]]

Comment on attachment 813629 [details] [diff] [review]
timestamp.patch

Agreed with Paul. Please use a CSS-based mechanism for toggling the timestamps.

Thanks!

Comment 18

[Mihai Sucan [:msucan]]

(In reply to Andrei Oprea [:andreio] from comment #16)
> Like using a "hidden" class on the timestamp element ? 

Yes, something like this. You can actually use a single class on the output container, something like .timestampsEnabled.

The reason we prefer this approach is because it's simpler and less invasive.

Comment 19

[Andrei Oprea [:andreio]]

I'll make the necessary changes to my patch. I did have some questions because I'm not as familiar with the code.
I was looking at
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox-options.js#144
So this emits an event for all checkboxes, does this include the one that I added ? If so I see multiple gDevTools.on("pref-changed") throughout the code but I'm not sure where is best to handle the event.

Comment 20

[Mihai Sucan [:msucan]]

(In reply to Andrei Oprea [:andreio] from comment #19)
> I'll make the necessary changes to my patch. I did have some questions
> because I'm not as familiar with the code.
> I was looking at
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/
> toolbox-options.js#144
> So this emits an event for all checkboxes, does this include the one that I
> added ?

Yes, it should.

> If so I see multiple gDevTools.on("pref-changed") throughout the
> code but I'm not sure where is best to handle the event.

In webconsole.js :: WebConsoleFrame add an event handler in initUI(). gDevTools.on("pref-changed", handler) -- make sure you also remove the event handler in destroy().

Comment 21

[Andrei Oprea [:andreio]]

Attachment: timestamp.patch

Added CSS class to toggle the timestamp.
Should the toggle button also show older timestamps that have been hidden or only work from that moment on ?

Comment 22

[Paul Rouget [:paul]]

Comment on attachment 815125 [details] [diff] [review]
timestamp.patch

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
>+pref("devtools.webconsole.enableConsoleTimestamp", true);

I would set this to "false" by default. But that's just my opinion :)

>diff --git a/browser/devtools/framework/toolbox-options.xul b/browser/devtools/framework/toolbox-options.xul
>--- a/browser/devtools/framework/toolbox-options.xul
>+++ b/browser/devtools/framework/toolbox-options.xul
>@@ -46,19 +46,22 @@
>                 <menuitem label="&options.defaultColorUnit.rgb;" value="rgb"/>
>                 <menuitem label="&options.defaultColorUnit.name;" value="name"/>
>               </menupopup>
>             </menulist>
>           </hbox>
>         </vbox>
>         <label value="&options.webconsole.label;"/>
>         <vbox id="webconsole-options" class="options-groupbox">
>-          <checkbox label="&options.enablePersistentLogging.label;"
>-                    tooltiptext="&options.enablePersistentLogging.tooltip;"
>-                    data-pref="devtools.webconsole.persistlog"/>
>+           <checkbox label="&options.enablePersistentLogging.label;"
>+                     tooltiptext="&options.enablePersistentLogging.tooltip;"
>+                     data-pref="devtools.webconsole.persistlog"/>
>+           <checkbox label="&options.enableConsoleTimestamp.label;"
>+                     tooltiptext="&options.enableConsoleTimestamp.tooltip;"
>+                     data-pref="devtools.webconsole.enableConsoleTimestamp"/>

Indentation is off.

>diff --git a/browser/devtools/webconsole/webconsole.js b/browser/devtools/webconsole/webconsole.js
>+const ENABLE_WEBCONSOLE_TIMESTAMP = "devtools.webconsole.enableConsoleTimestamp";
>+let TIMESTAMP_DISABLED = false;

We only capitalized names for `const`. Does this need to be global?

> /**
>  * A WebConsoleFrame instance is an interactive console initialized *per target*
>  * that displays console log data as well as provides an interactive terminal to
>  * manipulate the target's document content.
>  *
>  * The WebConsoleFrame is responsible for the actual Web Console UI
>  * implementation.
>@@ -548,16 +551,19 @@ WebConsoleFrame.prototype = {
>     });
> 
>     let clearButton = doc.getElementsByClassName("webconsole-clear-console-button")[0];
>     clearButton.addEventListener("command", () => {
>       this.owner._onClearButton();
>       this.jsterm.clearOutput(true);
>     });
> 
>+    gDevTools.on("pref-changed", toggleTimestamps);
>+    TIMESTAMP_DISABLED = !Services.prefs.getBoolPref(ENABLE_WEBCONSOLE_TIMESTAMP);
>+
>     this.jsterm = new JSTerm(this);
>     this.jsterm.init();
>     this.jsterm.inputNode.focus();
>   },
> 
>   /**
>    * Initialize the default filter preferences.
>    * @private
>@@ -2369,31 +2375,35 @@ WebConsoleFrame.prototype = {
>       repeatNode.className = "repeats";
>       repeatNode.textContent = 1;
>       repeatNode._uid = [bodyNode.textContent, aCategory, aSeverity, aLevel,
>                          aSourceURL, aSourceLine].join(":");
>     }
> 
>     // Create the timestamp.
>     let timestampNode = this.document.createElementNS(XHTML_NS, "span");
>-    timestampNode.className = "timestamp devtools-monospace";
>+    let timestampClassList = "timestamp devtools-monospace";
>+    if (TIMESTAMP_DISABLED) {
>+      timestampClassList += " timestampsDisabled";
>+    }
>+    timestampNode.className = timestampClassList;

You enable/disable timestemps per message. I thing you can do that at a higher level.



~~~

This is a good first step, but I think there's a better approach:


In WebConsoleFrame's constructor, I'd do this:

> this._toggleShowTimeStamps = this._toggleShowTimeStemps.bind(this);
> gDevTools.on("pref-changed", this._toggleShowTimeStamps);

So you'd need a new method _toggleShowTimeStamps in WebConsoleFrame.

This method would check if the pref is on or off, and then, add a class to "#output-container", and then, in CSS:

> #output-container.dont-show-timestamps > .message > .timestamp {
>   display: none;
> }

If you have any question, let me know.

Comment 23

[Mihai Sucan [:msucan]]

(In reply to Andrei Oprea [:andreio] from comment #21)
> Created attachment 815125 [details] [diff] [review]
> timestamp.patch
> 
> Added CSS class to toggle the timestamp.
> Should the toggle button also show older timestamps that have been hidden or
> only work from that moment on ?

Also show older timestamps that have been hidden. You may also want to rename the pref from devtools.webconsole.enableConsoleTimestamp to devtools.webconsole.timestampMessages.

Paul, thanks for your quick feedback on Andrei's patch.

Comment 24

[Andrei Oprea [:andreio]]

Attachment: timestamp.patch

The new default is "off" :)

When you toggle you hide/show all of the timestamps in the Web Console.

Comment 25

[Andrei Oprea [:andreio]]

(In reply to Mihai Sucan [:msucan] from comment #23)
> (In reply to Andrei Oprea [:andreio] from comment #21)
> > Created attachment 815125 [details] [diff] [review]
> > timestamp.patch
> > 
> > Added CSS class to toggle the timestamp.
> > Should the toggle button also show older timestamps that have been hidden or
> > only work from that moment on ?
> 
> Also show older timestamps that have been hidden. You may also want to
> rename the pref from devtools.webconsole.enableConsoleTimestamp to
> devtools.webconsole.timestampMessages.

I am going to do this now. Sorry I overlooked.

> 
> Paul, thanks for your quick feedback on Andrei's patch.

Comment 26

[Paul Rouget [:paul]]

Comment on attachment 817290 [details] [diff] [review]
timestamp.patch

> // The developer tools editor configuration:
>diff --git a/browser/devtools/framework/toolbox-options.xul b/browser/devtools/framework/toolbox-options.xul
>--- a/browser/devtools/framework/toolbox-options.xul
>+++ b/browser/devtools/framework/toolbox-options.xul
>@@ -46,19 +46,22 @@
>                 <menuitem label="&options.defaultColorUnit.rgb;" value="rgb"/>
>                 <menuitem label="&options.defaultColorUnit.name;" value="name"/>
>               </menupopup>
>             </menulist>
>           </hbox>
>         </vbox>
>         <label value="&options.webconsole.label;"/>
>         <vbox id="webconsole-options" class="options-groupbox">
>-          <checkbox label="&options.enablePersistentLogging.label;"
>+           <checkbox label="&options.enablePersistentLogging.label;"
>                     tooltiptext="&options.enablePersistentLogging.tooltip;"
>                     data-pref="devtools.webconsole.persistlog"/>
>+           <checkbox label="&options.enableConsoleTimestamp.label;"
>+                    tooltiptext="&options.enableConsoleTimestamp.tooltip;"
>+                    data-pref="devtools.webconsole.enableConsoleTimestamp"/>


Indentation is off.

> 
>   EventEmitter.decorate(this);
> }
> exports.WebConsoleFrame = WebConsoleFrame;
> 
> WebConsoleFrame.prototype = {
>   /**
>    * The WebConsole instance that owns this frame.
>@@ -483,16 +489,21 @@ WebConsoleFrame.prototype = {
> 
>     let doc = this.document;
> 
>     this.filterBox = doc.querySelector(".hud-filter-box");
>     this.outputNode = doc.getElementById("output-container");
>     this.completeNode = doc.querySelector(".jsterm-complete-node");
>     this.inputNode = doc.querySelector(".jsterm-input-node");
> 
>+    // If preferences are set to hide the timestamps
>+    if (!Services.prefs.getBoolPref(ENABLE_WEBCONSOLE_TIMESTAMP)) {
>+      this.output.classList.add("hideTimestamps");
>+    }

} else {
  this.output.classList.remove("hideTimestamps");
}

>     this._setFilterTextBoxEvents();
>     this._initFilterButtons();
>     this._changeClearModifier();
> 
>     let fontSize = this.owner._browserConsole ?
>                    Services.prefs.getIntPref("devtools.webconsole.fontSize") : 0;
> 
>     if (fontSize != 0) {
>@@ -2671,16 +2682,27 @@ WebConsoleFrame.prototype = {
>         return;
>       }
> 
>       aCallback(this, aEvent);
>     }, false);
>   },
> 
>   /**
>+   * Event handler for enabling timestamps in the toolbox options
>+  */
>+  _toggleShowTimeStamps: function WCF__toggleShowTimeStamps (event, data)
>+  {
>+    let outputContainer = this.window.document.getElementById("output-container");
>+    if (data.pref == "devtools.webconsole.enableConsoleTimestamp") {
>+      outputContainer.classList.toggle("hideTimestamps");
>+    }
>+  },

if (enabledConsoleTimestamp) {
  classList.remove("hideTimestamps");
} else {
  classList.add("hideTimestamps");
}

Comment 27

[Andrei Oprea [:andreio]]

Attachment: timestamp.patch

Comment 28

[Andrei Oprea [:andreio]]

I've modified the field name as Mihai said.
I think this time I got the indentation right.
I've added the else branch for

>+    if (!Services.prefs.getBoolPref(ENABLE_WEBCONSOLE_TIMESTAMP)) {
>+      this.output.classList.add("hideTimestamps");
>+    }

But I don't think it will do much. If preference is false then the CSS class selector just doesn't get added. Is it for consistency sake ?

Comment 29

[Paul Rouget [:paul]]

(In reply to Andrei Oprea [:andreio] from comment #28)
> I've modified the field name as Mihai said.
> I think this time I got the indentation right.

Nope :)

Look here: https://bug722267.bugzilla.mozilla.org/attachment.cgi?id=818303

The first checkbox block should not move.

> I've added the else branch for
> 
> >+    if (!Services.prefs.getBoolPref(ENABLE_WEBCONSOLE_TIMESTAMP)) {
> >+      this.output.classList.add("hideTimestamps");
> >+    }
> 
> But I don't think it will do much. If preference is false then the CSS class
> selector just doesn't get added. Is it for consistency sake ?

for consistency sake :) … and then we don't depend on the XUL code.

Comment 30

[Paul Rouget [:paul]]

Comment on attachment 818303 [details] [diff] [review]
timestamp.patch

I think this is good. Once you fixed the indentation, can you ask Mihai to review your code? (r?)

Comment 31

[Mihai Sucan [:msucan]]

Comment on attachment 817290 [details] [diff] [review]
timestamp.patch

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

Thanks for the update, Andrei! Please remove any trailing whitespaces added by the patch. Otherwise, the patch looks good.

::: browser/app/profile/firefox.js
@@ +1208,5 @@
>  pref("devtools.webconsole.persistlog", false);
>  
> +// Web Console timestamp: |true| if you want the logs and intructions
> +// in the Web Console to display a timestamp, |false| no timestamp is 
> +// associated with the output

There's a typo in this comment. s/intructions/instructions/.

Also, maybe the following would be better:

// Web Console timestamp: |true| if you want the logs
// in the Web Console to display a timestamp, or |false| to not display
// any timestamps.

@@ +1209,5 @@
>  
> +// Web Console timestamp: |true| if you want the logs and intructions
> +// in the Web Console to display a timestamp, |false| no timestamp is 
> +// associated with the output
> +pref("devtools.webconsole.enableConsoleTimestamp", false);

s/enableConsoleTimestamp/messageTimestamp/

(I asked for this in my previous comment)

::: browser/devtools/webconsole/webconsole.js
@@ +170,5 @@
>  const MAX_LONG_STRING_LENGTH = 200000;
>  
>  const PREF_CONNECTION_TIMEOUT = "devtools.debugger.remote-timeout";
>  const PREF_PERSISTLOG = "devtools.webconsole.persistlog";
> +const ENABLE_WEBCONSOLE_TIMESTAMP = "devtools.webconsole.enableConsoleTimestamp";

s/ENABLE_WEBCONSOLE_TIMESTAMP/PREF_MESSAGE_TIMESTAMP/

(for consistency with the other pref constants)

@@ +2691,5 @@
> +  */
> +  _toggleShowTimeStamps: function WCF__toggleShowTimeStamps (event, data)
> +  {
> +    let outputContainer = this.window.document.getElementById("output-container");
> +    if (data.pref == "devtools.webconsole.enableConsoleTimestamp") {

Please use the preference constant.

Comment 32

[Andrei Oprea [:andreio]]

(In reply to Mihai Sucan [:msucan] from comment #31)
> Comment on attachment 817290 [details] [diff] [review]
> timestamp.patch
> 
> Review of attachment 817290 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the update, Andrei! Please remove any trailing whitespaces added
> by the patch. Otherwise, the patch looks good.
> 
> ::: browser/app/profile/firefox.js
> @@ +1208,5 @@
> >  pref("devtools.webconsole.persistlog", false);
> >  
> > +// Web Console timestamp: |true| if you want the logs and intructions
> > +// in the Web Console to display a timestamp, |false| no timestamp is 
> > +// associated with the output
> 
> There's a typo in this comment. s/intructions/instructions/.
> 
> Also, maybe the following would be better:
> 
> // Web Console timestamp: |true| if you want the logs
> // in the Web Console to display a timestamp, or |false| to not display
> // any timestamps.
> 
> @@ +1209,5 @@
> >  
> > +// Web Console timestamp: |true| if you want the logs and intructions
> > +// in the Web Console to display a timestamp, |false| no timestamp is 
> > +// associated with the output
> > +pref("devtools.webconsole.enableConsoleTimestamp", false);
> 
> s/enableConsoleTimestamp/messageTimestamp/
> 
> (I asked for this in my previous comment)

I forgot to invalidate my previous patch. I think I removed all occurrences in my latest patch.

> 
> ::: browser/devtools/webconsole/webconsole.js
> @@ +170,5 @@
> >  const MAX_LONG_STRING_LENGTH = 200000;
> >  
> >  const PREF_CONNECTION_TIMEOUT = "devtools.debugger.remote-timeout";
> >  const PREF_PERSISTLOG = "devtools.webconsole.persistlog";
> > +const ENABLE_WEBCONSOLE_TIMESTAMP = "devtools.webconsole.enableConsoleTimestamp";
> 
> s/ENABLE_WEBCONSOLE_TIMESTAMP/PREF_MESSAGE_TIMESTAMP/
> 
> (for consistency with the other pref constants)
> 
> @@ +2691,5 @@
> > +  */
> > +  _toggleShowTimeStamps: function WCF__toggleShowTimeStamps (event, data)
> > +  {
> > +    let outputContainer = this.window.document.getElementById("output-container");
> > +    if (data.pref == "devtools.webconsole.enableConsoleTimestamp") {
> 
> Please use the preference constant.

Thanks for the feedback. I'm working on making the changes now.

Comment 33

[Andrei Oprea [:andreio]]

Attachment: timestamp.patch

Comment 34

[Andrei Oprea [:andreio]]

Attachment: timestamp.patch

removed a newline

Comment 35

[Mihai Sucan [:msucan]]

Comment on attachment 818525 [details] [diff] [review]
timestamp.patch

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

Patch looks better now. Please address the comments below, f+ with that. Thank you!

::: browser/devtools/webconsole/webconsole.js
@@ +204,5 @@
>  
>    this._outputTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>    this._outputTimerInitialized = false;
>  
> +  // Toggle the timestamp of preferance change

s/timestamp of preferance/timestamp on preference change./

@@ +2690,5 @@
>  
>    /**
> +   * Event handler for enabling timestamps in the toolbox options
> +  */
> +  _toggleShowTimeStamps: function WCF__toggleShowTimeStamps (event, data)

nits: end comments with a period, and add a @private in the jsdoc comment. Also, for consistency with other code, please use "a" to prefix arguments.

@@ +2693,5 @@
> +  */
> +  _toggleShowTimeStamps: function WCF__toggleShowTimeStamps (event, data)
> +  {
> +    let outputContainer = this.window.document.getElementById("output-container");
> +    let enabledConsoleTimestamp = Services.prefs.getBoolPref(PREF_MESSAGE_TIMESTAMP);

Do you need to read the pref? data.newValue should hold the new pref value.

@@ +2696,5 @@
> +    let outputContainer = this.window.document.getElementById("output-container");
> +    let enabledConsoleTimestamp = Services.prefs.getBoolPref(PREF_MESSAGE_TIMESTAMP);
> +    if (data.pref == PREF_MESSAGE_TIMESTAMP) {
> +      if (enabledConsoleTimestamp) {
> +        outputContainer.classList.remove("hideTimestamps");

You do not need the new outputContainer variable. You already have the #output-container element as this.outputNode.

@@ -2820,2 @@
>      return this._destroyer.promise;
> -  },

Is this change needed?

@@ -2822,3 @@
>  };
>  
> -

Is this change needed?

Comment 36

[Andrei Oprea [:andreio]]

Attachment: timestamp.patch

Comment 37

[Andrei Oprea [:andreio]]

Attachment: timestamp.patch

aData is object not boolean

Comment 38

[Mihai Sucan [:msucan]]

Comment on attachment 819950 [details] [diff] [review]
timestamp.patch

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

This is looking good. f+ with the comments below addressed. Thank you Andrei!

Can you please write a test? You can copy one of the tests in browser/devtools/webconsole/test and change it as needed. Make sure you add the new test file name in the browser.ini file, and you rebuild Firefox. More details here:

https://developer.mozilla.org/en-US/docs/Browser_chrome_tests

::: browser/devtools/webconsole/webconsole.js
@@ +31,5 @@
>  loader.lazyImporter(this, "VariablesView", "resource:///modules/devtools/VariablesView.jsm");
>  loader.lazyImporter(this, "VariablesViewController", "resource:///modules/devtools/VariablesViewController.jsm");
>  loader.lazyImporter(this, "PluralForm", "resource://gre/modules/PluralForm.jsm");
> +loader.lazyImporter(this, "gDevTools", "resource:///modules/devtools/gDevTools.jsm");
> + 

trailing whitespace...

@@ +2688,5 @@
>      }, false);
>    },
>  
>    /**
> +   * Event handler for enabling timestamps in the toolbox options.

"Handler for the pref-changed event coming from the toolbox. Currently this function only handles the timestamps preference."

@@ +2692,5 @@
> +   * Event handler for enabling timestamps in the toolbox options.
> +   *
> +   * @private
> +   * @param object aEvent
> +   *        The timestamp message checkbox event object.

This parameter is a string that holds the event name - pref-changed in this case.

@@ +2694,5 @@
> +   * @private
> +   * @param object aEvent
> +   *        The timestamp message checkbox event object.
> +   * @param object aData
> +   *        The new value of the checkbox.

This is thee pref-changed data object.

@@ +2696,5 @@
> +   *        The timestamp message checkbox event object.
> +   * @param object aData
> +   *        The new value of the checkbox.
> +  */
> +  _toggleShowTimeStamps: function WCF__toggleShowTimeStamps (aEvent, aData)

s/_toggleShowTimeStamps/_onToolboxPrefChanged/

@@ +2700,5 @@
> +  _toggleShowTimeStamps: function WCF__toggleShowTimeStamps (aEvent, aData)
> +  {
> +    let enabledConsoleTimestamp = aData.newValue;
> +    if (aData.pref == PREF_MESSAGE_TIMESTAMP) {
> +      if (enabledConsoleTimestamp) {

s/enabledConsoleTimestamp/aData.newValue/

Comment 40

[Paul Rouget [:paul]]

Hi Andrei, any update?

Comment 41

[Andrei Oprea [:andreio]]

Some trouble writting the test file but Mihai helped out on IRC. I expect to finish tomorrow :)

Comment 42

[Andrei Oprea [:andreio]]

Attachment: timestamp.patch

Test file is not working as it should. Preference value is not returned properly and & event for change does not get triggered.

Comment 43

[Girish Sharma [:Optimizer]]

(In reply to Andrei Oprea [:andreio] from comment #42)
> Created attachment 828877 [details] [diff] [review]
> timestamp.patch
> 
> Test file is not working as it should. Preference value is not returned
> properly and & event for change does not get triggered.

Andrei, the "pref-changed" event is not a default/in built event that will automatically fire on any preference change. Some will have to do "gDevTools.emit('pref-changed', {...})" for the event listeners to work.

Comment 44

[Girish Sharma [:Optimizer]]

Dammit, I am getting old. Disregard my previous comment. Everything *should* work :)

Comment 45

[Mihai Sucan [:msucan]]

Comment on attachment 828877 [details] [diff] [review]
timestamp.patch

Andrei, thanks for the updated patch!

This patch only needs two changes:

1. if I disable timestamps and I reopen the webconsole, I still see the timestamps. You do not read the pref when the webconsole is open and you don't add the class name to the output element during init.

I believe you had a patch which did this, but somehow that code got lost or something. At least I saw pastebins that had it.

2. the test doesn't get the pref-changed event because you need to open the options panel, and click the pref checkbox.

To open the options panel do gDevTools.getToolbox(hud.target).selectTool("options").then(onFoo).

To click the checkbox in the options panel, you need to add an id to the new pref you added in toolbox-options.xul.

Comment 46

[Mihai Sucan [:msucan]]

Attachment: patch with test fixed

The suggested changes were small enough and I had to make those changes to the test file while figuring out why it fails to work.

This is the resulting patch. Pushed to try servers: https://tbpl.mozilla.org/?tree=Try&rev=e7f24a9d0071

Thanks Andrei!


r=me

Comment 47

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

schweet! Thanks for the patches Andrei!

Comment 48

[Mihai Sucan [:msucan]]

Landed: https://hg.mozilla.org/integration/fx-team/rev/419d3348fab4

Comment 49

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

https://hg.mozilla.org/mozilla-central/rev/419d3348fab4
https://hg.mozilla.org/mozilla-central/rev/947929b2bd68

Comment 50

[Alfred Kayser]

Minor nit:
This: #output-container.hideTimestamps > .message > .timestamp {
Should be: .hideTimestamps > .message > .timestamp {

No need to also specify the ID, just specifying the class is enough and is faster.