Add JavaScript Error Console to Fennec

RESOLVED FIXED

Status

Firefox for Android Graveyard
General
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: blassey, Assigned: ndaversa)

Tracking

({uiwanted})

Trunk
ARM
Windows Mobile 6 Professional
uiwanted

Details

Attachments

(2 attachments, 4 obsolete attachments)

Created attachment 377285 [details]
screen shot

also, it goes away if you switch between portrait and landscape modes
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.
Assignee: nobody → ninodaversa
(Assignee)

Comment 2

9 years ago
Created attachment 377555 [details] [diff] [review]
[WIP] Adding JS Console to Fennec

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

Updated

9 years ago
Summary: error console is really small → Add JavaScript Error Console to Fennec
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.
(Assignee)

Comment 4

9 years ago
Nochum,

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

Comment 5

9 years ago
Heh! Console² has been using a richlistbox since the begining.
Status: NEW → ASSIGNED
(Assignee)

Comment 6

9 years ago
Created attachment 378393 [details] [diff] [review]
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
Attachment #377555 - Attachment is obsolete: true
Attachment #378393 - Flags: review?(mark.finkle)
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?
Attachment #378393 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 8

9 years ago
Created attachment 378443 [details] [diff] [review]
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.
Attachment #378393 - Attachment is obsolete: true
Attachment #378443 - Flags: review?(mark.finkle)
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.
Attachment #378443 - Flags: review?(mark.finkle) → review+
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?
Keywords: uiwanted
(Assignee)

Comment 11

9 years ago
Created attachment 379204 [details] [diff] [review]
Adding JS Console to Fennec v3

Removes bitrot.

Carried forward the r+ from mfinkle.
Attachment #378443 - Attachment is obsolete: true
Attachment #379204 - Flags: review+
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.
(Assignee)

Comment 13

9 years ago
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.
(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.
chrome://global/content/console.xul in the urlbar also works.
(Assignee)

Comment 16

9 years ago
Created attachment 380879 [details] [diff] [review]
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.
Attachment #379204 - Attachment is obsolete: true
Attachment #380879 - Flags: review?(mark.finkle)
Attachment #380879 - Flags: review?(mark.finkle) → review+
* 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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

8 years ago
Component: Windows Mobile → General
QA Contact: mobile-windows → general
Hardware: All → ARM
You need to log in before you can comment on or make changes to this bug.