Closed Bug 593540 Opened 14 years ago Closed 11 years ago

Don't display content errors in the Error Console

Categories

(Toolkit Graveyard :: Error Console, defect, P1)

Product:
Toolkit Graveyard
Component:
Error Console
Type:
defect

Tracking

(blocking2.0 -)

Status:
RESOLVED WONTFIX
Tracking Flags:
Tracking Status
blocking2.0 --- -

People

(Reporter: dangoor, Unassigned)

References

Details

Attachments

(3 files, 7 obsolete files)

[obsolete] Error Console Show Content v4
6.08 KB, patch
mossop
: review+
Details | Diff | Splinter Review
Error Console Show Content v5
5.46 KB, patch
Details | Diff | Splinter Review
Error Console Show Content Test 4
21.22 KB, patch
mossop
: review+
Details | Diff | Splinter Review 
In bug 593538, we are hiding the Error Console and changing its function to be a "Chrome Error Console". We should therefore not display content errors in this console, since the content errors will be appearing in the new Web Console.
This doesn't need to be done right away, but we should get this in Firefox 4 so that the content error messages are not appearing in multiple places.
blocking2.0: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
Blocks: devtools4b8
The error console lives in the toolkit - changing its behavior across the board would affect all users. We could add a pref that let's the what-to-show behavior be app-customizable and only change it in Firefox, though.
Assignee: nobody → rcampbell
blocking2.0: ? → betaN+
Should be very straightforward, add a pref and check for it here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/content/consoleBindings.xml#146

So instead of:
if (!this.showChromeErrors && scriptError.sourceName.substr(0, 9) == "chrome://")

You'd do something like this:

if (scriptError.sourceName.substr(0, 9) != "chrome://") {
  if (this.showOnlyChromeErrors) {
    return;
  }
}
else if (!this.showOnlyChromeErrors && !this.showChromeErrors) {
  return;
}

That should catch all the scenarios (showOnlyChromeErrors should override the showChromeErrors pref).
Thanks Nochum. You've all but written this for me. :)
Just noticed this was assigned to me, adding myself to the CC list.
Status: NEW → ASSIGNED
Attached patch Error Console Show Only Chrome (obsolete) — — Splinter Review 
basic patch to implement showOnlyChromeErrors in Error Console.
Attachment #482911 - Flags: review?(dtownsend)
Attached patch Error Console Show Only Chrome (obsolete) — — Splinter Review 
Did a little verification and found the logic was not quite right. The three states where we should exit out of the function are:

1. !showOnlyChrome + !showErrors + !chromeURL
2. showOnlyChrome + !showErrors + !chromeURL
3. showOnlyChrome + showErrors + !chromeURL

cases 2 and 3 can be combined by removing the check for showErrors, leaving only 2 states that we care about.

Added a comment to explain the checks.
Attachment #482911 - Attachment is obsolete: true
Attachment #482938 - Flags: review?(dtownsend)
Attachment #482911 - Flags: review?(dtownsend)
and, I copied my logic incorrectly.

The first state should be:
1. !showOnlyChrome + !showErrors + chromeURL
Comment on attachment 482938 [details] [diff] [review]
Error Console Show Only Chrome

I think it's the wrong choice to have a pref that shows only chrome errors (especially when it confusingly overrides the pref to show chrome errors). A pref that turns on/off content errors would seem to make more sense. The existing code is also broken in that it assumes only errors from "chrome://" urls are "chrome" errors when in fact it needs to take into account more (about:, resource:, file:). Unfortunately some of those can be either chrome or content which makes for some interesting results.

How were you thinking of testing this?

>       <method name="appendItem">
>         <parameter name="aObject"/>
>         <body><![CDATA[
>           try {
>             // Try to QI it to a script error to get more info
>             var scriptError = aObject.QueryInterface(Components.interfaces.nsIScriptError);
>             
>             // filter chrome urls
>-            if (!this.showChromeErrors && scriptError.sourceName.substr(0, 9) == "chrome://")
>+            // Two states where we bailout:
>+            // 1. chromeUrl + not showOnlyChromeErrors + not showChrome
>+            // 2. not chromeUrl + showOnlyChromeErrors

Put these comments alongside the actual code implementing them and use words to describe them rather than + (which in my engineering head translates to ||)

>+            
>+            var chromeUrl = scriptError.sourceName.substr(0, 9) == "chrome://";
>+
>+            if (chromeUrl && !this.showOnlyChromeErrors && !this.showChromeErrors) {
>               return;
>- 
>+            }

Nit: No braces around the single statement

>+
>+            if (!chromeUrl && this.showOnlyChromeErrors) {
>+              return;
>+            }

Nit: No braces around the single statement
Attachment #482938 - Flags: review?(dtownsend) → review-
(In reply to comment #9)
> Comment on attachment 482938 [details] [diff] [review]
> Error Console Show Only Chrome
> 
> I think it's the wrong choice to have a pref that shows only chrome errors
> (especially when it confusingly overrides the pref to show chrome errors). A
> pref that turns on/off content errors would seem to make more sense.

It doesn't really override, it supercedes. Merely an accident of logic.

Isn't adding a pref to turn off content changing the default behavior of the Error Console? With the above patch, it should behave the same way for other consumers.

> The
> existing code is also broken in that it assumes only errors from "chrome://"
> urls are "chrome" errors when in fact it needs to take into account more
> (about:, resource:, file:). Unfortunately some of those can be either chrome or
> content which makes for some interesting results.

Ah, that's a good point. I was lifting the current mechanism's chrome uri detection bit. Might need an additional method to determine if the source is really chrome or not.

> How were you thinking of testing this?

There aren't any tests in the Error Console directory now and was hoping this was going to be a simple-enough case to avoid it. That's becoming less clear now. We'll probably need to add a tests directory to this and write something up.

Do we have a mechanism to perform "restartable" unittests? Like, if I flip a pref and need to restart, how do we test that? I suppose I could just set the variables manually to fake it, but that seems like cheating a bit.

> >       <method name="appendItem">
> >         <parameter name="aObject"/>
> >         <body><![CDATA[
> >           try {
> >             // Try to QI it to a script error to get more info
> >             var scriptError = aObject.QueryInterface(Components.interfaces.nsIScriptError);
> >             
> >             // filter chrome urls
> >-            if (!this.showChromeErrors && scriptError.sourceName.substr(0, 9) == "chrome://")
> >+            // Two states where we bailout:
> >+            // 1. chromeUrl + not showOnlyChromeErrors + not showChrome
> >+            // 2. not chromeUrl + showOnlyChromeErrors
> 
> Put these comments alongside the actual code implementing them and use words to
> describe them rather than + (which in my engineering head translates to ||)

Yeah, I wasn't sure if I should put ⋀ in there. Maybe an ampersand?

> >+            var chromeUrl = scriptError.sourceName.substr(0, 9) == "chrome://";
> >+
> >+            if (chromeUrl && !this.showOnlyChromeErrors && !this.showChromeErrors) {
> >               return;
> >- 
> >+            }
> 
> Nit: No braces around the single statement

OK

> >+
> >+            if (!chromeUrl && this.showOnlyChromeErrors) {
> >+              return;
> >+            }
> 
> Nit: No braces around the single statement
(In reply to comment #10)
> Do we have a mechanism to perform "restartable" unittests? Like, if I flip a
> pref and need to restart, how do we test that? I suppose I could just set the
> variables manually to fake it, but that seems like cheating a bit.

Why do you have to restart the app, just restarting the error console (i.e. close and reopen) should work.

Ftr, there are some tests for error console stuff in bug 490499 that *might* help you (I say that because the tests were written over some other patches).
(In reply to comment #11)
> (In reply to comment #10)
> > Do we have a mechanism to perform "restartable" unittests? Like, if I flip a
> > pref and need to restart, how do we test that? I suppose I could just set the
> > variables manually to fake it, but that seems like cheating a bit.
> 
> Why do you have to restart the app, just restarting the error console (i.e.
> close and reopen) should work.

I figured they got set once and lived for the rest of the lifetime of browser. I'll check that out, thanks!

> Ftr, there are some tests for error console stuff in bug 490499 that *might*
> help you (I say that because the tests were written over some other patches).

That is also helpful. Thanks a bunch for the pointers!
Comment on attachment 482938 [details] [diff] [review]
Error Console Show Only Chrome

>diff --git a/toolkit/components/console/content/consoleBindings.xml b/toolkit/components/console/content/consoleBindings.xml
>+          var pref = Components.classes['@mozilla.org/preferences-service;1'].getService();
>+          pref = pref.QueryInterface(Components.interfaces.nsIPrefBranch);

You can do Components.classes['@mozilla.org/preferences-service;1'].getService(Components.interfaces.nsIPrefBranch); and then you don't need the separate QI.
Attached patch Error Console Show Content (obsolete) — — Splinter Review 
Flipped the logic to require either showChrome or showContent based on prefs. Added a pref (toolkit.errorconsole.showContent) to set this and made it true in prefs.js and false in firefox.js.

Testcode to follow.
Attachment #482938 - Attachment is obsolete: true
Attachment #485286 - Flags: review?(dtownsend)
Comment on attachment 485286 [details] [diff] [review]
Error Console Show Content

>diff --git a/toolkit/components/console/content/consoleBindings.xml b/toolkit/components/console/content/consoleBindings.xml
>--- a/toolkit/components/console/content/consoleBindings.xml
>+++ b/toolkit/components/console/content/consoleBindings.xml
>@@ -30,17 +30,35 @@
> 
>           try {
>             return this._showChromeErrors = pref.getBoolPref("javascript.options.showInConsole");
>           }
>           catch(ex) {
>             return this._showChromeErrors = false;
>           }
>         ]]></getter>
>-      </property>          
>+      </property>
>+
>+      <field name="_showContent">-1</field>
>+
>+      <property name="showContent">
>+        <getter><![CDATA[
>+          if (this._showContent != -1)
>+            return this._showContent;
>+          var pref = Components.classes['@mozilla.org/preferences-service;1'].getService();
>+          pref = pref.QueryInterface(Components.interfaces.nsIPrefBranch);
>+
>+          try {
>+            return this._showContent = pref.getBoolPref("toolkit.errorconsole.showContent");
>+          }
>+          catch(ex) {
>+            return this._showContent = false;

This needs to be true, content errors should be shown by default. We can't change that behavior.

>       <method name="appendItem">
>         <parameter name="aObject"/>
>         <body><![CDATA[
>           try {
>             // Try to QI it to a script error to get more info
>             var scriptError = aObject.QueryInterface(Components.interfaces.nsIScriptError);
>+            var show = false;
>             
>             // filter chrome urls
>-            if (!this.showChromeErrors && scriptError.sourceName.substr(0, 9) == "chrome://")
>-              return;
>- 
>-            this.appendError(scriptError);
>+            if (this.showChromeErrors && scriptError.sourceName.substr(0, 9) == "chrome://")
>+              show = true;
>+
>+            if (this.showContent)
>+              show = true;

So, if it's a chrome error, and this.showChromeErrors is false but this.showContent is true, it will still show!?
Attachment #485286 - Flags: feedback-
Whiteboard: [kd4b7] → [kd4b8]
you're right, that's not going to work...
I think the logic should be changed to:

if (scriptError.sourceName.substr(0, 9) == "chrome://") {
  if (this.showChromeErrors)
    show = true;
}
else if (this.showContent) {
  show = true;
}

The funny thing that comes out of this is: it's possible to turn off the error console entirely now :).
(In reply to comment #17)
> I think the logic should be changed to:
> 
> if (scriptError.sourceName.substr(0, 9) == "chrome://") {
>   if (this.showChromeErrors)
>     show = true;
> }
> else if (this.showContent) {
>   show = true;
> }

That's closer to what I'm doing, though I'm looking at nsIScriptingError to see what the category field looks like. Parsing the sourceName with regexes for chrome|resource|about|file is proving to be unreliable.

> The funny thing that comes out of this is: it's possible to turn off the error
> console entirely now :).

Kind of. There are still messages being pumped through nsIConsoleService, but not much. :)
The biggest problem you'll probably face is bug 510242 for nsIXPCExceptions. You can't get the sourceName from those, so you have to manually extract the source...
Oh, and see bug 510138 too. Simon mentions an interim solution in one of the comments.
so it looks like scriptError.sourceName is returning empty strings for a bunch of chrome-originating errors. Possibly all of them?

The category field is more than just "content|component|xul javascript" as suggested in:

http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/idl/nsIScriptError.idl#78
(In reply to comment #19)
> The biggest problem you'll probably face is bug 510242 for nsIXPCExceptions.
> You can't get the sourceName from those, so you have to manually extract the
> source...

that explains it...
why do I feel like you've been down this road before?
I apologize, I once tried revamping the whole error console, a - z, but got stuck in the middle and then had no time to complete it. If/when I have some useful information to share, I try to pass it around. Let me know if I'm getting annoying ;)
Not at all. I appreciate the pointers! :)
Comment on attachment 485286 [details] [diff] [review]
Error Console Show Content

Looks like Natch has given all the comments I was about to, waiting for a new patch
Attachment #485286 - Flags: review?(dtownsend)
Attached patch Error Console Show Content v3 (obsolete) — — Splinter Review 
Updated version. Tried incorporating as much of Nochum's suggestions as I could. I can still think of two cases that I'm not catching:
1) if either the scriptError.sourceName is a local file 
2) scriptError.message string includes location: "local file name"

These messages don't include a file:// protocol prefixing them.

Also, still needs a unittest!

This has turned into a surprisingly tricky bug.
Attachment #485286 - Attachment is obsolete: true
Attachment #485348 - Flags: review?(dtownsend)
Attachment #485348 - Flags: feedback?(highmind63)
Comment on attachment 485348 [details] [diff] [review]
Error Console Show Content v3

>diff --git a/toolkit/components/console/content/consoleBindings.xml b/toolkit/components/console/content/consoleBindings.xml
>--- a/toolkit/components/console/content/consoleBindings.xml
>+++ b/toolkit/components/console/content/consoleBindings.xml
>+          var pref = Components.classes['@mozilla.org/preferences-service;1'].getService();
>+          pref = pref.QueryInterface(Components.interfaces.nsIPrefBranch);

Again, you *can* do this in one shot, up to you :)

>+            // is the category in the set of chrome errors?
>+            if (scriptError.category.toLowerCase() in chromeCategories)

I don't think this works. You have to do chromeCategories.indexOf(scriptError.category.toLowerCase()).

The rest looks good though. feedback- only  because of the check error above.

Fyi, anything error[-console]-related is a pita unfortunately.
Attachment #485348 - Flags: feedback?(highmind63) → feedback-
Whiteboard: [kd4b8]
Comment on attachment 485348 [details] [diff] [review]
Error Console Show Content v3

r- for the same reason as Natch, but otherwise this looks good
Attachment #485348 - Flags: review?(dtownsend) → review-
Updated to include the one-shot preference fetch (I forgot about that in my weak attempts to get the logic right).

Fixed check for scriptError.category lookup.

Now to look at some test code...
Attachment #485348 - Attachment is obsolete: true
Attachment #486093 - Flags: review?(dtownsend)
Comment on attachment 486093 [details] [diff] [review]
[obsolete] Error Console Show Content v4

WINNER!
Attachment #486093 - Flags: review?(dtownsend) → review+
hehe. Awesome! Thanks for the r+.

I'm still figuring out how best to write some test code for this. To do it well could be pretty difficult.

Looking at Nochum's test code from bug 490499 got me started, but to really test this, I'll need some content pages that throw errors as well as some faulty browser code to throw some chrome errors.

We'll see what I can come up with.
(In reply to comment #33)
> Looking at Nochum's test code from bug 490499 got me started, but to really
> test this, I'll need some content pages that throw errors as well as some
> faulty browser code to throw some chrome errors.

Why? To test just this specific addition to the error console it should be sufficient to call Components.utils.reportError or (in content code) throw "Test Message" or some such. Then check if the error console created messages for these errors.
Attached patch Error Console Show Content Test (obsolete) — — Splinter Review 
After bashing my head against this test code for a couple of days, I have something ugly that's on the verge of working.

Right now it's failing to detect the "TestError" row in the console. I'd love a sleep function but don't think that writing a while loop that tests for date.now > some delay is the right way to do it.

I also still have to test for content errors, but since this is a toolkit test, I don't think I can rely on being able to open a content page with a browser window.

Suggestions welcome.
(In reply to comment #35)
> Created attachment 487997 [details] [diff] [review]
> Error Console Show Content Test

First of all, nice!! :)

> Right now it's failing to detect the "TestError" row in the console.

Hmm, I used mutation listeners for the error console and it worked (although it was pretty hairy at best). I didn't look __too__ closely at the patch, but it seems you are using them too...

> I also still have to test for content errors, but since this is a toolkit test,
> I don't think I can rely on being able to open a content page with a browser
> window.
> 

You can. There will always be a browser window, all of the toolkit tests rely on that. This one does too (it calls window.open, window is a browser window...).
(In reply to comment #36)
> (In reply to comment #35)
> > Created attachment 487997 [details] [diff] [review] [details]
> > Error Console Show Content Test
> 
> First of all, nice!! :)

I used your test as a starting point, so it should look at least somewhat familiar (and left you in you the license headers, you've been a huge help).

Right now this test feels a little kludgey. I think I'll be able to clean it up to get it working properly.

> > Right now it's failing to detect the "TestError" row in the console.
> 
> Hmm, I used mutation listeners for the error console and it worked (although it
> was pretty hairy at best). I didn't look __too__ closely at the patch, but it
> seems you are using them too...

I am, I've got a DOMSubtreeModified listener on the console's output vbox. It works, I seem to be getting notified on addition. It might just be a problem with the containsInConsole function.

> > I also still have to test for content errors, but since this is a toolkit test,
> > I don't think I can rely on being able to open a content page with a browser
> > window.
> 
> You can. There will always be a browser window, all of the toolkit tests rely
> on that. This one does too (it calls window.open, window is a browser
> window...).

Yeah, I realized this last night while thinking about it. This bug is haunting my sleep. :)
wip, v2. Now catching the chrome error in the console. Next up, some content tests and we'll be done.

warning: ugly test is ugly.
Attachment #487997 - Attachment is obsolete: true
Blocks: devtools4
Priority: -- → P1
removed bad preference

        Attachment #490974 -
        Attachment is obsolete: true

        Attachment #492853 -
        Flags: review?(dtownsend)

        Attachment #486093 -
        Attachment description: Error Console Show Content v4 → [obsolete] Error Console Show Content v4

    
    

    
  
Comment on attachment 492853 [details] [diff] [review]
Error Console Show Content Test 3

>diff --git a/toolkit/components/console/tests/Makefile.in b/toolkit/components/console/tests/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/console/tests/Makefile.in
>@@ -0,0 +1,53 @@
>+#
>+# ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+#
>+# The contents of this file are subject to the Mozilla Public License Version
>+# 1.1 (the "License"); you may not use this file except in compliance with
>+# the License. You may obtain a copy of the License at
>+# http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS IS" basis,
>+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+# for the specific language governing rights and limitations under the
>+# License.
>+#
>+# The Original Code is Mozilla code.
>+#
>+# The Initial Developer of the Original Code is
>+# Mozilla Corporation.

You want "the Mozilla Foundation."

>+# Portions created by the Initial Developer are Copyright (C) 2008

Woo, we have a full 2 years till we plan to ship Firefox 4 now?

>diff --git a/toolkit/components/console/tests/browser/Makefile.in b/toolkit/components/console/tests/browser/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/console/tests/browser/Makefile.in
>@@ -0,0 +1,61 @@
>+#
>+# ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+#
>+# The contents of this file are subject to the Mozilla Public License Version
>+# 1.1 (the "License"); you may not use this file except in compliance with
>+# the License. You may obtain a copy of the License at
>+# http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS IS" basis,
>+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+# for the specific language governing rights and limitations under the
>+# License.
>+#
>+# The Original Code is Error Console Tests.
>+#
>+# The Initial Developer of the Original Code is
>+# Mozilla Corporation.

Ditto

>+# Portions created by the Initial Developer are Copyright (C) 2008

Maybe my watch is broken

>diff --git a/toolkit/components/console/tests/browser/browser_errorConsoleContent.js b/toolkit/components/console/tests/browser/browser_errorConsoleContent.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/console/tests/browser/browser_errorConsoleContent.js
>@@ -0,0 +1,135 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Error Console Tests.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * The Mozilla Foundation.

\o/

>+ * Portions created by the Initial Developer are Copyright (C) 2008

:(

>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Nochum Sossonko <nsossonko@hotmail.com>
>+ *   Rob Campbell <rcampbell@mozilla.com>
>+ *   David Dahl <ddahl@mozilla.com>
>+ *   Patrick Walton <pcwalton@mozilla.com>
>+ *   Julian Viereck <jviereck@mozilla.com>
>+ *   Mihai Sucan <mihai.sucan@gmail.com>

That's a lot of contributors for a new file, is it accurate or just copied?

>+function addTab(aURL)
>+{
>+  gBrowser.selectedTab = gBrowser.addTab();
>+  content.location = aURL;
>+  tab = gBrowser.selectedTab;
>+  browser = gBrowser.getBrowserForTab(tab);

Both tab and browser are undeclared, but then you don't seem to need the result anyway so just drop the two lines

>+function onLoad(aEvent) {
>+  browser.removeEventListener(aEvent.type, arguments.callee, true);
>+  var button = content.document.querySelector("button").wrappedJSObject;
>+  var clickEvent = content.document.createEvent("MouseEvents");
>+  clickEvent.initMouseEvent("click", true, true,
>+    content, 0, 0, 0, 0, 0, false, false,
>+    false, false, 0, null);

Any reason not to just use EventUtils.synthesizeMouse ?

>+function testFunction(aSubject, aTopic, aObject)
>+{
>+  ok(true, "in testFunction");

Use info for these sorts of messages

>+  var win = aSubject.QueryInterface(Ci.nsIDOMEventTarget);
>+  ok(win, "we have a window");

This test is kind of pointless here since the line above will throw an exception if aSubject is null or not an nsIDOMEventTarget.

>+  win.addEventListener("load", function ECT_onLoad() {
>+    win.removeEventListener("load", ECT_onLoad, false);
>+    if (win.location != ERROR_CONSOLE_URI) {
>+      ok(false, "not our window, returning early");
>+      return;
>+    }
>+
>+    Services.ww.unregisterNotification(testFunction, "domwindowopened", false);

Move this to outside the event listener to ensure it can't receive a notification again.

>+    ok(true, "window opened");
>+
>+    var showContent = Services.prefs.getBoolPref("devtools.errorconsole.showContent");
>+    if (showContent)
>+      ok(true, "devtools.errorconsole.showContent = true");
>+    else
>+      ok(true, "devtools.errorconsole.showContent = false");

Should one of these be a failure? If not then just do a single info call.

This test should flip the pref and re-open the window to make sure that the error shows/doesn't show as appropriate.

>diff --git a/toolkit/components/console/tests/chrome/Makefile.in b/toolkit/components/console/tests/chrome/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/console/tests/chrome/Makefile.in
>@@ -0,0 +1,55 @@
>+#
>+# ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+#
>+# The contents of this file are subject to the Mozilla Public License Version
>+# 1.1 (the "License"); you may not use this file except in compliance with
>+# the License. You may obtain a copy of the License at
>+# http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS IS" basis,
>+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+# for the specific language governing rights and limitations under the
>+# License.
>+#
>+# The Original Code is Error Console Tests.
>+#
>+# The Initial Developer of the Original Code is
>+# Mozilla Corporation.

*sigh*

>+# Portions created by the Initial Developer are Copyright (C) 2008

I feel all young

>diff --git a/toolkit/components/console/tests/chrome/test_errorConsole.js b/toolkit/components/console/tests/chrome/test_errorConsole.js

What is the point of this test? Why not just add a syntax error into your browser chrome test above and verify it does/doesn't appear?

        Attachment #492853 -
        Flags: review?(dtownsend) → review-

    
    

    
  


  
(In reply to comment #41)
> Comment on attachment 492853 [details] [diff] [review]
> Error Console Show Content Test 3
> 
> >diff --git a/toolkit/components/console/tests/Makefile.in b/toolkit/components/console/tests/Makefile.in
> >new file mode 100644
> >--- /dev/null
> >+++ b/toolkit/components/console/tests/Makefile.in
[...]
> >+# The Initial Developer of the Original Code is
> >+# Mozilla Corporation.
> 
> You want "the Mozilla Foundation."
> 
> >+# Portions created by the Initial Developer are Copyright (C) 2008
> 
> Woo, we have a full 2 years till we plan to ship Firefox 4 now?

Two years since that file was initially written anyway. Updated to reflect the state of my calendar and added the proper Initial Developer.

Updated the Original Code description to Error Console Test code.

> >diff --git a/toolkit/components/console/tests/browser/Makefile.in b/toolkit/components/console/tests/browser/Makefile.in
> >new file mode 100644
> >--- /dev/null
> >+++ b/toolkit/components/console/tests/browser/Makefile.in
> >@@ -0,0 +1,61 @@
> >+#
> >+# ***** BEGIN LICENSE BLOCK *****
[...]
> >+# The Original Code is Error Console Tests.
> >+#
> >+# The Initial Developer of the Original Code is
> >+# Mozilla Corporation.
> 
> Ditto
> 
> >+# Portions created by the Initial Developer are Copyright (C) 2008
> 
> Maybe my watch is broken

Maybe you fell into a ... Hot Tub Time Machine! (I'm sure this reference will make zero sense in another 2 years.

> >diff --git a/toolkit/components/console/tests/browser/browser_errorConsoleContent.js b/toolkit/components/console/tests/browser/browser_errorConsoleContent.js
> >new file mode 100644
> >--- /dev/null
> >+++ b/toolkit/components/console/tests/browser/browser_errorConsoleContent.js
> >@@ -0,0 +1,135 @@
> >+/* ***** BEGIN LICENSE BLOCK *****
> >+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
> >+ *
[...]
> >+ * The Initial Developer of the Original Code is
> >+ * The Mozilla Foundation.
> 
> \o/
> 
> >+ * Portions created by the Initial Developer are Copyright (C) 2008
> 
> :(

I'm just gonna convert that whole thing to PD.

> >+ * the Initial Developer. All Rights Reserved.
> >+ *
> >+ * Contributor(s):
> >+ *   Nochum Sossonko <nsossonko@hotmail.com>
> >+ *   Rob Campbell <rcampbell@mozilla.com>
> >+ *   David Dahl <ddahl@mozilla.com>
> >+ *   Patrick Walton <pcwalton@mozilla.com>
> >+ *   Julian Viereck <jviereck@mozilla.com>
> >+ *   Mihai Sucan <mihai.sucan@gmail.com>
> 
> That's a lot of contributors for a new file, is it accurate or just copied?

Copied. Removed.

> 
> >+function addTab(aURL)
> >+{
> >+  gBrowser.selectedTab = gBrowser.addTab();
> >+  content.location = aURL;
> >+  tab = gBrowser.selectedTab;
> >+  browser = gBrowser.getBrowserForTab(tab);
> 
> Both tab and browser are undeclared, but then you don't seem to need the result
> anyway so just drop the two lines

Whups. Using browser and tab in test() after addTab is called. I'm gonna just combine those and get rid of addTab.

browser sticks around so I declared it.

> >+function onLoad(aEvent) {
> >+  browser.removeEventListener(aEvent.type, arguments.callee, true);
> >+  var button = content.document.querySelector("button").wrappedJSObject;
> >+  var clickEvent = content.document.createEvent("MouseEvents");
> >+  clickEvent.initMouseEvent("click", true, true,
> >+    content, 0, 0, 0, 0, 0, false, false,
> >+    false, false, 0, null);
> 
> Any reason not to just use EventUtils.synthesizeMouse ?

Not a good reason! Changed.

> >+function testFunction(aSubject, aTopic, aObject)
> >+{
> >+  ok(true, "in testFunction");
> 
> Use info for these sorts of messages

I missed that one.

> >+  var win = aSubject.QueryInterface(Ci.nsIDOMEventTarget);
> >+  ok(win, "we have a window");
> 
> This test is kind of pointless here since the line above will throw an
> exception if aSubject is null or not an nsIDOMEventTarget.

Yeah, more debugging code really. Removed.

> >+  win.addEventListener("load", function ECT_onLoad() {
> >+    win.removeEventListener("load", ECT_onLoad, false);
> >+    if (win.location != ERROR_CONSOLE_URI) {
> >+      ok(false, "not our window, returning early");
> >+      return;
> >+    }
> >+
> >+    Services.ww.unregisterNotification(testFunction, "domwindowopened", false);
> 
> Move this to outside the event listener to ensure it can't receive a
> notification again.

I put the unregisterNotification inside the load listener because I didn't want to remove it if we were in the wrong type of window. IOW, I only remove the event listener of the Error Console is opened and apparently win.location is only populated on load (or probably DOMContentLoaded).

I'll document this in the test so it's clearer.

> >+    ok(true, "window opened");
> >+
> >+    var showContent = Services.prefs.getBoolPref("devtools.errorconsole.showContent");
> >+    if (showContent)
> >+      ok(true, "devtools.errorconsole.showContent = true");
> >+    else
> >+      ok(true, "devtools.errorconsole.showContent = false");
> 
> Should one of these be a failure? If not then just do a single info call.

Not really, since I wasn't sure what the state of devtools.errorconsole.showContent was going to be in the test case, and didn't really care, I figured we could just check that it's ok. I'll make it an info call.

> This test should flip the pref and re-open the window to make sure that the
> error shows/doesn't show as appropriate.

yeah, that should be do-able without having to change anything other than clearing the error console at the end of testFunction.

(some time later)

Ok, played around with this and I have it testing both cases now.

> >diff --git a/toolkit/components/console/tests/chrome/Makefile.in b/toolkit/components/console/tests/chrome/Makefile.in
> >new file mode 100644
> >--- /dev/null
> >+++ b/toolkit/components/console/tests/chrome/Makefile.in
> >@@ -0,0 +1,55 @@
> >+#
> >+# ***** BEGIN LICENSE BLOCK *****
> >+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
> >+#
[...]
> >+# The Original Code is Error Console Tests.
> >+#
> >+# The Initial Developer of the Original Code is
> >+# Mozilla Corporation.
> 
> *sigh*

i sorry. ._. (fixed)

> >+# Portions created by the Initial Developer are Copyright (C) 2008
> 
> I feel all young

you're very spry!

> >diff --git a/toolkit/components/console/tests/chrome/test_errorConsole.js b/toolkit/components/console/tests/chrome/test_errorConsole.js
> 
> What is the point of this test? Why not just add a syntax error into your
> browser chrome test above and verify it does/doesn't appear?

The point of this test is to verify that errors from Chrome space appear. In this case, I'm generating one explicitly with Cu.reportError();

If I add a syntax error to my browser chrome test above, won't the test just fail in the middle? That'll make it hard for it to do further testing. I can document it so it's more understandable.

Anyway, I updated the license header on this. If you'd rather I just added another test to browser-chrome, I can do that instead. Better yet, I could just remove it altogether...

        Attachment #492853 -
        Attachment is obsolete: true

        Attachment #496417 -
        Flags: review?(dtownsend)

    
    

    
  
Given bug 602006, why would this block the release?
Severity: blocker → normal

    
  
Product: Firefox → Toolkit
QA Contact: general → general

    
    

    
  
(In reply to comment #43)
Just because the Error Console is going to be off for the regular user doesn't mean we shouldn't expect it to be fully functional as intended for the final release for those who would need it. If anything related to the Web (error) Console blocks then in my opinion a (chrome) Error Console should block too.

    
    

    
  
(In reply to comment #44)
> Just because the Error Console is going to be off for the regular user doesn't
> mean we shouldn't expect it to be fully functional as intended for the final
> release for those who would need it.

First of all, we usually don't block on tweaking pref'd off features. Secondly, this appears to be an ordinary bug or enhancement request, not something that prevents the error console from being "fully functional" -- those who need it and manually enable it would get the same as in 3.6, which doesn't seem like a showstopper at all.

> If anything related to the Web (error)
> Console blocks then in my opinion a (chrome) Error Console should block too.

This makes no sense, even if everything related to the Web console would block (which isn't the case).

I wish I had discovered this bug earlier, as it seems that Rob and Dave already wasted too much time on this...
blocking2.0: betaN+ → ?

    
    

    
  
(In reply to comment #45)
It's not wasted time. For the purposes of debugging an application or extension the Error Console is cluttered with a mess of messages about content. This is now separated into two different consoles, which is good. It, however, is not finished until the Web Console has the web messages and this console has only the rest. That is what I mean by "fully functional"; there's no reason to ship the same old 3.6 version if you're already adding a partial replacement. It should be fully replaced with the set of two different consoles for different purposes.

As this is a developer feature, which is supposedly an aim of improvement for Firefox 4, I see no reason this should be considered unimportant.

    
    

    
  
It's an open bug and a valid one. Open bug doesn't translate to blocker. We'll have thousands of open bugs by the time Firefox 4 ships, assuming it ships some day. For the purpose of getting Firefox 4 out the door, trying to fix those bugs would be fatal.

    
    

    
  
I don't dispute your reasoning, but there are some bugs that fall under the category of "this really needs to be in there" for Firefox 4.0 to be "finished" that are not technically blockers by the criteria by which you're stating.

Let me put it this way: we need to use the status2.0:wanted+ flag more. ;)

          status2.0:
          --- → ?

    
    

    
  
Dao, I think the rationale for this bug was initially to reduce duplication across features. We've done a lot of work to ensure that we get all content messages into the Web Console and even clean up the way script errors are being reported. Leaving them in the Error Console seems shoddy.

Given the hard choice of whether or not I'd block the release for this, I'd probably say no, given that we're going to disable the Error Console for most users anyway. That said, we're almost to the end of this and I'd hate to drop it now.

    
    

    
  
(In reply to comment #42)
> Created attachment 496417 [details] [diff] [review]
> Error Console Show Content Test 4
> 
> (In reply to comment #41)
> > Comment on attachment 492853 [details] [diff] [review] [details]
> > Error Console Show Content Test 3
> > 
> > >diff --git a/toolkit/components/console/tests/Makefile.in b/toolkit/components/console/tests/Makefile.in
> > >new file mode 100644
> > >--- /dev/null
> > >+++ b/toolkit/components/console/tests/Makefile.in
> [...]
> > >+# The Initial Developer of the Original Code is
> > >+# Mozilla Corporation.
> > 
> > You want "the Mozilla Foundation."
> > 
> > >+# Portions created by the Initial Developer are Copyright (C) 2008
> > 
> > Woo, we have a full 2 years till we plan to ship Firefox 4 now?
> 
> Two years since that file was initially written anyway. Updated to reflect the
> state of my calendar and added the proper Initial Developer.
> 
> Updated the Original Code description to Error Console Test code.
> 
> > >diff --git a/toolkit/components/console/tests/browser/Makefile.in b/toolkit/components/console/tests/browser/Makefile.in
> > >new file mode 100644
> > >--- /dev/null
> > >+++ b/toolkit/components/console/tests/browser/Makefile.in
> > >@@ -0,0 +1,61 @@
> > >+#
> > >+# ***** BEGIN LICENSE BLOCK *****
> [...]
> > >+# The Original Code is Error Console Tests.
> > >+#
> > >+# The Initial Developer of the Original Code is
> > >+# Mozilla Corporation.
> > 
> > Ditto
> > 
> > >+# Portions created by the Initial Developer are Copyright (C) 2008
> > 
> > Maybe my watch is broken
> 
> Maybe you fell into a ... Hot Tub Time Machine! (I'm sure this reference will
> make zero sense in another 2 years.
> 
> > >diff --git a/toolkit/components/console/tests/browser/browser_errorConsoleContent.js b/toolkit/components/console/tests/browser/browser_errorConsoleContent.js
> > >new file mode 100644
> > >--- /dev/null
> > >+++ b/toolkit/components/console/tests/browser/browser_errorConsoleContent.js
> > >@@ -0,0 +1,135 @@
> > >+/* ***** BEGIN LICENSE BLOCK *****
> > >+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
> > >+ *
> [...]
> > >+ * The Initial Developer of the Original Code is
> > >+ * The Mozilla Foundation.
> > 
> > \o/
> > 
> > >+ * Portions created by the Initial Developer are Copyright (C) 2008
> > 
> > :(
> 
> I'm just gonna convert that whole thing to PD.
> 
> > >+ * the Initial Developer. All Rights Reserved.
> > >+ *
> > >+ * Contributor(s):
> > >+ *   Nochum Sossonko <nsossonko@hotmail.com>
> > >+ *   Rob Campbell <rcampbell@mozilla.com>
> > >+ *   David Dahl <ddahl@mozilla.com>
> > >+ *   Patrick Walton <pcwalton@mozilla.com>
> > >+ *   Julian Viereck <jviereck@mozilla.com>
> > >+ *   Mihai Sucan <mihai.sucan@gmail.com>
> > 
> > That's a lot of contributors for a new file, is it accurate or just copied?
> 
> Copied. Removed.
> 
> > 
> > >+function addTab(aURL)
> > >+{
> > >+  gBrowser.selectedTab = gBrowser.addTab();
> > >+  content.location = aURL;
> > >+  tab = gBrowser.selectedTab;
> > >+  browser = gBrowser.getBrowserForTab(tab);
> > 
> > Both tab and browser are undeclared, but then you don't seem to need the result
> > anyway so just drop the two lines
> 
> Whups. Using browser and tab in test() after addTab is called. I'm gonna just
> combine those and get rid of addTab.
> 
> browser sticks around so I declared it.
> 
> > >+function onLoad(aEvent) {
> > >+  browser.removeEventListener(aEvent.type, arguments.callee, true);
> > >+  var button = content.document.querySelector("button").wrappedJSObject;
> > >+  var clickEvent = content.document.createEvent("MouseEvents");
> > >+  clickEvent.initMouseEvent("click", true, true,
> > >+    content, 0, 0, 0, 0, 0, false, false,
> > >+    false, false, 0, null);
> > 
> > Any reason not to just use EventUtils.synthesizeMouse ?
> 
> Not a good reason! Changed.
> 
> > >+function testFunction(aSubject, aTopic, aObject)
> > >+{
> > >+  ok(true, "in testFunction");
> > 
> > Use info for these sorts of messages
> 
> I missed that one.
> 
> > >+  var win = aSubject.QueryInterface(Ci.nsIDOMEventTarget);
> > >+  ok(win, "we have a window");
> > 
> > This test is kind of pointless here since the line above will throw an
> > exception if aSubject is null or not an nsIDOMEventTarget.
> 
> Yeah, more debugging code really. Removed.
> 
> > >+  win.addEventListener("load", function ECT_onLoad() {
> > >+    win.removeEventListener("load", ECT_onLoad, false);
> > >+    if (win.location != ERROR_CONSOLE_URI) {
> > >+      ok(false, "not our window, returning early");
> > >+      return;
> > >+    }
> > >+
> > >+    Services.ww.unregisterNotification(testFunction, "domwindowopened", false);
> > 
> > Move this to outside the event listener to ensure it can't receive a
> > notification again.
> 
> I put the unregisterNotification inside the load listener because I didn't want
> to remove it if we were in the wrong type of window. IOW, I only remove the
> event listener of the Error Console is opened and apparently win.location is
> only populated on load (or probably DOMContentLoaded).
> 
> I'll document this in the test so it's clearer.
> 
> > >+    ok(true, "window opened");
> > >+
> > >+    var showContent = Services.prefs.getBoolPref("devtools.errorconsole.showContent");
> > >+    if (showContent)
> > >+      ok(true, "devtools.errorconsole.showContent = true");
> > >+    else
> > >+      ok(true, "devtools.errorconsole.showContent = false");
> > 
> > Should one of these be a failure? If not then just do a single info call.
> 
> Not really, since I wasn't sure what the state of
> devtools.errorconsole.showContent was going to be in the test case, and didn't
> really care, I figured we could just check that it's ok. I'll make it an info
> call.
> 
> > This test should flip the pref and re-open the window to make sure that the
> > error shows/doesn't show as appropriate.
> 
> yeah, that should be do-able without having to change anything other than
> clearing the error console at the end of testFunction.
> 
> (some time later)
> 
> Ok, played around with this and I have it testing both cases now.
> 
> > >diff --git a/toolkit/components/console/tests/chrome/Makefile.in b/toolkit/components/console/tests/chrome/Makefile.in
> > >new file mode 100644
> > >--- /dev/null
> > >+++ b/toolkit/components/console/tests/chrome/Makefile.in
> > >@@ -0,0 +1,55 @@
> > >+#
> > >+# ***** BEGIN LICENSE BLOCK *****
> > >+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
> > >+#
> [...]
> > >+# The Original Code is Error Console Tests.
> > >+#
> > >+# The Initial Developer of the Original Code is
> > >+# Mozilla Corporation.
> > 
> > *sigh*
> 
> i sorry. ._. (fixed)
> 
> > >+# Portions created by the Initial Developer are Copyright (C) 2008
> > 
> > I feel all young
> 
> you're very spry!
> 
> > >diff --git a/toolkit/components/console/tests/chrome/test_errorConsole.js b/toolkit/components/console/tests/chrome/test_errorConsole.js
> > 
> > What is the point of this test? Why not just add a syntax error into your
> > browser chrome test above and verify it does/doesn't appear?
> 
> The point of this test is to verify that errors from Chrome space appear. In
> this case, I'm generating one explicitly with Cu.reportError();
> 
> If I add a syntax error to my browser chrome test above, won't the test just
> fail in the middle? That'll make it hard for it to do further testing. I can
> document it so it's more understandable.

I would expect this to work and not kill the test:

executeSoon(function() {
  foobar();
});

executeSoon(function() {
  // Continue and test that an error about undefined foobar is in the list.
});
blocking2.0: ? → -

          status2.0:
          ? → ---

    
    

    
  
I can try that Dave.

This is no longer blocking though, do we still want this?

    
    

    
  
I think you nailed it in comment 49, Rob. We shouldn't block the release for this, but it *is* desirable and whether we continue with this should likely depend on how much work is required to finish it up. Given the number of blockers pending, it is possible that we'll have to drop this in favor of those.

    
    

    
  
I don't think it's much more work, and if Dave's above suggestion works (and I have no reason to suspect it won't), it should be easy to get it to the end of the line.

    
    

    
  
Comment on attachment 496417 [details] [diff] [review]
Error Console Show Content Test 4

I do think it'd be simpler to just do the chrome error in the browser_chrome test but this works too, just slightly more maintenance I guess.

>diff --git a/toolkit/components/console/tests/Makefile.in b/toolkit/components/console/tests/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/console/tests/Makefile.in
>@@ -0,0 +1,54 @@
>+#
>+# ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+#
>+# The contents of this file are subject to the Mozilla Public License Version
>+# 1.1 (the "License"); you may not use this file except in compliance with
>+# the License. You may obtain a copy of the License at
>+# http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS IS" basis,
>+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+# for the specific language governing rights and limitations under the
>+# License.
>+#
>+# The Original Code is Error Console Tests.
>+#
>+# The Initial Developer of the Original Code is
>+# Mozilla Foundation.
>+# Portions created by the Initial Developer are Copyright (C) 2010
>+# the Initial Developer. All Rights Reserved.
>+#
>+# Contributor(s):
>+#	  Nochum Sossonko <nsossonko@hotmail.com>
>+#     Rob Campbell <rcampbell@mozilla.com>

Unless you're trying to suggest that Nochum is somehow more important than you and needs more indentation please line these up, I believe two space indent is the norm.

>diff --git a/toolkit/components/console/tests/browser/Makefile.in b/toolkit/components/console/tests/browser/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/console/tests/browser/Makefile.in
>@@ -0,0 +1,61 @@
>+#
>+# ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+#
>+# The contents of this file are subject to the Mozilla Public License Version
>+# 1.1 (the "License"); you may not use this file except in compliance with
>+# the License. You may obtain a copy of the License at
>+# http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS IS" basis,
>+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+# for the specific language governing rights and limitations under the
>+# License.
>+#
>+# The Original Code is Error Console Tests.
>+#
>+# The Initial Developer of the Original Code is
>+# Mozilla Foundation.
>+# Portions created by the Initial Developer are Copyright (C) 2010
>+# the Initial Developer. All Rights Reserved.
>+#
>+# Contributor(s):
>+#   Nochum Sossonko <nsossonko@hotmail.com>
>+#   Rob Campbell <rcampbell@mozilla.com>

Yeah, just like that

>diff --git a/toolkit/components/console/tests/browser/browser_errorConsoleContent.js b/toolkit/components/console/tests/browser/browser_errorConsoleContent.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/console/tests/browser/browser_errorConsoleContent.js
>@@ -0,0 +1,120 @@
>+/* vim:set ts=2 sw=2 sts=2 et: */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ *
>+ * Contributor(s):
>+ *   Rob Campbell <rcampbell@mozilla.com>
>+ *
>+ * ***** END LICENSE BLOCK ***** */

Public domain is simple because we don't have to track contributors etc. See http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_migrate1.js for an example usage.

>diff --git a/toolkit/components/console/tests/chrome/test_errorConsole.js b/toolkit/components/console/tests/chrome/test_errorConsole.js

>+function test() {
>+  SimpleTest.waitForExplicitFinish();
>+  Services.ww.registerNotification(testFunction, "domwindowopened", false);
>+  window.open(ERROR_CONSOLE_URI, 
>+    "_blank",
>+    "chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar");
>+}
>\ No newline at end of file

Add one please

>diff --git a/toolkit/components/console/tests/chrome/test_errorConsole.xul b/toolkit/components/console/tests/chrome/test_errorConsole.xul
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/console/tests/chrome/test_errorConsole.xul
>@@ -0,0 +1,61 @@
>+<?xml version="1.0"?>
>+<!--
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Error Console Tests.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * The Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2008

You did so well on the dates, only to lose it at the end.

        Attachment #496417 -
        Flags: review?(dtownsend) → review+
Blocks: 490499

    
  
Assignee: rcampbell → nobody
Component: General → Error Console
QA Contact: general → error.console
Version: unspecified → Trunk

    
    

    
  
accidental assignment change? I still have this and plan to check it in with modifications. Just waiting to make sure we have all the pieces we need in the web console first.
Assignee: nobody → rcampbell

    
    

    
  
In order to avoid churn and get more feedback from users before making this
change, we're going to postpone this until after Firefox 4. (This is the route
preferred by the Firefox module owner.)

http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/d8e7c1976d4d8448#

There are still people who want to use the Error Console at this point, due to a couple remaining issues with the Web Console.
No longer blocks: devtools4

    
  
Blocks: 654023

    
    

    
  
Can we do this now?
Flags: needinfo?(rcampbell)

    
    

    
  
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #57)
> Can we do this now?

I would say yes, but don't have time to work on the patch.
Flags: needinfo?(rcampbell)
Assignee: rcampbell → nobody

    
    

    
  
I didn't know this bug existed. Maybe I can reuse this for bug 860705.

Rob, do you plan to finish and land this patch?

    
    

    
  
(In reply to Mihai Sucan [:msucan] from comment #59)
> I didn't know this bug existed. Maybe I can reuse this for bug 860705.
> 
> Rob, do you plan to finish and land this patch?

Mihai, I don't think so. It's pretty ancient at this point. Feel free to reuse what you can though and I'll mark this WONTFIX.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
Product: Toolkit → Toolkit Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: