492883 - Add JavaScript Error Console to Fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: screen shot

also, it goes away if you switch between portrait and landscape modes

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

typing "window.fullScreen=true;" into the evaluator will make the console bigger. But you still lose the window when switching apps.

Nino is building an error console tool into Fennec itself.

Comment 2

[Nino D'Aversa (:ndaversa)]

Attachment: [WIP] Adding JS Console to Fennec

This patch still needs some more work for styling, but all the functionality of the console exists.

Comment 3

[Nochum Sossonko [:Natch]]

You *may* be interested in looking at bug 305206, which will hopefully land on m-c sometime, it adds some improvements to the console in Firefox.

Comment 4

[Nino D'Aversa (:ndaversa)]

Nochum,

Thanks, just took a quick look, seems like we both changed to a richlistbox, :)

Comment 5

[Philip Chee]

Heh! Console² has been using a richlistbox since the begining.

Comment 6

[Nino D'Aversa (:ndaversa)]

Attachment: Adding JS Console to Fennec v1

Fully working version with the console message/warnings/errors templating done in XBL.

Mark, let me know what you think when you got some time. Looking forward to your critique.

Thanks
Nino

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 378393 [details] [diff] [review]
Adding JS Console to Fennec v1

>diff --git a/chrome/content/bindings/console.xml b/chrome/content/bindings/console.xml

>+  <binding id="error" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>+    <content orient="vertical">      
>+      <xul:hbox class="console-row-internal-box" flex="1">
>+        <xul:hbox class="console-row-icon" align="center">
>+          <xul:image class="console-icon" xbl:inherits="src,type"/>
>+        </xul:hbox>

I wonder if we can just drop the images. We have limited space and the images are not that useful.

>+          
>+          <xul:hbox class="console-row-file" xbl:inherits="hidden=hideSource">
>+            <xul:label class="label title" value="&consoleErrFile.label;"/>
>+            <xul:hbox class="console-error-source title" xbl:inherits="href,line"/>

No special source reference handling. Let's just use a simple label here:

              <xul:label class="title" xbl:inherits="value=href" crop="right"/>

>+  <binding id="console-error-source" extends="xul:hbox">
>+    <content>
>+      <xul:label class="text-link title" xbl:inherits="href,value=href" crop="right"/>
>+    </content>
>+
>+    <handlers>
>+      <handler event="click" phase="capturing" button="0" preventdefault="true">
>+        <![CDATA[
>+          var url = this.getAttribute("href");
>+          var line = getAttribute("line");
>+          gViewSourceUtils.viewSource(url, null, null, line);
>+        ]]>
>+      </handler>
>+    </handlers>
>+  </binding>

Remove this entire binding. We will not be opening source code in popup windows.

>+  
>+  <binding id="message" extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>+    <content>
>+      <xul:hbox class="console-internal-box" flex="1">
>+        <xul:hbox class="console-row-icon" align="center">
>+          <xul:image class="console-icon" xbl:inherits="src,type"/>
>+        </xul:hbox>

Same about losing the image

>diff --git a/chrome/content/browser.css b/chrome/content/browser.css

>+.console-error-source {
>+  -moz-binding: url("chrome://browser/content/bindings/console.xml#console-error-source");

Remove this one

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul
>+  <script type="application/x-javascript" src="chrome://global/content/viewSourceUtils.js"/>

Remove this, since we are dropping the "view source in new window" thing

>+          <richlistbox id="console-box" class="console-box" flex="1"/>
>+          <stringbundle id="strBundle" src="chrome://global/locale/console.properties"/>

put the stringbundle with the other bundles higher in the file and use "console-bundle" for the id please

>diff --git a/chrome/content/console.js b/chrome/content/console.js

>+var ConsoleView = {

we like "let" instead of "var" these days. do a search on this file and convert them over.

>+  gConsole: null,
>+  gTextBoxEval: null,
>+  gEvaluator: null,
>+  gCodeToEvaluate: "",
>+  mStrBundle: null,

change these to "_list", "_evalTextbox", "_evalFrame", "_evalCode", "_bundle"

>+    this.gTextBoxEval = document.getElementById("console-textbox-eval")  
>+    this.gEvaluator = document.getElementById("console-evaluator");

maybe change id's to "console-eval-textbox" and "console-eval-frame" ?

>+    this.mCount = 0;

"_count"

>+    this.mCService = 

use "_console" instead of "mCService"

>+  showChromeErrors: function(){

add space between "function()" & "{"

>+
>+  appendItem: function cv_appendItem(aObject){

add space between "function()" & "{"

>+    try {
>+    // Try to QI it to a script error to get more info
>+    var scriptError = aObject.QueryInterface(Ci.nsIScriptError);
>+    
>+    // filter chrome urls
>+    if (!this.showChromeErrors && scriptError.sourceName.substr(0, 9) == "chrome://")
>+      return;
>+    this.appendError(scriptError);

Indent the above block

>+        if (msg.message)
>+          this.appendMessage(msg.message);
>+        else // observed a null/"clear" messagehttp://www.torrentleech.org/download.php/154043/30.Rock.S03E22.HDTV.XviD-LOL.torrent

Remove this http link

>+  appendError: function cv_appendError(aObject){

add space between "function()" & "{"

>+    if (aObject.lineNumber || aObject.sourceName) {
>+      row.setAttribute("href", aObject.sourceName);
>+      row.setAttribute("line", aObject.lineNumber);
>+    } else {

don't cuddle the "else", put it on it's own line

>+      if (aObject.columnNumber) {
>+        row.setAttribute("col", aObject.columnNumber);
>+        row.setAttribute("errorDots", this.repeatChar(" ", aObject.columnNumber));
>+        row.setAttribute("errorCaret", " ");
>+      } else {
>+        row.setAttribute("hideCaret", "true");
>+      }
>+    } else {

no cuddle (2 of them)

>+  appendMessage: function cv_appendMessage (aMessage){

add space between "function()" & "{"

>+  createConsoleRow: function cv_createConsoleRow(){

add space between "function()" & "{"

>+  appendConsoleRow: function cv_appendConsoleRow(aRow){

add space between "function()" & "{"

>+    this.gConsole.appendChild(aRow);
>+    if (++this.mCount > this.limit) this.deleteFirst();

| this.deleteFirst() | on it's own line

>+  deleteFirst: function cv_deleteFirst(){

add space between "function()" & "{"

>+  appendInitialItems: function cv_appendInitialItems(){

add space between "function()" & "{"

>+    var limit = messages.length - this.limit;
>+    if (limit < 0) limit = 0;

own line

>+  changeMode: function cv_changeMode() {
>+

remove blank line

>+    let mode = document.getElementById("console-filter").value;
>+    if (this.gConsole.getAttribute("mode") != mode){

add space between ")" & "{"

>+      let rows = this.gConsole.childNodes;
>+      for (let i=0; i < rows.length; i++){

add space between ")" & "{"

>+        let row = rows[i];
>+        if (mode == "all" || row.getAttribute ("type") == mode )

remove space between "mode" & ")"

>+  repeatChar: function cv_repeatChar(aChar, aCol){

add space between "function()" & "{"

>+  // XXX DEBUG
>+  debug: function cv_debug(aText) {
>+    var cs = Cc['@mozilla.org/consoleservice;1'].getService(Ci.nsIConsoleService);
>+    cs.logStringMessage(aText);
>+  }

Remove this function?

Comment 8

[Nino D'Aversa (:ndaversa)]

Attachment: Adding JS Console to Fennec v2

Once again, thanks for the review.

Addressed all your concerns from the previous review. Let me know if there is anything else.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 378443 [details] [diff] [review]
Adding JS Console to Fennec v2

If we decide to land this as a built-in (and not as an extension), we should move the ConsoleView.init() call to BrowserUI.init() with DownloadsView and ExtensionsView.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Madhava, we need to decide if we will ship the Error Console as part of Fennec. We can have it visible by default, or we can hide it by default. If hidden (my preference), we could enable it via a Preference for "Developer Tools" (my preference) or a hidden pref in about:config

Thoughts?

Comment 11

[Nino D'Aversa (:ndaversa)]

Attachment: Adding JS Console to Fennec v3

Removes bitrot.

Carried forward the r+ from mfinkle.

Comment 12

[Brian King [:kinger]]

Short of building Fennec yourself and applying the patch, is there another way to hook this up?

With the Mozilla/Maemo weekend upcoming and the focus on add-ons, I want to point developers to the best available tools thus far.

Comment 13

[Nino D'Aversa (:ndaversa)]

The current patch needs to be built with Fennec. However we could pull this out as an extension, however I am in favor of having it build with Fennec and having the pref to turn it on/off.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #12)
> Short of building Fennec yourself and applying the patch, is there another way
> to hook this up?
> 
> With the Mozilla/Maemo weekend upcoming and the focus on add-ons, I want to
> point developers to the best available tools thus far.

Brian, on desktop you can always launch | fennec -jsconsole | to open error console in a separate window. I do this on the n810 too. Windows Mobile doesn't handle separate windows as well. It loses them and the task manager is the only way to access the window, and that only rarely works.

Comment 15

[Brian King [:kinger]]

chrome://global/content/console.xul in the urlbar also works.

Comment 16

[Nino D'Aversa (:ndaversa)]

Attachment: Adding JS Console to Fennec v4

The JS console is now wrapped in a pref. To enable it set browser.console.showInPanel to true.

Currently I have just copied the images for download as a placeholder. These will need to be changed when Madhava and Sean get a chance to create an icon for hildon and wince.

Any other comments are welcome.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

* Replaced hildon images with the final ones (wince will get updated with the rest of the theme)
* Pulled global console.properties into browser.properties

pushed:
http://hg.mozilla.org/mobile-browser/rev/995d0b9c70fd

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Oops:
http://hg.mozilla.org/mobile-browser/rev/587224e233c7