Closed Bug 612658 Opened 14 years ago Closed 13 years ago

Implement ConsoleAPIStorage - add caching for the window.console API

Categories

(DevTools :: General, defect, P1)

x86
All
defect

Tracking

(blocking2.0 -)

RESOLVED FIXED
Firefox 9
Tracking Status
blocking2.0 --- -

People

(Reporter: ddahl, Assigned: msucan)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [console-1][fixed-in-fx-team])

Attachments

(1 file, 29 obsolete files)

18.07 KB, patch
Details | Diff | Splinter Review
ConsoleStorageService caches all messages that eventually are displayed in each WebConsole. If the WebConsole UI is displayed, the messages will be cached and displayed. If not, the messages are cached - to be potentially displayed when the UI is opened.
I think this is already covered by bug 601260 and bug 609890?
Attached patch v 0.1 ConsoleStorageService (obsolete) — Splinter Review
Attachment #491074 - Flags: review?(gavin.sharp)
Assignee: nobody → ddahl
mass change: filter on PRIORITYSETTING
Priority: -- → P1
mass change: filter on PRIORITYSETTING
Blocks: devtools4
needs to block as this patch will allow for caching errors and console api calls regardless of having a Web Console UI visible
blocking2.0: --- → ?
Blocking. There's another bug that's a dupe and blocker, that David is going to close.
blocking2.0: ? → betaN+
Gavin:

Would love to get the review process going on this and it's related bugs: bug 609890 and bug 602199. Let me know if you won't be able to get to it any time soon, so I can find another reviewer.
Attached patch v 0.2 reduced noisyness (obsolete) — Splinter Review
removed where we throw when storage not found. too noisy.
Attachment #491074 - Attachment is obsolete: true
Attachment #495121 - Flags: review?(gavin.sharp)
Attachment #491074 - Flags: review?(gavin.sharp)
Comment on attachment 495121 [details] [diff] [review]
v 0.2 reduced noisyness

>diff --git a/dom/base/ConsoleStorageService.idl b/dom/base/ConsoleStorageService.idl

>+interface nsIConsoleStorageService : nsISupports

>+  nsIVariant createConsoleStorage(in AString aId);
>+  void removeConsoleStorage(in AString aId);
>+  void recordEvent(in AString aId, in nsIVariant aEvent);
>+  void recordGlobalEvent(in nsIVariant aEvent);
>+  nsIVariant getConsoleStorage(in AString aId);  
>+  nsIVariant getGlobalConsoleStorage();

This looks more complicated than I was expecting... can we have something more like:

void recordEvent(aWindowID, aEvent);
nsIVariant getCachedEvents(aWindowID, aEvent);

I'm not sure what the separate global storage is intended to be used for, since I don't see any use of it in the blocked bug.

>diff --git a/dom/base/ConsoleStorageService.js b/dom/base/ConsoleStorageService.js

>+ConsoleStorageService.prototype = {

>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIConsoleStorageService]),

Missing nsIObserver?

>+  getInterfaces: function CS_getInterfaces(countRef)
>+  {
>+    var interfaces = [Ci.nsIConsoleStorageService, Ci.nsIObserver,];
>+    countRef.value = interfaces.length;
>+    return interfaces;
>+  },

This isn't necessary - this is a nsIDOMClassInfo method, and this component doesn't implement that interface (and doesn't need to).

>+  recordEvent: function CS_recordEvent(aId, aEvent)

Seems like this method should be a no-op if we're in private browsing mode.

>+  truncateConsoleStorage: function CS_truncateConsoleStorage(aId)

This is fine for preventing individual buckets from getting too big, but there's no limit on the number of buckets, right? Nor does there seem to be anything that evicts the contents of the cache when windows are destroyed, which seems problematic. Perhaps we can address that by adding a notification similar to content-document-global-created but for destruction, and use that to trigger the evicting.

Another potential issue is that the window IDs we use are outer window IDs, so entirely different documents/domains loaded in the same tab will share a cache. It seems like it would be an odd experience to open the console on a page and potentially end up seeing a bunch of messages from previous unrelated pages that have been loaded in that tab... For this use case it seems like we actually want inner window IDs, which I suppose we could do by having the ConsoleAPI object pass both in its event object (using nsIDOMWindowUtils::GetCurrentInnerWindowID).
Attachment #495121 - Flags: review?(gavin.sharp) → review-
That's a really good point, re: outer window.IDs as our cache. Presumably we'll need both outer and inner to keep events stored across reloads. Or will we? I guess we only care in that case if the console is opened, and it should be able to keep the previous contents before the reload anyway.

content-document-global-destroyed sounds like a good idea for doing this. Would be cleaner than using a ProgressListener to watch the document.
(In reply to comment #10)
> That's a really good point, re: outer window.IDs as our cache. Presumably we'll
> need both outer and inner to keep events stored across reloads. Or will we? I
> guess we only care in that case if the console is opened, and it should be able
> to keep the previous contents before the reload anyway.
> 

I'm not sure this matters at all. it really is all about the tab, not so much the window. Isn't it true that a new inner id will be generated for a new inner window at the same URL? Web devs will be reloading the same page over and over to debug stuff. Won't this just complicate the way we display logged data/events?

> content-document-global-destroyed sounds like a good idea for doing this. Would
> be cleaner than using a ProgressListener to watch the document.

indeed, this will be a clean way to jettison old storage data.
(In reply to comment #11)
 
> I'm not sure this matters at all. it really is all about the tab, not so much
> the window. Isn't it true that a new inner id will be generated for a new inner
> window at the same URL? Web devs will be reloading the same page over and over
> to debug stuff. Won't this just complicate the way we display logged
> data/events?

Yeah, a reload replaces the inner window object with a new one as does any other type of navigation. The outer window associated with the tab seems to make sense, though it might be difficult to isolate chatter in the log across page load boundaries.

(maybe a side-note for a follow-up bug to add separators to the console on navigation or reload)

What do you think, Gavin? We could still store the inner window id, but collecting for the outer window still seems to make sense to me. The tab is the focus of the user, not the transient objects underneath it.

> > content-document-global-destroyed sounds like a good idea for doing this. Would
> > be cleaner than using a ProgressListener to watch the document.
> 
> indeed, this will be a clean way to jettison old storage data.
(In reply to comment #9)
> Comment on attachment 495121 [details] [diff] [review]
> v 0.2 reduced noisyness
> 
> >diff --git a/dom/base/ConsoleStorageService.idl b/dom/base/ConsoleStorageService.idl
> 
> >+interface nsIConsoleStorageService : nsISupports
> 
> >+  nsIVariant createConsoleStorage(in AString aId);
> >+  void removeConsoleStorage(in AString aId);
> >+  void recordEvent(in AString aId, in nsIVariant aEvent);
> >+  void recordGlobalEvent(in nsIVariant aEvent);
> >+  nsIVariant getConsoleStorage(in AString aId);  
> >+  nsIVariant getGlobalConsoleStorage();
> 
> This looks more complicated than I was expecting... can we have something more
> like:
> 
> void recordEvent(aWindowID, aEvent);
> nsIVariant getCachedEvents(aWindowID, aEvent);
> 
OK. sounds good.

> I'm not sure what the separate global storage is intended to be used for, since
> I don't see any use of it in the blocked bug.


I can remove it - I was anticipating using it for what will be vaporware for some time to come:)
(In reply to comment #9)
> 
> Seems like this method should be a no-op if we're in private browsing mode.
> 
I have another bug that deals with private browsing, can I attend to this change there? see bug 602199: 

> >+  truncateConsoleStorage: function CS_truncateConsoleStorage(aId)
> Perhaps we can address that by adding a notification
> similar to content-document-global-created but for destruction, and use that to
> trigger the evicting.
ok
Attached patch v 0.3 Review comments adressed (obsolete) — Splinter Review
I am using a call to notify observers ("console-storage-cache-event") that should not be used, as the test is the only thing listening. What is the better way of doing this?
Attachment #495121 - Attachment is obsolete: true
Attachment #496917 - Flags: review?(gavin.sharp)
(In reply to comment #15)
> Created attachment 496917 [details] [diff] [review]
> v 0.3 Review comments adressed
I removed the xpcshell test and am using a browser chrome test now because of the storage IDL changes - the test relied on previously public API calls. I still need one more test that exhaustively calls the API.
Attached patch v 0.4 comments addressed (obsolete) — Splinter Review
added to test so all api methods are called
Attachment #496917 - Attachment is obsolete: true
Attachment #496923 - Flags: review?(gavin.sharp)
Attachment #496917 - Flags: review?(gavin.sharp)
The cache is only used to populate the console on opening. It seems to me that we really only want to do that for the current child inner windows, rather than all of the inner windows that have ever been associated with the outer window (tab). Storing using the inner window ID as the key and properly evicting the cached data for destroyed windows will do that automatically. You can still persist data across reloads while the console is open, because that doesn't depend on the cache.

Private browsing support isn't hard to add (single call to isPrivateBrowsingEnabled), so it shouldn't be deferred to a followup bug - let's just do it right the first time.
(In reply to comment #18)
> The cache is only used to populate the console on opening. It seems to me that
> we really only want to do that for the current child inner windows, rather than
> all of the inner windows that have ever been associated with the outer window
> (tab).

I think this will break from what might be expected from many developers (me included), as firebug keeps old logging data until you clear it or you hit a "MAX" limit. Also the Error Console keeps old data around until you hit 300 log messages.

> 
> Private browsing support isn't hard to add (single call to
> isPrivateBrowsingEnabled), so it shouldn't be deferred to a followup bug -
> let's just do it right the first time.

Right. I added it after all. (as far as recordEvent being a noop). I have another patch that clears the entire storage object when leaving PB mode.
Attached patch v 0.4.1 comments addressed (obsolete) — Splinter Review
removed unused IDL method
Attachment #496923 - Attachment is obsolete: true
Attachment #496963 - Flags: review?(gavin.sharp)
Attachment #496923 - Flags: review?(gavin.sharp)
(In reply to comment #19)
> I think this will break from what might be expected from many developers (me
> included), as firebug keeps old logging data until you clear it or you hit a
> "MAX" limit. Also the Error Console keeps old data around until you hit 300 log
> messages.

Why would you want the web console to show output from previous pages *when you open it*? I'm not suggesting we need to clear output while it's open, but only that we discard data generated by now-dead documents while the console is closed. If you're debugging a page and reloading it a lot, you're going to have the console open all the time, so our behavior here won't matter.

I don't think the old error console's behavior is particularly relevant. The nature of its output is slightly different (warnings generated by us running/loading the page rather than the page itself), and it's global, rather than tab specific, so it really has no other choice than to just show everything and act like a dumb buffer.

We currently don't even support showing exceptions thrown prior to the web console being opened for the *current page*, so my proposed behavior is still a net win over what we currently do for other types of console output.
Group: mozilla-confidential
(In reply to comment #21)
> Why would you want the web console to show output from previous pages *when you
> open it*? I'm not suggesting we need to clear output while it's open, but only
> that we discard data generated by now-dead documents while the console is
> closed. If you're debugging a page and reloading it a lot, you're going to have
> the console open all the time, so our behavior here won't matter.

OK, I hear ya. Just playing devil's advocate. I can see a bug being filed over and over again for this, but it really is not that big a deal.
it's a good advocate to play, David.

I was thinking the same thing regarding stashing of previous inner window messages, but on further persuasion by Gavin, I think making a clean break and just giving contents of the current inner window on console activation is a good idea, will give us cleaner results and probably make the console snappier to boot.
Group: mozilla-confidential
(In reply to comment #23)
> it's a good advocate to play, David.
> 
> I was thinking the same thing regarding stashing of previous inner window
> messages, but on further persuasion by Gavin, I think making a clean break and
> just giving contents of the current inner window on console activation is a
> good idea, will give us cleaner results and probably make the console snappier
> to boot.

I just confirmed with Jonas that document-content-global-created is synchronous and a safe place to empty the storage array before the new window loads a document, script or css.

So, it will be simpler to empty the cache on window create, ignoring completely the innerWindow ids, etc...
There is an additional observer topic that is used in this patch, which is the wrong way to handle the events in the tests. I would love some pointers on how to do it correctly.
Attachment #496963 - Attachment is obsolete: true
Attachment #497997 - Flags: review?(gavin.sharp)
Attachment #496963 - Flags: review?(gavin.sharp)
Attachment #497997 - Attachment is obsolete: true
Attachment #498872 - Flags: review?(gavin.sharp)
Attachment #497997 - Flags: review?(gavin.sharp)
forgot to hg add a file
Attachment #498872 - Attachment is obsolete: true
Attachment #498873 - Flags: review?(gavin.sharp)
Attachment #498872 - Flags: review?(gavin.sharp)
Comment on attachment 498873 [details] [diff] [review]
v 0.5.1 moved test to toolkit, some tweaks

>diff --git a/content/base/test/file_htmlserializer_2.html b/content/base/test/file_htmlserializer_2.html
>index 2156b1610c6a19d841d75059b31936f2237ac25e..99973da63bad131a6277d481a5eec484f3dc4a69
>GIT binary patch

Looks like this somehow got included by mistake.

>diff --git a/dom/base/ConsoleStorageService.js b/dom/base/ConsoleStorageService.js

>+XPCOMUtils.defineLazyServiceGetter(this, "gPrivBrowsing",
>+                                   "@mozilla.org/privatebrowsing;1",
>+                                   "nsIPrivateBrowsingService");

This needs to be a soft dependency (private browsing service isn't part of Gecko so might not exist), so I don't think you can use defineLazyServiceGetter. You probably need to use a normal defineLazyGetter that puts the getService call in a try/catch and returns null if it fails, and then null check gPrivBrowsing everywhere it's used.

>+function ConsoleStorageService()

>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIConsoleStorageService,
>+                                         Ci.nsIObserver,
>+                                         Ci.nsISupports]),

Don't need to include nsISupports in argument passed to generateQI.

>+  getInterfaces: function CS_getInterfaces(countRef)

Don't need this, this is a nsIClassInfo method and you don't implement that.

>+  observe: function CS_observe(aSubject, aTopic, aData)

>+    else if (aTopic == "private-browsing") {
>+      if (aData == "exit") {
>+        this.resetStorage();

I think this isn't needed, given that you don't record data while in PB mode.

>+  createConsoleStorage: function CS_createConsoleStorage(aId)

This doesn't seem worth splitting out - just inline it where it's currently called in recordEvent?

>+  removeConsoleStorage: function CS_removeConsoleStorage(aId)

This isn't on the interface and has no caller.

>+  /**
>+   * Record an event to the storage array specified

How about "Record an event associated with the given window ID" (same re-wording applies to the interface docs - "storage array" as a term seems like an implementation detail that's best avoided).

>+   * @param string aId
>+   *        The id of the storage array

"windowID, the ID of the outer window for which the event occurred."

>+  recordEvent: function CS_recordEvent(aId, aEvent)
>+  {
>+    if (gPrivBrowsing.privateBrowsingEnabled) {
>+      this.resetStorage();

I don't see why you'd want to resetStorage here.

>+    aEvent.timestamp = Date.now();

This doesn't seem to be used? Not sure it's a good idea in general to modify the passed-in event object.

>+  truncateConsoleStorage: function CS_truncateConsoleStorage(aId)

>+  deleteStorage: function CS_DeleteStorage()

These also seems like they would be simpler to just have inline.

>+  removeStorage: function CS_removeStorage(aId)

Hmm, I think we still need a better solution to clearing out data for closed windows that doesn't rely on the HUDService calling removeStorage. Perhaps we do need that outer-window-destroyed notification after all.

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

>   uninit: function HWO_uninit()
>   {
>-    Services.obs.removeObserver(this, "content-document-global-created");

This looks unrelated?
Attachment #498873 - Flags: review?(gavin.sharp) → review-
(In reply to comment #29)

> >diff --git a/content/base/test/file_htmlserializer_2.html b/content/base/test/file_htmlserializer_2.html
> >index 2156b1610c6a19d841d75059b31936f2237ac25e..99973da63bad131a6277d481a5eec484f3dc4a69
> >GIT binary patch
> 
> Looks like this somehow got included by mistake.

Yep. hg confused. will edit patch after qref

> >diff --git a/dom/base/ConsoleStorageService.js b/dom/base/ConsoleStorageService.js

> This needs to be a soft dependency (private browsing service isn't part of
> Gecko so might not exist), so I don't think you can use
> defineLazyServiceGetter. 
OK. 10 - 4.

> >+    else if (aTopic == "private-browsing") {
> >+      if (aData == "exit") {
> >+        this.resetStorage();
> 
> I think this isn't needed, given that you don't record data while in PB mode.

I was being too aggressive on that front.

> >+  recordEvent: function CS_recordEvent(aId, aEvent)
> >+  {
> >+    if (gPrivBrowsing.privateBrowsingEnabled) {
> >+      this.resetStorage();
> 
> I don't see why you'd want to resetStorage here.

right. too much data cleaning again.

> >+  truncateConsoleStorage: function CS_truncateConsoleStorage(aId)
> 
> >+  deleteStorage: function CS_DeleteStorage()
> 
> These also seems like they would be simpler to just have inline.

Check.

> 
> >+  removeStorage: function CS_removeStorage(aId)
> 
> Hmm, I think we still need a better solution to clearing out data for closed
> windows that doesn't rely on the HUDService calling removeStorage. Perhaps we
> do need that outer-window-destroyed notification after all.

Cool. so simple. 

> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >   uninit: function HWO_uninit()
> >   {
> >-    Services.obs.removeObserver(this, "content-document-global-created");
> 
> This looks unrelated?

i'll add it back if it does not cause anymore asserts - it seems like we were trying to remove observers that were already removed.
(In reply to comment #30)
> i'll add it back if it does not cause anymore asserts - it seems like we were
> trying to remove observers that were already removed.

Looks like Josh filed that as bug 620832.
More of a feedback request... Trying to figure out a nasty leak of at least a DOMWindow, maybe you can pinpoint it? I'll keep looking at it as well.
Attachment #498873 - Attachment is obsolete: true
Attachment #499226 - Flags: review?(gavin.sharp)
Attachment #499226 - Attachment is obsolete: true
Attachment #499348 - Flags: review?(gavin.sharp)
Attachment #499226 - Flags: review?(gavin.sharp)
Attached patch v 0.6.1 forgot to remove idl (obsolete) — Splinter Review
Attachment #499348 - Attachment is obsolete: true
Attachment #499349 - Flags: review?(gavin.sharp)
Attachment #499348 - Flags: review?(gavin.sharp)
Comment on attachment 499349 [details] [diff] [review]
v 0.6.1 forgot to remove idl

>diff --git a/dom/base/ConsoleAPI.js b/dom/base/ConsoleAPI.js

>+XPCOMUtils.defineLazyGetter(this, "gConsoleStorage", function (){
>+  Cu.import("resource://gre/modules/ConsoleStorageService.jsm");
>+  return consoleStorageService;
>+});

Need to use the object scoping variant of import() to avoid polluting the global scope:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#332

>diff --git a/dom/base/ConsoleStorageService.jsm b/dom/base/ConsoleStorageService.jsm

>+var EXPORTED_SYMBOLS = ["consoleStorageService"];

ConsoleStorageService capitalization is more conventional for module symbols, I think, so switch them around?

>+function ConsoleStorageService()
>+{
>+  Services.obs.addObserver(this, "xpcom-shutdown", false);
>+  Services.obs.addObserver(this, "content-document-global-created", false);
>+  Services.obs.addObserver(this, "private-browsing", false);

no longer needed

>+  observe: function CS_observe(aSubject, aTopic, aData)

>+    else if (aTopic == "content-document-global-created") {
>+      // need to clear out previously-loaded window's storage:
>+      let windowID  = aSubject.QueryInterface(Ci.nsIInterfaceRequestor).
>+                        getInterface(Ci.nsIDOMWindowUtils).outerWindowID;
>+      this.consoleStorage[windowID] = [];

delete this.consoleStorage[windowID]? Might not exist, in which case you don't really want to set it.

>+    else if (aTopic == "outer-window-destroyed") {
>+      try {
>+        // aSubject will sometimes throw NS_NOINTERFACE here
>+        // They are windows we are uninterested in
>+        let windowID  = aSubject.QueryInterface(Ci.nsIInterfaceRequestor).
>+                          getInterface(Ci.nsIDOMWindowUtils).outerWindowID;
>+        this.removeStorage(windowID);

This topic actually only gets the ID, as a nsISupportsPRUint64 (see http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#6374 ).

So I believe you want just windowID = aSubject.QueryInterface(Ci.nsISupportsPRUint64)

>+  getCachedEvents: function CS_getCachedEvents(aId)

>+    // TODO: report info message here "No storage for Window ID blah"

Don't think this is needed - it's not really an exceptional case for there not to be anything stored for a given window ID.

>+  recordEvent: function CS_recordEvent(aWindowID, aEvent)
>+  {
>+    if (gPrivBrowsing) {
>+      if (gPrivBrowsing.privateBrowsingEnabled) {

Collapse these into one line

>+    var storage = this.getCachedEvents(aWindowID);
>+    storage.push(aEvent);
>+
>+    // truncate
>+    var maxLength, toRemove;
>+    var currentStorage = this.consoleStorage[aWindowID];
>+    var len = currentStorage.length;
>+
>+    if (len > STORAGE_MAX_EVENTS) {
>+      toRemove = (len - STORAGE_MAX_EVENTS);
>+      currentStorage.splice(0, toRemove);
>+    }

currentStorage and storage refer to the same array here, so this can be simplified. There's no real benefit to accessing consoleStorage via getCachedEvents, so AFAICT this could just be:

storage.push(aEvent);
if (storage.length > STORAGE_MAX_EVENTS)
  storage.shift();

>+  removeStorage: function CS_removeStorage(aId)
>+  {
>+    try {
>+      delete this.consoleStorage[aId];
>+    } catch (ex) {}

delete can't ever throw, AFAIK, so the try isn't needed.

>diff --git a/dom/base/ConsoleStorageService.manifest b/dom/base/ConsoleStorageService.manifest

No longer needed.

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

>   closeConsoleOnTab: function HS_closeConsoleOnTab(aTab)

>+    // need to kill off the consoleStorageData
>+    let windowID = this.getWindowId(linkedBrowser.contentWindow);

Also no longer needed now that we evict data on window destruction.
Attachment #499349 - Flags: review?(gavin.sharp) → review-
(In reply to comment #35)
> Comment on attachment 499349 [details] [diff] [review]

> >+function ConsoleStorageService()
> >+{
> >+  Services.obs.addObserver(this, "xpcom-shutdown", false);
> >+  Services.obs.addObserver(this, "content-document-global-created", false);
> >+  Services.obs.addObserver(this, "private-browsing", false);
> 
> no longer needed

I don't follow - why not?
Attachment #499349 - Attachment is obsolete: true
Attachment #499414 - Flags: review?(gavin.sharp)
removed addObserver call for private browsing. Gavin: I think that is what you meant.
Attachment #499414 - Attachment is obsolete: true
Attachment #499417 - Flags: review?(gavin.sharp)
Attachment #499414 - Flags: review?(gavin.sharp)
Yes, sorry I didn't trim enough - that is what I meant.
Comment on attachment 499417 [details] [diff] [review]
v 0.7.1 comments addressed, leaks gone!

>diff --git a/dom/base/ConsoleStorageService.jsm b/dom/base/ConsoleStorageService.jsm

>+function consoleStorageService()

>+  observe: function CS_observe(aSubject, aTopic, aData)

>+    else if (aTopic == "content-document-global-created") {
>+      // need to clear out previously-loaded window's storage:
>+      let windowID  = aSubject.QueryInterface(Ci.nsIInterfaceRequestor).
>+                        getInterface(Ci.nsIDOMWindowUtils).outerWindowID;

Missing a call to actually use windowID here. Should probably have a test that covers this? Just need to check that the cache for a given outer window gets cleared on navigation.

>+    else if (aTopic == "outer-window-destroyed") {
>+      try {
>+        let windowID  = aSubject.QueryInterface(Ci.nsISupportsPRUint64);
>+        this.removeStorage(windowID);

Test for this too would be good (close window, test that cache is cleared). Don't think the try{}/catch{} is necessary.

>+  getCachedEvents: function CS_getCachedEvents(aId)

This can just be:
return this.consoleStorage[aId] || [];

>+  recordEvent: function CS_recordEvent(aWindowID, aEvent)

It would probably be good to validate that aWindowID is an integer.

let ID = parseInt(aWindowID);
if (isNaN(ID))
  throw new Error("Invalid window ID argument");

>+    var storage = this.getCachedEvents(aWindowID);

let storage = this.consoleStorage[ID];

>+    storage.push(aEvent);
>+
>+    // truncate
>+    storage.push(aEvent);

Adding twice is wrong. Could use a test that would catch this too (check cache length before/after a cached event addition?)

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

>+XPCOMUtils.defineLazyGetter(this, "gConsoleStorage", function () {
>+  let obj = {};
>+  Cu.import("resource://gre/modules/ConsoleStorageService.jsm", obj);
>+  return obj.ConsoleStorageService;
>+});

Now unused

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

>+const PB_KEEP_SESSION_PREF = "browser.privatebrowsing.keep_current_session";

>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+Cu.import("resource://gre/modules/Services.jsm");

Again, no need to import these.
Attachment #499417 - Flags: review?(gavin.sharp) → review-
Whiteboard: [needs new patch]
Attachment #499417 - Attachment is obsolete: true
Attachment #501590 - Flags: review?(gavin.sharp)
Whiteboard: [needs new patch] → [has new patch]
Whiteboard: [has new patch] → [has new patch][softblocker]
gavin:

just curious if you will have time to finish this review this week? Would LOVE to get this in for Beta 12.
Depends on: 611032
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+ → -
Whiteboard: [has new patch][softblocker] → [has new patch][softblocker][console-1]
Attached patch 0.7.3 unbitrot (obsolete) — Splinter Review
Attachment #501590 - Attachment is obsolete: true
Attachment #522510 - Flags: review?(gavin.sharp)
Attachment #501590 - Flags: review?(gavin.sharp)
Comment on attachment 522510 [details] [diff] [review]
0.7.3 unbitrot

>diff --git a/content/base/test/file_htmlserializer_2.html b/content/base/test/file_htmlserializer_2.html

I hope you sorted out why this keeps appearing in your diffs? :)

>diff --git a/dom/base/ConsoleStorageService.jsm b/dom/base/ConsoleStorageService.jsm

>+consoleStorageService.prototype = {

>+    else if (aTopic == "content-document-global-created") {

>+    else if (aTopic == "outer-window-destroyed") {

Looks like we can actually store based on the inner window ID, and use the notification from bug 484710 for eviction.

That means:
  - edit ConsoleAPI to also pass the inner window ID
  - make this component store based on inner window IDs (and adjust documentation to make that clear)
  - use inner-window-destroyed to evict entries from the cache

We might also want to reset the storage on memory-pressure notifications.

>diff --git a/toolkit/components/console/hudservice/tests/browser/Makefile.in b/toolkit/components/console/hudservice/tests/browser/Makefile.in

>+	browser_ConsoleStorageAPITests.js \

Shouldn't this test live under dom/, next to the relevant code? (it might need to not depend on PrivateBrowsing service existing then, I suppose)

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

>+  observe: function CO_observe(aSubject, aTopic, aData)

>+        gWindow.wrappedJSObject.console.log("testing noop in storage");
>+
>+        let events = gConsoleStorage.getCachedEvents(gWindowID);
>+        let checkDone = false;
>+        for (let idx in events) {
>+          let msg = Array.join(events[idx].arguments, " ");
>+          if (msg == "testing noop in storage") {
>+            ok(false, "Found a logged message during Private Browsing");

Shouldn't this just check that event.length == 0 (or that the length didn't change from before the .log() call)
Attachment #522510 - Flags: review?(gavin.sharp) → review-
(In reply to comment #46)
> Comment on attachment 522510 [details] [diff] [review] [review]
> 0.7.3 unbitrot
> 
> >diff --git a/content/base/test/file_htmlserializer_2.html b/content/base/test/file_htmlserializer_2.html
> 
> I hope you sorted out why this keeps appearing in your diffs? :)
I really do not get it. It must be a linux thing. Some patches pick up "changed files" when I never have opened or touched them. I will eyeball the patch before attaching. 

> 
> >diff --git a/dom/base/ConsoleStorageService.jsm b/dom/base/ConsoleStorageService.jsm
> 
> >+consoleStorageService.prototype = {
> 
> >+    else if (aTopic == "content-document-global-created") {
> 
> >+    else if (aTopic == "outer-window-destroyed") {
> 
> Looks like we can actually store based on the inner window ID, and use the
> notification from bug 484710 for eviction.
> 
Sounds good.

> We might also want to reset the storage on memory-pressure notifications.
>
check - did not know about that topic, very cool.
 
> >diff --git a/toolkit/components/console/hudservice/tests/browser/Makefile.in b/toolkit/components/console/hudservice/tests/browser/Makefile.in
> 
> >+	browser_ConsoleStorageAPITests.js \
> 
> Shouldn't this test live under dom/, next to the relevant code? (it might
> need to not depend on PrivateBrowsing service existing then, I suppose)
> 
I thought we were being pretty adamant about testing new features with PB in mind. That is the only reason the test is in toolkit. I can make 2 tests, but is that really necessary?

> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_ConsoleStorageAPITests.js b/toolkit/components/console/hudservice/tests/browser/browser_ConsoleStorageAPITests.js
> 
> >+  observe: function CO_observe(aSubject, aTopic, aData)
> 
> >+        gWindow.wrappedJSObject.console.log("testing noop in storage");
> >+
> >+        let events = gConsoleStorage.getCachedEvents(gWindowID);
> >+        let checkDone = false;
> >+        for (let idx in events) {
> >+          let msg = Array.join(events[idx].arguments, " ");
> >+          if (msg == "testing noop in storage") {
> >+            ok(false, "Found a logged message during Private Browsing");
> 
> Shouldn't this just check that event.length == 0 (or that the length didn't
> change from before the .log() call)

right, nice shortcut. I think i needed to add a resetStorage() call before doing that check, that's why it was so convoluted.
Attached patch 0.8 Addressed gavin's comments (obsolete) — Splinter Review
Attachment #522510 - Attachment is obsolete: true
Attachment #531742 - Flags: review?(gavin.sharp)
Comment on attachment 531742 [details] [diff] [review]
0.8 Addressed gavin's comments

>diff --git a/dom/base/ConsoleAPI.js b/dom/base/ConsoleAPI.js

>   init: function CA_init(aWindow) {

>     try {
>-      id = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+      outerID = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>                   .getInterface(Ci.nsIDOMWindowUtils)
>                   .outerWindowID;
>-    } catch (ex) {
>+      innerID = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+                  .getInterface(Ci.nsIDOMWindowUtils).currentInnerWindowID;
>+    }

Seems to me like this should be inside the try{} too, rather than the catch.

>diff --git a/dom/base/ConsoleStorageService.jsm b/dom/base/ConsoleStorageService.jsm

>+function consoleStorageService()
>+{
>+  Services.obs.addObserver(this, "xpcom-shutdown", false);
>+  Services.obs.addObserver(this, "content-document-global-created", false);

No need to watch for this anymore.

>+  Services.obs.addObserver(this, "outer-window-destroyed", false);

"inner", not "outer"
Attachment #531742 - Flags: review?(gavin.sharp) → review-
(In reply to comment #47)
> I thought we were being pretty adamant about testing new features with PB in
> mind. That is the only reason the test is in toolkit. I can make 2 tests,
> but is that really necessary?

Yes, the test should test the PB interaction (if the PB service exists). It just also needs to handle the PB service not existing (this is true regardless of whether it lives in toolkit/ or dom/). There's no need for two tests.
(In reply to comment #49)
> Comment on attachment 531742 [details] [diff] [review] [review]
> 0.8 Addressed gavin's comments
> 
> >diff --git a/dom/base/ConsoleAPI.js b/dom/base/ConsoleAPI.js
> 
> >   init: function CA_init(aWindow) {
> 
> >     try {
> >-      id = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> >+      outerID = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> >                   .getInterface(Ci.nsIDOMWindowUtils)
> >                   .outerWindowID;
> >-    } catch (ex) {
> >+      innerID = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> >+                  .getInterface(Ci.nsIDOMWindowUtils).currentInnerWindowID;
> >+    }
> 
> Seems to me like this should be inside the try{} too, rather than the catch.
>
indeed, it was: https://bugzilla.mozilla.org/attachment.cgi?id=531742&action=diff  -- weird, huh? hg freakout or something?
Attachment #531742 - Attachment is obsolete: true
Attachment #532050 - Flags: review?(gavin.sharp)
(In reply to comment #51)
> indeed, it was:
> https://bugzilla.mozilla.org/attachment.cgi?id=531742&action=diff  -- weird,
> huh? hg freakout or something?

oh no, I just misread the diff! my bad :)
Comment on attachment 532050 [details] [diff] [review]
v 0.9 Fixed test and observer fixes

>diff --git a/dom/base/ConsoleAPI.js b/dom/base/ConsoleAPI.js

> ConsoleAPI.prototype = {

>   init: function CA_init(aWindow) {

>+      outerID = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+                  .getInterface(Ci.nsIDOMWindowUtils).outerWindowID;
>+      
>+      innerID = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+                  .getInterface(Ci.nsIDOMWindowUtils).currentInnerWindowID;

nit: add a local var for "windowUtils" and (trim that unnecessary whitespace on the blank line :)

>diff --git a/dom/base/ConsoleStorageService.jsm b/dom/base/ConsoleStorageService.jsm

>+  removeStorage: function CS_removeStorage(aId)

>+  resetStorage: function CS_resetStorage()

These are kind of confusingly similar. Perhaps removeStorage should be removeStorageForWindow, and resetStorage should be clearStorage? I suppose removeStorageForWindow doesn't even need to be "public".

>+var ConsoleStorageService = new consoleStorageService();

This is a bit confusing. I'd just have all the methods live on a ConsoleStorageService object directly, give it an init method, and then call that here. So:

var ConsoleStorageService = {
  // everything you put in the prototype above

  init: function () {
    // addObserver calls from consoleStorageService constructor
  }
}

ConsoleStorageService.init();

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

>+/* ***** BEGIN LICENSE BLOCK *****

You can use the PD license from https://www.mozilla.org/MPL/boilerplate-1.1/pd-c for tests, if you'd like.

>+XPCOMUtils.defineLazyServiceGetter(this, "pb",
>+                                   "@mozilla.org/privatebrowsing;1",
>+                                   "nsIPrivateBrowsingService");
>+
>+if (!pb) {
>+  let pb = null;
>+}

This won't work, since getService will throw. You need to use defineLazyGetter and handle fallback yourself.

>+var tab, gWindow, gWindowID, apiCallCount;

Please avoid using global variables wherever possible. browser_loadDisallowInherit.js is a good example of this. gWindowID doesn't need to be a global (can be retrieved when needed), tab can be local, etc.

>+var ConsoleObserver = {

>+  init: function CO_init()

>+    Services.prefs.setBoolPref(PB_KEEP_SESSION_PREF, true);

Looks like this pref doesn't get unset. Should clearUserPref it in a cleanup function (and set it in the test() function, rather than in this init() method).

>+  observe: function CO_observe(aSubject, aTopic, aData)

>+    if (pb) {
>+      if (aTopic == "private-browsing") {

You won't get private browsing notifications if the service doesn't exist, so these checks are redundant.

>+          gConsoleStorage.resetStorage();

You should have an explicit test for resetStorage() (probably in console-storage-cache-event), that checks length before/after. The PB test just needs to check that length before == length after (i.e. no additional entry was added). It would probably be cleaner to have the PB test in a seperate file, just to keep the tests simpler.

>+            // test to make sure the storage is cleared on navigation

>+                  ok(storageArray.length == 0, "storageArray is empty");

This should also check that the storage for the old window's ID is empty, shouldn't it? This could be tricky because the window could end up in bfcache and thus not be dead. Same applies to the tab close case (but delayed GC might be the issue there). Maybe you can use nsIDOMWindowUtils.garbageCollect() to ensure that the window dies.

>+                  // now test that the console cache is cleared on tab close:

This test doesn't seem to actually wait for TEST_URI to load, so I don't think it's testing what it wants to.

>+function test()

>+      let win = XPCNativeWrapper.unwrap(gWindow);

|win| is unused here (you should use it instead of "gWindow.wrappedJSObject")
Attachment #532050 - Flags: review?(gavin.sharp) → review-
(In reply to comment #54)

> This should also check that the storage for the old window's ID is empty,
> shouldn't it? This could be tricky because the window could end up in
> bfcache and thus not be dead. Same applies to the tab close case (but
> delayed GC might be the issue there). Maybe you can use
> nsIDOMWindowUtils.garbageCollect() to ensure that the window dies.

Yep. this is tricky. I log in the test when the window is closed and the inner-window-destroyed event fires. That appears to be taking to long to sweep out the messages from the storage array. executeSoon is not helping either.
So if you do these in order:
- close the tab (gBrowser.removeTab with {animate: false})
- call nsIDOMWindowUtils.garbageCollect
- check that the storage for the window ID is gone

you should be able to get consistent results. Is that not the case?
(In reply to comment #56)
> So if you do these in order:
> - close the tab (gBrowser.removeTab with {animate: false})
> - call nsIDOMWindowUtils.garbageCollect
> - check that the storage for the window ID is gone
> 
> you should be able to get consistent results. Is that not the case?

I keep getting an event in the cache, test:

        // make sure a closed window's events are in fact removed from the
        // storage cache
        window.console.log("adding a new event");

        // close the window - the storage cache should now be empty
        gBrowser.removeTab(tab, {animate: false});

        // need to get a new handle to a window in order to call garbageCollect()
        tab = gBrowser.addTab(TEST_URI);
        gBrowser.selectedTab = tab;
        browser = gBrowser.selectedBrowser;
        window = XPCNativeWrapper.unwrap(browser.contentWindow);
        window.QueryInterface(Ci.nsIInterfaceRequestor)
          .getInterface(Ci.nsIDOMWindowUtils).garbageCollect();

        // use the old windowID again to see if we have any stray cached messages
        messages = gConsoleStorage.getCachedEvents(windowID);

        executeSoon(function (){
          ok(messages.length == 0,
            "0 events found, tab close is clearing the cache");
        });

Not sure what I am doing wrong. In the inner-window-destroyed event I do this:

      let innerWindowID = aSubject.QueryInterface(Components.interfaces.nsISupportsPRUint64).data;
      this.removeStorageForWindow(innerWindowID);
Ok, moving the getConsoleStorage call into the executeSoon does the trick. Really don't like the executeSoon crutch.
Attached patch v 0.9.1 comments addressed (obsolete) — Splinter Review
split out private browsing test, added check for inner-window-destroyed
Attachment #532050 - Attachment is obsolete: true
Attachment #535173 - Flags: review?(gavin.sharp)
(In reply to comment #57)
>         // need to get a new handle to a window in order to call
> garbageCollect()

You want to use the chrome window for this (just |window|).
Attachment #535173 - Attachment is obsolete: true
Attachment #535176 - Flags: review?(gavin.sharp)
Attachment #535173 - Flags: review?(gavin.sharp)
Comment on attachment 535176 [details] [diff] [review]
v 0.10 changed window used by garbageCollect()

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

>+function log(aMsg)

Don't add this (in either test). Use info() if you really need debugging output in the tests.

>+XPCOMUtils.defineLazyGetter(this, "gConsoleStorage", function () {
>+  let obj = {};
>+  Cu.import("resource://gre/modules/ConsoleStorageService.jsm", obj);
>+  return obj.ConsoleStorageService;
>+});

Just Cu.import(ConsoleStorageService.jsm) directly (both tests).

>+  observe: function CO_observe(aSubject, aTopic, aData)

>+        executeSoon(function (){

This executeSoon shouldn't be here - if it's really needed, then the test is susceptible to intermittent failures.

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

>+  observe: function CO_observe(aSubject, aTopic, aData)

>+        executeSoon(function() {
>+          try {
>+            if (pb.privateBrowsingEnabled) {
>+              pb.privateBrowsingEnabled = false;
>+              finish();
>+            }
>+          }

Why try/catch?

>+    else {
>+      finish();

Why is this here? A test generally shouldn't have multiple calls to finish().
Comment on attachment 535176 [details] [diff] [review]
v 0.10 changed window used by garbageCollect()

>diff --git a/dom/base/ConsoleAPI.js b/dom/base/ConsoleAPI.js

>+XPCOMUtils.defineLazyGetter(this, "gConsoleStorage", function () {

Just import ConsoleStorageService directly.

>diff --git a/dom/base/ConsoleStorageService.jsm b/dom/base/ConsoleStorageService.jsm

>+var ConsoleStorageService = {

>+  consoleStorage: {},

Just realized that this is actually exposed, so naming it _consoleStorage is probably best.

Sorry to flip-flop on the method names, but I think an API that looks like this would probably be best:

getEvents(id)
clearEvents(optional ID) (clears all events if no ID is passed)

recordEvent(id, event)

r- for the test issues, but this is very close.
Attachment #535176 - Flags: review?(gavin.sharp) → review-
(In reply to comment #62)
> Comment on attachment 535176 [details] [diff] [review] [review]
> Why try/catch?
> 
> >+    else {
> >+      finish();

Because this happens each time i try to shutdown pb mode:

TEST-PASS | chrome://mochitests/content/browser/dom/tests/browser/browser_ConsoleStoragePBTest.js | No logging entries found in PB mode
TEST-INFO | chrome://mochitests/content/browser/dom/tests/browser/browser_ConsoleStoragePBTest.js | [Exception... "'Failure' when calling method: [nsIPrivateBrowsingService::privateBrowsingEnabled]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://mochitests/content/browser/dom/tests/browser/browser_ConsoleStoragePBTest.js :: CO_observe :: line 48"  data: no]

It is very vague
Ok, it is throwing because PB mode is never entered into in the first place. All that happens is this: http://img717.imageshack.us/img717/103/selection008j.png

I thought PB mode would be automatically entered when you set pb.privateBrowsingMode = true;
(In reply to comment #65)

> pb.privateBrowsingMode = true;

or - rather:     pb.privateBrowsingEnabled = true;
(In reply to comment #65)
> Ok, it is throwing because PB mode is never entered into in the first place.
> All that happens is this:
> http://img717.imageshack.us/img717/103/selection008j.png
> 
> I thought PB mode would be automatically entered when you set
> pb.privateBrowsingMode = true;

That is correct (provided that we're not already in PB mode, i.e., pb.privateBrowsingEnabled === false).  What exactly is failing for you?
latest madness: So when I am in PB mode, and I do 'pb.privateBrowsingEnabled = false;'

It is supposed to exit and notify observers with aData == exit - right? In my case it does not exit, it just throws an error which is swallowed by mochitest:

[Exception... "'Failure' when calling method: [nsIPrivateBrowsingService::privateBrowsingEnabled]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://mochitests/content/browser/dom/tests/browser/browser_ConsoleStoragePBTest.js :: CO_observe :: line 48"  data: no]

And the test times out, however all checks pass
Gavin and Ehsan:

In the test browser_ConsoleStorageServicePBTest.js I cannot get the test to finish() without calling finish() 2X. Its very weird. All checks pass, but cannot get things to quit cleanly and am unsure why.
Attachment #535176 - Attachment is obsolete: true
Attachment #538108 - Flags: review?(ehsan)
Attached file modified test (obsolete) —
Here's a modified version of browser_ConsoleStoragePBTest.js that seems to work fine.
Comment on attachment 538120 [details]
modified test

nice. I was hoping one of you guys could tell me the error in my test, but i'll take this and run with it!
Attached patch v 0.12 comments addressed (obsolete) — Splinter Review
stole your test and made the last tweaks, I hope.
Attachment #538108 - Attachment is obsolete: true
Attachment #538120 - Attachment is obsolete: true
Attachment #538108 - Flags: review?(ehsan)
Attachment #538138 - Flags: review?(gavin.sharp)
Sorry for my delay here.  In your previous version of the test, you never quit the private browsing mode.  Also, doing stuff with the window on the enter notification is not safe, because the transition has not been completed.

Also, you don't need to check for the existence of the private browsing service, since this is a browser-chrome test which only runs in Firefox, which should always have this service.  So, please remove the check for the PB service and just assume that it's always available.
Browser chrome tests don't only run on Firefox. They also run in SeaMonkey, and could potentially run in other applications in the future. So please leave the private browsing check in.
(In reply to comment #74)
> Browser chrome tests don't only run on Firefox. They also run in SeaMonkey, and
> could potentially run in other applications in the future. So please leave the
> private browsing check in.

If that's the case, there are many more browser chrome tests which should also change.  At least in those tests that I wrote myself, I've assumed the existence of the PB service.
(In reply to comment #75)
> (In reply to comment #74)
> > Browser chrome tests don't only run on Firefox. They also run in SeaMonkey, and
> > could potentially run in other applications in the future. So please leave the
> > private browsing check in.
> 
> If that's the case, there are many more browser chrome tests which should
> also change.  At least in those tests that I wrote myself, I've assumed the
> existence of the PB service.

Gavin's new version checks and quits if we get null, so that should be good. Thanks for looking at this.
Comment on attachment 538138 [details] [diff] [review]
v 0.12 comments addressed

>diff --git a/dom/base/ConsoleStorageService.jsm b/dom/base/ConsoleStorageService.jsm

One (hopefully the last) naming nit: let's just rename this to ConsoleAPIStorage (filename and exported symbol), to avoid confusion with nsIConsoleService, and avoid using "Service" since this isn't a service in the XPCOM sense.

>+var ConsoleStorageService = {

>+  getEvents: function CS_getEvents(aId)
>+  {
>+    if (!aId)
>+      return this._consoleStorage;

I'm not sure this functionality is useful (who would want to get all events for all windows?), so let's omit it until there is a use case.

>+  removeStorageForWindow: function CS_removeStorageForWindow(aId)

>+  clearEvents: function CS_clearEvents()

Let's merge these (as suggested in comment 63)

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

>+        executeSoon(function (){

This executeSoon needs to go (comment 62).

>+registerCleanupFunction(tearDown);

Browser chrome tests shouldn't run code outside of test() - put this inside.
Attachment #538138 - Flags: review?(gavin.sharp) → review-
Attached patch v 0.13 comments addressed (obsolete) — Splinter Review
Comments addressed
Attachment #538138 - Attachment is obsolete: true
Attachment #538428 - Flags: review?(gavin.sharp)
Comment on attachment 538428 [details] [diff] [review]
v 0.13 comments addressed

>diff --git a/dom/base/ConsoleAPIStorage.jsm b/dom/base/ConsoleAPIStorage.jsm

>+var ConsoleAPIStorage = {

>+  /**
>+   * Object that contains all storage arrays
>+   */
>+  _consoleStorage: {},

Let's put this object outside of ConsoleAPIStorage (just a global in the file) so that it isn't exposed.

>+  /**
>+   * Get a storage array by ID
>+   *
>+   * @param string aId [optional]
>+   * @returns array
>+   *          (with optional ID, an object containing all events without an ID)

I don't understand this parenthetical.

>+  recordEvent: function CS_recordEvent(aWindowID, aEvent)

>+    if (gPrivBrowsing && gPrivBrowsing.privateBrowsingEnabled) {
>+      return;
>+    }

I think we'll probably want to revisit this - we should be able to store stuff in memory while in private browsing mode, we just need to clear it when we transition out. Let's do that in a followup.

>+   * Clear storage data
>+   *
>+   * @param string aId [optional]

I'm not a fan of these verbose comment blocks in general (particularly when they're larger than the self-explanatory functions they describe), but if they're going to be here you might as well document this properly, i.e. "ID of the window whose events should be cleared".

I don't think the "@returns void" comments are useful, I would just remove them.

>+  clearEvents: function CS_clearEvents(aId)
>+  {
>+    if (aId) {

Let's make this (aId != null) - clearEvents(0) should probably not clear all events.

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

>+var ConsoleObserver = {

>+  observe: function CO_observe(aSubject, aTopic, aData)

>+      dump("observe: " + aTopic + "\n\n\n");

remove this

>+        // close the window - the storage cache should now be empty
>+        gBrowser.removeTab(tab, {animate: false});
>+
>+        window.QueryInterface(Ci.nsIInterfaceRequestor)
>+          .getInterface(Ci.nsIDOMWindowUtils).garbageCollect();
>+        executeSoon(function (){

OK, so I looked into this further - this executeSoon *is* needed, but is potentially not even sufficient given nsGlobalWindow::TryClearWindowScope's behavior (potentially yielding multiple times before actually clearing the scope and firing the notification). I guess we can leave it as is for now and just keep an eye out for intermittent oranges, in which case we'll have to revisit.

r=me with those addressed (!)
Attachment #538428 - Flags: review?(gavin.sharp) → review+
Attached patch v 0.14 Comments addressed (obsolete) — Splinter Review
Attachment #538428 - Attachment is obsolete: true
Attachment #539664 - Flags: review+
Whiteboard: [has new patch][softblocker][console-1] → [has new patch][softblocker][console-1][has review]
(In reply to comment #79)
> >+  /**
> >+   * Get a storage array by ID
> >+   *
> >+   * @param string aId [optional]
> >+   * @returns array
> >+   *          (with optional ID, an object containing all events without an ID)
> 
> I don't understand this parenthetical.
> 
It does not apply any longer, I removed it.

> >+  recordEvent: function CS_recordEvent(aWindowID, aEvent)
> 
> >+    if (gPrivBrowsing && gPrivBrowsing.privateBrowsingEnabled) {
> >+      return;
> >+    }
> 
> I think we'll probably want to revisit this - we should be able to store
> stuff in memory while in private browsing mode, we just need to clear it
> when we transition out. Let's do that in a followup.

I will file a followup bug

Thank you gavin!
Attached patch v 0.15 fix for iframes (obsolete) — Splinter Review
Updated the patch to fix issues with iframes. This is a minor change actually. We recordEvent() with the window.top inner ID instead of the window inner ID.

The window.console API messages need to be recorded into the window.top inner ID "bucket" so the Web Console code from bug 609890 doesn't have to call getEvents() for each iframe window inner ID. We can just call getEvents() with the window.top inner ID.

Please let me know if this is fine. Thank you!
Assignee: ddahl → mihai.sucan
Attachment #539664 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #544317 - Flags: review?(gavin.sharp)
Comment on attachment 544317 [details] [diff] [review]
v 0.15 fix for iframes

I don't think this consolidation should happen in the core code. I can see why it's annoying for bug 609890's use case to have things stored for each individual window separately, but I think that's an edge case that we shouldn't complicate the basic ConsoleStorage/ConsoleAPI code for.

For bug 609890, we should either just loop through iframes and getEvents() for all of them, or just ignore subframes and only retrieve cached events for the top level window.
Attachment #544317 - Flags: review?(gavin.sharp) → review-
(In reply to comment #83)
> Comment on attachment 544317 [details] [diff] [review] [review]
> v 0.15 fix for iframes
> 
> I don't think this consolidation should happen in the core code. I can see
> why it's annoying for bug 609890's use case to have things stored for each
> individual window separately, but I think that's an edge case that we
> shouldn't complicate the basic ConsoleStorage/ConsoleAPI code for.
> 
> For bug 609890, we should either just loop through iframes and getEvents()
> for all of them, or just ignore subframes and only retrieve cached events
> for the top level window.

I would say it's an unneeded complication to work around this issue in bug 609890 when we can do it here.

For example, we'll have problems with sorting the messages if we do getEvents() for each iframe, and so on. I believe that doing getEvents() for each iframe is like opening a can of worms.

Hence if you do not want it here I am just going to show the cached events from the top level window - in bug 609890. Is this fine? Shall I revert the change here?

Thanks for your quick review! :)
Tested in Chrome's Web Inspector: they do cache and display errors and console API messages coming from iframes.

In complex web apps I believe we'll want this to work. It's not just a corner case.
Whiteboard: [has new patch][softblocker][console-1][has review] → [has new patch][console-1][has review]
(In reply to comment #84)
> For example, we'll have problems with sorting the messages if we do
> getEvents() for each iframe, and so on. I believe that doing getEvents() for
> each iframe is like opening a can of worms.

You could add a timestamp to the message object and use that to sort them. Granted that makes the process of displaying cached messages more processing-intensive, but I don't think that is necessarily a deal breaker for that approach.

The problem with storing windows using the topInnerID as the key is that evicting the cache becomes more complicated - the inner-window-destroyed code in that patch is broken because it only attempts to remove the storage for an inner window at the top level, when that inner window's events may actually be stored on under its parent window's ID (in which case you'd need to iterate over events from that topInnerID and only remove those with the given innerID). Doing that work (get topmost inner ID, iterate over cache) every time any inner window is destroyed doesn't sound great.
We have somewhat conflicting goals:
- the retrieval task (on Web Console open) wants an ordered list of messages by topInnerID
- the cache eviction task (on inner window destruction) wants an easy way to clear out all messages for a given innerID

Of those two tasks, the latter should occur much more frequently in common practice (most people won't ever open the Web Console), so I think it makes sense to try to optimize for that case, even at the expense of the former.
(In reply to comment #87)
> We have somewhat conflicting goals:
> - the retrieval task (on Web Console open) wants an ordered list of messages
> by topInnerID
> - the cache eviction task (on inner window destruction) wants an easy way to
> clear out all messages for a given innerID
> 
> Of those two tasks, the latter should occur much more frequently in common
> practice (most people won't ever open the Web Console), so I think it makes
> sense to try to optimize for that case, even at the expense of the former.

Good points. Thanks for your explanation!
Attached patch v 0.14b revert (obsolete) — Splinter Review
Reverted the use of window.top inner ID to record Console API changes.

Not asking for review again since there's only a change that adds the jsdoc comment to the notifyObservers() method.

Thank you for your review Gavin!
Attachment #544317 - Attachment is obsolete: true
It would be good to get someone like Boris or Sicking to SR this, I think, given that it's being added to dom/ (where I'm not a peer), and because it has the potential to introduce a perf/memory penalty for core code.
Comment on attachment 544849 [details] [diff] [review]
v 0.14b revert

Asking for superreview as suggested in comment 90.
Attachment #544849 - Flags: superreview?(bzbarsky)
If you want me to sr this, please write a decent description of exactly what this thing is doing _somewhere_.  Code comments ideal, but checkin comment is an OK fallback if needed.  In any case, the checkin comment needs to be a lot more useful than what's there now.
Attachment #544849 - Flags: superreview?(bzbarsky) → superreview-
Summary: Implement ConsoleStorageService → Implement ConsoleAPIStorage - add caching for the window.console API
Whiteboard: [has new patch][console-1][has review] → [console-1]
Attached patch v 0.15 improved comments (obsolete) — Splinter Review
Boris: I have updated the bug title and the commit message to better reflect what the patch does.

I have also added a comment to the new ConsoleAPIStorage.jsm, and improved the comments in general, to be clearer. Copying the ConsoleAPIStorage description:

-----

The ConsoleAPIStorage is meant to cache window.console API calls for later
reuse by other components when needed. For example, the Web Console code can
display the cached messages when it opens for the active tab.

ConsoleAPI messages are stored as they come from the ConsoleAPI code, with
all their properties. They are kept around until the window object that
created the messages is destroyed. Messages are indexed by the inner window
ID.

Usage:
   Cu.import("resource://gre/modules/ConsoleAPIStorage.jsm");

   // Get the cached events array for the window you want (use the inner
   // window ID).
   let events = ConsoleAPIStorage.getEvents(innerWindowID);
   events.forEach(function(event) { ... });

   // Clear the events for the given inner window ID.
   ConsoleAPIStorage.clearEvents(innerWindowID);

-----

I hope the changes help. Please let me know if any changes are needed. Thank you!
Attachment #544849 - Attachment is obsolete: true
Attachment #545243 - Flags: superreview?(bzbarsky)
Boris, is that description enough to get started on an sr?
Yes, I've been working on it...
Comment on attachment 545243 [details] [diff] [review]
v 0.15 improved comments

>They are kept around until the window object that
>+ * created the messages is destroyed.

This should probably specify that the messages are kept until the associated inner window is destroyed.

sr=me with that nit.  Sorry for the lag here, and I really appreciate the comments!
Attachment #545243 - Flags: superreview?(bzbarsky) → superreview+
Attached patch v 0.16 (obsolete) — Splinter Review
Updated the comment as suggested.

Thank you for your sr+!
Attachment #545243 - Attachment is obsolete: true
Attached patch v 0.17 rebase (obsolete) — Splinter Review
Rebased the patch on top of fxteam.
Attachment #547930 - Attachment is obsolete: true
Whiteboard: [console-1] → [console-1][land-in-fx-team]
another rebase
Attachment #550660 - Attachment is obsolete: true
Comment on attachment 555700 [details] [diff] [review]
[checked-in] v 0.18 rebase

Landed:

http://hg.mozilla.org/integration/fx-team/rev/bb48d11f9c08
Attachment #555700 - Attachment description: v 0.18 rebase → [in-fx-team] v 0.18 rebase
Whiteboard: [console-1][land-in-fx-team] → [console-1][fixed-in-fx-team]
Comment on attachment 555700 [details] [diff] [review]
[checked-in] v 0.18 rebase

http://hg.mozilla.org/mozilla-central/rev/bb48d11f9c08
Attachment #555700 - Attachment description: [in-fx-team] v 0.18 rebase → [checked-in] v 0.18 rebase
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Depends on: 728538
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: