window.console API messages from before the Web Console is opened don't show

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Developer Tools
P1
normal
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: ddahl, Assigned: msucan)

Tracking

({dev-doc-complete})

Trunk
Firefox 12
x86
All
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

(Whiteboard: [console-1])

Attachments

(1 attachment, 20 obsolete attachments)

12.53 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
Followup bug to the lazy console integration. We need to display console api log messages that are cached when the web console opens.
(Reporter)

Updated

7 years ago
Assignee: nobody → ddahl

Comment 1

7 years ago
Requesting blocking status for this. Before the console is opened, the messages will be held in a cache. That cache isn't useful if it's not displayed when the user opens the console (which is what this bug is to address.)
Blocks: 593956
blocking2.0: --- → ?
blocking2.0: ? → betaN+

Comment 2

7 years ago
Unassigning so that this can be picked up by anyone after the caching lands.
Assignee: ddahl → nobody
Depends on: 601260
(Reporter)

Comment 3

7 years ago
I already have a patch for this.
Assignee: nobody → ddahl
(Reporter)

Updated

7 years ago
Duplicate of this bug: 606793
(Reporter)

Comment 5

7 years ago
Created attachment 491435 [details] [diff] [review]
v 0.1 Display Cached ConsoleAPI Messages
Attachment #491435 - Flags: review?(gavin.sharp)
(Reporter)

Comment 6

7 years ago
(In reply to comment #5)
> Created attachment 491435 [details] [diff] [review]
> v 0.1 Display Cached ConsoleAPI Messages

Current patch is not using the refactor of "sendToWebConsole" methods - yet.
(Reporter)

Comment 7

7 years ago
(In reply to comment #6)
> Current patch is not using the refactor of "sendToWebConsole" methods - yet.

Also, this patch requires the one from bug 612658

Comment 8

7 years ago
mass change: filter on PRIORITYSETTING
Priority: -- → P1

Comment 9

7 years ago
mass change: filter on PRIORITYSETTING
(Reporter)

Comment 10

7 years ago
Created attachment 492458 [details] [diff] [review]
v 0.2 removed usage of sendToWebConsole

updated patch to latest trunk, terminated using "CAO_sendToWebConsole()"
Attachment #491435 - Attachment is obsolete: true
Attachment #492458 - Flags: review?(gavin.sharp)
Attachment #491435 - Flags: review?(gavin.sharp)
Depends on: 612658

Comment 11

7 years ago
mass change: filter mail on BLOCKERSETTING
Severity: normal → blocker
Keywords: dev-doc-needed
Whiteboard: [has patch][needs review gavin]
Marking as assigned.
Status: NEW → ASSIGNED
Comment on attachment 492458 [details] [diff] [review]
v 0.2 removed usage of sendToWebConsole

>diff --git a/dom/tests/browser/browser_ConsoleAPICachedMessages.js b/dom/tests/browser/browser_ConsoleAPICachedMessages.js

>+XPCOMUtils.defineLazyGetter(this, "HUDService", function () {
>+  Cu.import("resource:///modules/HUDService.jsm");

Can't have a dom/ test that depends on the HUD service, since it's Firefox-only. This could just move to live next to the other hudservice tests, though. A really simple dom/ test that just tests that messages are being cached (calling console.log() and checking the cache manually) would be good.

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+  displayCachedConsoleMessages: function HUD_displayCachedConsoleMessages()
>+  {
>+    // get the messages from the storageService:
>+    let messages = [];
>+    try {
>+      let windowId = HUDService.getWindowId(this.contentWindow);
>+      messages = gConsoleStorage.getConsoleStorage(windowId);
>+    }

Doesn't look like either of these calls can really fail, so the try is unnecessary.

>+    let len = messages.length;
>+    if (len) {
>+      for (let i = 0; i < len; i++) {

No need to check len beforehand.

>+        HUDService.logConsoleAPIMessage(this.hudId,

We should take this opportunity to move the logConsoleAPIMessage implementation to the HeadsUpDisplay objects themselves rather than HUDService (part of bug 592523). That way you can just do messages.forEach(this.logConsoleAPIMessage, this); (adjusting logConsoleAPIMessage's signature as needed).
Attachment #492458 - Flags: review?(gavin.sharp) → review-

Updated

7 years ago
Severity: blocker → normal
Summary: Implement WebConsole displayCachedConsoleMessages → Errors from before console is opened don't appear
(Reporter)

Comment 14

7 years ago
Created attachment 498256 [details] [diff] [review]
v 0.3 updated as per comments
Attachment #492458 - Attachment is obsolete: true
Attachment #498256 - Flags: review?(gavin.sharp)
(Reporter)

Comment 15

7 years ago
Created attachment 498886 [details] [diff] [review]
v 0.3.1 updated for bitrot and changes in storageSvc patch
Attachment #498256 - Attachment is obsolete: true
Attachment #498886 - Flags: review?(gavin.sharp)
Attachment #498256 - Flags: review?(gavin.sharp)
Comment on attachment 498886 [details] [diff] [review]
v 0.3.1 updated for bitrot and changes in storageSvc patch

>diff --git a/dom/tests/browser/Makefile.in b/dom/tests/browser/Makefile.in

>+		browser_ConsoleStorageTests.js \

This is part of some other patch, I think?

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+  displayCachedConsoleMessages: function HUD_displayCachedConsoleMessages()

>+      HUDService.logConsoleAPIMessage(this.hudId,

I suppose ideally logConsoleAPIMessage would be a method of the HUD object rather than the service, and then displayCachedConsoleMessages could be called from the constructor for HeadsUpDisplay objects rather than manually after registerHUDReference. That probably requires some refactoring though, so no need to block on that.

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_ConsoleAPICachedMessages.js b/toolkit/components/console/hudservice/tests/browser/browser_ConsoleAPICachedMessages.js

>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+const Cu = Components.utils;
>+
>+Cu.import("resource://gre/modules/Services.jsm");
>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");

Browser chrome tests run in the browser chrome window scope, where these are already imported/defined, so you shouldn't import/define them again.

>+function testOpenUI()

>+  function testLogEntry(aOutputNode, aMatchString, aSuccessErrObj)

Can't you use the one defined in head.js?
Attachment #498886 - Flags: review?(gavin.sharp) → review+
(Reporter)

Comment 17

7 years ago
(In reply to comment #16)
> Comment on attachment 498886 [details] [diff] [review]
> v 0.3.1 updated for bitrot and changes in storageSvc patch
> 
> >diff --git a/dom/tests/browser/Makefile.in b/dom/tests/browser/Makefile.in
> 
> >+		browser_ConsoleStorageTests.js \
> 
> This is part of some other patch, I think?

Ah, crap. Looks like it.

> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >+  displayCachedConsoleMessages: function HUD_displayCachedConsoleMessages()
> 
> >+      HUDService.logConsoleAPIMessage(this.hudId,
> 
> I suppose ideally logConsoleAPIMessage would be a method of the HUD object
> rather than the service, and then displayCachedConsoleMessages could be called
> from the constructor for HeadsUpDisplay objects rather than manually after
> registerHUDReference. That probably requires some refactoring though, so no
> need to block on that.

OK, filed followup bug 620958
> 
> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_ConsoleAPICachedMessages.js b/toolkit/components/console/hudservice/tests/browser/browser_ConsoleAPICachedMessages.js
> 

> >+Cu.import("resource://gre/modules/Services.jsm");
> >+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> 
> Browser chrome tests run in the browser chrome window scope, where these are
> already imported/defined, so you shouldn't import/define them again.
> 
Gotcha.

> >+function testOpenUI()
> 
> >+  function testLogEntry(aOutputNode, aMatchString, aSuccessErrObj)
> 
> Can't you use the one defined in head.js?

Yep, not sure why this is like this - perhaps I copied a test from dom/tests
(Reporter)

Updated

7 years ago
Whiteboard: [has patch][needs review gavin]
Whiteboard: [has review][needs new patch]
(Reporter)

Comment 18

7 years ago
Created attachment 500962 [details] [diff] [review]
v 0.3.2 Comments Address

r+ by gavin with the current changes
Attachment #498886 - Attachment is obsolete: true
Attachment #500962 - Flags: review+
(Reporter)

Updated

7 years ago
Whiteboard: [has review][needs new patch] → [has review][new patch attached]
(Reporter)

Comment 19

7 years ago
Created attachment 500963 [details] [diff] [review]
v 0.3.2 Comments Address

Forgot to qref a slight test change. r+ by gavin with current changes
Attachment #500962 - Attachment is obsolete: true
Attachment #500963 - Flags: review+
(Reporter)

Comment 20

7 years ago
Created attachment 501795 [details] [diff] [review]
v 0.3.3 Comments Address

moved the lazyGetter for the consoleStorageSvc (inHUDService.jsm) to this patch from the consoleStorageSvc patch
Attachment #501795 - Flags: review+
(Reporter)

Updated

7 years ago
Attachment #500963 - Attachment is obsolete: true
(Assignee)

Comment 21

7 years ago
Comment on attachment 501795 [details] [diff] [review]
v 0.3.3 Comments Address

In dom/tests/browser/Makefile.in:

+		browser_ConsoleAPICachedMessages.js \

This change is not needed. You have the test in the hudservice tests. This also breaks Make - compilation fails.
Whiteboard: [has review][new patch attached] → [has review][new patch attached][softblocker]
Depends on: 611032
No longer depends on: 601260
Out of curiosity, what is this bug waiting for before it can land?
(Reporter)

Comment 23

7 years ago
(In reply to comment #22)
> Out of curiosity, what is this bug waiting for before it can land?

It depends on bug 612658 - which is so close - we were in mid-review when we paused for hardblockers
(Reporter)

Comment 24

7 years ago
Created attachment 511799 [details] [diff] [review]
v 0.3.4 unbitrot
Attachment #501795 - Attachment is obsolete: true
Attachment #511799 - Flags: review+
(Reporter)

Updated

7 years ago
Blocks: 611032
No longer depends on: 611032
** PRODUCT DRIVERS PLEASE NOTE **

This bug is one of 19 being moved from blocking2.0:betaN+ to blocking2.0:- as we reached the endgame of Firefox 4. The rationale for the move is:

 - the bug had been identified as a "soft" blocker which could be fixed in some follow up release
 - the bug had been identified as one requiring beta coverage, thus is not appropriate for a ".x" stability & security release

The owner of the bug may wish to renominate for .x consideration.
blocking2.0: betaN+ → .x+
(er updating flag to "-" as per previous comment!)
blocking2.0: .x+ → -
(Reporter)

Updated

7 years ago
Whiteboard: [has review][new patch attached][softblocker] → [has review][new patch attached][softblocker][console-1]
(Reporter)

Comment 27

7 years ago
Created attachment 522511 [details] [diff] [review]
v 0.3.5 unbitrot
Attachment #511799 - Attachment is obsolete: true
Attachment #522511 - Flags: review+
(Reporter)

Comment 28

6 years ago
Created attachment 531532 [details] [diff] [review]
0.3.6 Un bitrot
Attachment #522511 - Attachment is obsolete: true
Attachment #531532 - Flags: review+
(Reporter)

Comment 29

6 years ago
hrmmm, getting a failure in 1 test now after updating bug 612658 and the patch on this bug:

TEST-PASS | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_notifications.js | we have a console ID
TEST-PASS | chrome://mochitests/content/browser/toolkit/components/console/hudservice/tests/browser/browser_webconsole_notifications.js | message node id is not null
*** WebConsoleTest: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebProgress.removeProgressListener]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource:///modules/HUDService.jsm :: HS_deactivateHUDForContext :: line 1473"  data: no]
*** WebConsoleTest: undefined


And of course, this is in an 'unrelated' test.

msucan: do you know why that exception would throw in this case?

very strange.
(Reporter)

Comment 30

6 years ago
Created attachment 532300 [details] [diff] [review]
v 0.4 updated for innerWindow IDs. now we have a broken test

Mihai, somehow this patch breaks the notifications test. very strange, do you think you could look at the exception - would love some feedback on why it is breaking. We throw in HUDService.jsm on line 1474: 

      browser.webProgress.removeProgressListener(hud.progressListener);
Attachment #531532 - Attachment is obsolete: true
Attachment #532300 - Flags: feedback?(mihai.sucan)
(Reporter)

Comment 31

6 years ago
Created attachment 534118 [details] [diff] [review]
0.5 Updated the logConsoleAPI call

Working all tests pass
Attachment #532300 - Attachment is obsolete: true
Attachment #534118 - Flags: review+
Attachment #532300 - Flags: feedback?(mihai.sucan)
(Reporter)

Comment 32

6 years ago
Also, the latest patch was rebased against the devtools branch
(Reporter)

Comment 33

6 years ago
Created attachment 539566 [details] [diff] [review]
v 0.6 test fixes, plus a tweak to HUDService where we throw because of undefined hud

This should be re-reviewed. A check in the test toolkit/components/console/hudservice/tests/browser/browser_webconsole_notifications.js was not passing after the consoleAPIStorage patch was applied and this patch was updated. I filed bug 664466 as a followup - i think we may just need to return early or call the UI node removal and return if the hud object is undefined. 

Anyway, all tests pass now.
Attachment #534118 - Attachment is obsolete: true
Attachment #539566 - Flags: review?(gavin.sharp)
Comment on attachment 539566 [details] [diff] [review]
v 0.6 test fixes, plus a tweak to HUDService where we throw because of undefined hud

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>     let displayNode = chromeDocument.getElementById(hudId);
> 
>     if (hudId in this.hudReferences && displayNode) {

>       let hud = this.hudReferences[hudId];
>+      try {
>+        // hud can sometimes be undefined at this point

I don't think that should be possible, particularly given the checks above. We shouldn't wallpaper over this, we need to figure out what's going on.
Attachment #539566 - Flags: review?(gavin.sharp) → review-
(Reporter)

Comment 35

6 years ago
Created attachment 539692 [details] [diff] [review]
v 0.7 fixed wonkey undefined hud behavior

Found a place or two where we assume the existence of a hud reference and there is not one
Attachment #539566 - Attachment is obsolete: true
Attachment #539692 - Flags: review?(gavin.sharp)
(Reporter)

Comment 36

6 years ago
this patch also fixes the followup: bug 664466
Comment on attachment 539692 [details] [diff] [review]
v 0.7 fixed wonkey undefined hud behavior

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+    var hud = this.hudReferences[hudId];
>+
>     if (hudId in this.hudReferences && displayNode) {

This code pattern doesn't make much sense - this change doesn't look necessary, though.

>   logConsoleAPIMessage: function HS_logConsoleAPIMessage(aHUDId, aMessage)

>     let hud = HUDService.hudReferences[aHUDId];
>+    if (!hud) {
>+      return;
>+    }

This method is called with the return value of HUDService.getHudIdByWindow, so if aHUDId is bogus, something more fundamental is wrong. Can't just wallpaper over it.

>   disableAnimation: function HS_disableAnimation(aHUDId)

>+    let reference = HUDService.hudReferences[aHUDId];
>+    if (!reference) {
>+      return;

Similarly, it should be impossible for this to be null (this is only called from activateHUDForContext).
Attachment #539692 - Flags: review?(gavin.sharp) → review-
(In reply to comment #37)
> Comment on attachment 539692 [details] [diff] [review] [review]
> v 0.7 fixed wonkey undefined hud behavior
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >   logConsoleAPIMessage: function HS_logConsoleAPIMessage(aHUDId, aMessage)
> 
> >     let hud = HUDService.hudReferences[aHUDId];
> >+    if (!hud) {
> >+      return;
> >+    }
> 
> This method is called with the return value of HUDService.getHudIdByWindow,
> so if aHUDId is bogus, something more fundamental is wrong. Can't just
> wallpaper over it.
> 
> >   disableAnimation: function HS_disableAnimation(aHUDId)
> 
> >+    let reference = HUDService.hudReferences[aHUDId];
> >+    if (!reference) {
> >+      return;
> 
> Similarly, it should be impossible for this to be null (this is only called
> from activateHUDForContext).

I removed these two checks and I don't get any failed tests on OS X. Were the failures we are investigating consistent or intermittent oranges?
(Reporter)

Comment 39

6 years ago
(In reply to comment #38)
> > Similarly, it should be impossible for this to be null (this is only called
> > from activateHUDForContext).
> 
> I removed these two checks and I don't get any failed tests on OS X. Were
> the failures we are investigating consistent or intermittent oranges?

They were consistent, but I may have tweaked some other things. Need to look at an interdiff.
(Reporter)

Comment 40

6 years ago
Created attachment 542487 [details] [diff] [review]
v 0.8 removed conditional hud checks. all tests pass

ok, looks like I may have had a wonkey build. clobbered, removed hud checks and all tests do pass now. very weird.
Attachment #539692 - Attachment is obsolete: true
Attachment #542487 - Flags: review?(gavin.sharp)
(Assignee)

Comment 41

6 years ago
Created attachment 544049 [details] [diff] [review]
v 0.9 rebase and cleanup

Updated the patch.

Rebased, some cleanup and exceptions fixed.

Looking forward to your review. Thank you!
Assignee: ddahl → mihai.sucan
Attachment #542487 - Attachment is obsolete: true
Attachment #544049 - Flags: review?(gavin.sharp)
Attachment #542487 - Flags: review?(gavin.sharp)
(Assignee)

Comment 42

6 years ago
Updating bug title to reflect what the patch is about.
Summary: Errors from before console is opened don't appear → window.console API messages from before the Web Console is opened don't show
(Assignee)

Comment 43

6 years ago
Created attachment 544320 [details] [diff] [review]
v 0.10 rebase, fix for iframes and scroll to bottom

Updated the patch. Rebased, fixed code for iframes and added logic to scroll to the bottom of the outputNode. Minimal changes, actually (only +20 lines or so).
Attachment #544049 - Attachment is obsolete: true
Attachment #544320 - Flags: review?(gavin.sharp)
Attachment #544049 - Flags: review?(gavin.sharp)
Comment on attachment 544320 [details] [diff] [review]
v 0.10 rebase, fix for iframes and scroll to bottom

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+  getInnerWindowId: function HS_getInnerWindowId(aWindow)

Don't think it's necessary to add a generic helper given that you're only adding one caller - just inline this code.

>+  displayCachedConsoleMessages: function HUD_displayCachedConsoleMessages()

>+    // get the messages from the storageService:
>+    let windowId = HUDService.getInnerWindowId(this.contentWindow.top);

Isn't contentWindow.top == contentWindow guaranteed to be true? I would hope that HUDs only get attached to top-level windows, so if it isn't I think we have a bug to fix!

>+    // Scroll to bottom.
>+    if (this.outputNode.firstChild) {
>+      this.outputNode.ensureIndexIsVisible(this.outputNode.childNodes.length - 1);

How about:
let numChildren = this.outputNode.childNodes.length;
if (numChildren)
  this.outputNode.ensureIndexIsVisible(numChildren - 1);

(I would suggest using itemCount, but its getter seems horribly expensive. :( )

r=me with those addressed.
Attachment #544320 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 45

6 years ago
Pushed this patch and the dependencies to the try server.

Results are green:

http://tbpl.mozilla.org/?tree=Try&rev=c43bd1b03e3c

Builds and logs:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail.com-c43bd1b03e3c
(Assignee)

Comment 46

6 years ago
(In reply to comment #44)
> Comment on attachment 544320 [details] [diff] [review] [review]
> v 0.10 rebase, fix for iframes and scroll to bottom
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >+  getInnerWindowId: function HS_getInnerWindowId(aWindow)
> 
> Don't think it's necessary to add a generic helper given that you're only
> adding one caller - just inline this code.

If you do not mind, I am going to keep this because it's also used in multiple places in bug 611032.


> >+  displayCachedConsoleMessages: function HUD_displayCachedConsoleMessages()
> 
> >+    // get the messages from the storageService:
> >+    let windowId = HUDService.getInnerWindowId(this.contentWindow.top);
> 
> Isn't contentWindow.top == contentWindow guaranteed to be true? I would hope
> that HUDs only get attached to top-level windows, so if it isn't I think we
> have a bug to fix!

Good point! Checked the code and yes contentWindow == contentWindow.top.

Thanks for your r+!
(Assignee)

Comment 47

6 years ago
Created attachment 544446 [details] [diff] [review]
v 0.11 comments addressed

Comments addressed.

Thanks!
Attachment #544320 - Attachment is obsolete: true
Whiteboard: [has review][new patch attached][softblocker][console-1] → [has review][new patch attached][console-1]
(Assignee)

Comment 48

6 years ago
Created attachment 550136 [details] [diff] [review]
v 0.12 rebase

Rebased patch.
Attachment #544446 - Attachment is obsolete: true
(Assignee)

Comment 49

6 years ago
Created attachment 550661 [details] [diff] [review]
v 0.13 rebase

Another rebase.
Attachment #550136 - Attachment is obsolete: true
what's going on here? This bug hasn't been poked in a couple of months so I'm giving it a stern poking.
Target Milestone: --- → Firefox 8
Version: Trunk → 9 Branch

Updated

6 years ago
Target Milestone: Firefox 8 → ---
Version: 9 Branch → Trunk
(Assignee)

Updated

6 years ago
Duplicate of this bug: 713385
(Assignee)

Comment 52

6 years ago
Created attachment 586537 [details] [diff] [review]
[in-fx-team] v 0.14 rebase

Rebased patch.
Attachment #550661 - Attachment is obsolete: true
(Assignee)

Comment 53

6 years ago
Comment on attachment 586537 [details] [diff] [review]
[in-fx-team] v 0.14 rebase

Landed:
https://hg.mozilla.org/integration/fx-team/rev/7c20376168c8
Attachment #586537 - Attachment description: v 0.14 rebase → [in-fx-team] v 0.14 rebase
(Assignee)

Updated

6 years ago
Whiteboard: [has review][new patch attached][console-1] → [console-1][fixed-in-fx-team]
Whiteboard: [console-1][fixed-in-fx-team] → [console-1]
https://hg.mozilla.org/mozilla-central/rev/7c20376168c8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
(Assignee)

Updated

6 years ago
Blocks: 664466
Documentation updated:

https://developer.mozilla.org/en/DOM/console

Mentioned on Firefox 12 for developers.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.