Option to enable/disable timestamps for messages in the console

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Developer Tools: Console
P3
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: paul, Assigned: andreio)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Firefox 28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first verify])

Attachments

(2 attachments, 8 obsolete attachments)

(Reporter)

Description

6 years ago
Timestamps are not always useful. We need a way to toggle them.
(Reporter)

Updated

6 years ago
Depends on: 704110
Keywords: uiwanted
Stephen hopefully knows what the context here is. :)
Assignee: nobody → shorlander
Keywords: uiwanted
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 |
 +-------------+-------------------------------
(Reporter)

Comment 3

6 years ago
A "clock" icon would work I think.
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.
Assignee: shorlander → nobody
Depends on: 778766
Priority: -- → P3
Hardware: x86 → All

Updated

4 years ago
Duplicate of this bug: 892407
(Reporter)

Updated

4 years ago
Assignee: nobody → paul
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.
(Reporter)

Updated

4 years ago
Depends on: 760876
(Reporter)

Comment 7

4 years ago
Is it ok if we should the timestemps only if a checkbox is checked in the option panel?
(Reporter)

Comment 8

4 years ago
s/should/show
(Reporter)

Updated

4 years ago
Assignee: paul → andrei.br92
(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.
(Assignee)

Comment 10

4 years ago
I am planning to add it in the Devtools Toolbox Option under Web Console
(Assignee)

Comment 11

4 years ago
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.
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!

Updated

4 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 13

4 years ago
(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)?
(Assignee)

Comment 14

4 years ago
Created attachment 813629 [details] [diff] [review]
timestamp.patch
Attachment #813629 - Flags: feedback?(paul)
(Reporter)

Comment 15

4 years ago
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?
Attachment #813629 - Flags: feedback?(paul) → feedback?(mihai.sucan)
(Assignee)

Comment 16

4 years ago
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 on attachment 813629 [details] [diff] [review]
timestamp.patch

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

Thanks!
Attachment #813629 - Flags: feedback?(mihai.sucan)
(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.
(Assignee)

Comment 19

4 years ago
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.
(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().
(Assignee)

Comment 21

4 years ago
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 ?
Attachment #813629 - Attachment is obsolete: true
Attachment #815125 - Flags: feedback?(mihai.sucan)
(Reporter)

Comment 22

4 years ago
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.
Attachment #815125 - Flags: feedback?(mihai.sucan)
(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.
(Assignee)

Comment 24

4 years ago
Created attachment 817290 [details] [diff] [review]
timestamp.patch

The new default is "off" :)

When you toggle you hide/show all of the timestamps in the Web Console.
Attachment #815125 - Attachment is obsolete: true
Attachment #817290 - Flags: feedback?(paul)
Attachment #817290 - Flags: feedback?(mihai.sucan)
(Assignee)

Comment 25

4 years ago
(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.
(Reporter)

Comment 26

4 years ago
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");
}
Attachment #817290 - Flags: feedback?(paul)
(Assignee)

Comment 27

4 years ago
Created attachment 818303 [details] [diff] [review]
timestamp.patch
Attachment #818303 - Flags: feedback?
(Assignee)

Comment 28

4 years ago
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 ?
(Reporter)

Comment 29

4 years ago
(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.
(Reporter)

Updated

4 years ago
Attachment #817290 - Flags: feedback?(mihai.sucan)
(Reporter)

Comment 30

4 years ago
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?)
Attachment #818303 - Flags: feedback? → feedback+
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.
(Assignee)

Comment 32

4 years ago
(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.
(Assignee)

Comment 33

4 years ago
Created attachment 818522 [details] [diff] [review]
timestamp.patch
Attachment #817290 - Attachment is obsolete: true
Attachment #818303 - Attachment is obsolete: true
Attachment #818522 - Flags: feedback?(mihai.sucan)
(Assignee)

Comment 34

4 years ago
Created attachment 818525 [details] [diff] [review]
timestamp.patch

removed a newline
Attachment #818522 - Attachment is obsolete: true
Attachment #818522 - Flags: feedback?(mihai.sucan)
Attachment #818525 - Flags: feedback?(mihai.sucan)
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?
Attachment #818525 - Flags: feedback?(mihai.sucan) → feedback+
(Assignee)

Comment 36

4 years ago
Created attachment 819949 [details] [diff] [review]
timestamp.patch
Attachment #818525 - Attachment is obsolete: true
Attachment #819949 - Flags: feedback?(mihai.sucan)
(Assignee)

Comment 37

4 years ago
Created attachment 819950 [details] [diff] [review]
timestamp.patch

aData is object not boolean
Attachment #819949 - Attachment is obsolete: true
Attachment #819949 - Flags: feedback?(mihai.sucan)
Attachment #819950 - Flags: feedback?(mihai.sucan)
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/
Attachment #819950 - Flags: feedback?(mihai.sucan) → feedback+

Updated

4 years ago
Duplicate of this bug: 909750
Blocks: 909803
(Reporter)

Comment 40

4 years ago
Hi Andrei, any update?
(Assignee)

Comment 41

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

Comment 42

4 years ago
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.
Attachment #819950 - Attachment is obsolete: true
Attachment #828877 - Flags: feedback?(mihai.sucan)
(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.
Dammit, I am getting old. Disregard my previous comment. Everything *should* work :)
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.
Attachment #828877 - Flags: feedback?(mihai.sucan)

Updated

4 years ago
Summary: The timestamps should be expandable → Option to enable/disable timestamps for messages in the console
Created attachment 830253 [details] [diff] [review]
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
Attachment #830253 - Flags: review+
schweet! Thanks for the patches Andrei!
Landed: https://hg.mozilla.org/integration/fx-team/rev/419d3348fab4
Whiteboard: [fixed-in-fx-team]

Updated

4 years ago
Attachment #830253 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/419d3348fab4
https://hg.mozilla.org/mozilla-central/rev/947929b2bd68
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28

Comment 50

4 years ago
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.

Updated

4 years ago
Depends on: 955820

Updated

4 years ago
Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.