566083 - Inspect disappears when switching tabs

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[(no longer active)]

The inspect panel seems to be associated with tabs, since switching tabs will cancel it, but switching back to the same tab doesn't cause it to reappear.  I'm not sure if this is as designed, but it sure seems strange.

Comment 1

[Rob Campbell [:rc] (:robcee)]

this is as-designed for now. Changing tabs loses inspection state. We can use this bug to track this feature.

Comment 2

[Mihai Sucan [:msucan]]

Attachment: patch + test code

This patch fixes the issue as we discussed on IRC. This should apply cleanly on the default branch of mozilla-central.

Things to note:

- I updated the InspectorTreeView.selectedIndex getter to not throw when this.selection is undefined. It bumped into this issue writing the test code when I wanted to check which is the selectedNode - I expected it to return null, knowing that no node is selected, however it did throw.

I also updated the Inspector.selectedNode getter accordingly.

- You may want to use InspectorUI.store instead of the new global InspectorStore.

Looking forward to your feedback. Thanks!

Comment 3

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 461246 [details] [diff] [review]
patch + test code

I am considering using InspectorUI.store instead of the new global as you guessed. Given the existence of the other Inspector classes, though this seems acceptable. If we did want to move it to InspectorUI.store, I'd suggest putting the InspectorStore in a separate .jsm.

+  isStore: function IS_isStore(aID)
+  {
+    return (aID in this.store);
+  },

I'd rename this to exists: or hasID: rather than isStore. An InspectorStore is an InspectorStore.

Otherwise this all looks great.

Comment 4

[Mihai Sucan [:msucan]]

Attachment: updated patch + test code

Thanks Robert for your feedback+!

I have updated the patch as requested.

Comment 5

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 461290 [details] [diff] [review]
updated patch + test code

sorry, going to move this to gavin as I think he's more familiar with the code.

Comment 6

[Mihai Sucan [:msucan]]

Comment on attachment 461290 [details] [diff] [review]
updated patch + test code

Asking review from Dolske, as Gavin is leaving really soon on vacation.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

InspectorStore won't handle e.g. "toSource" as an ID correctly. I don't know whether that is a problem.

Comment 8

[Mihai Sucan [:msucan]]

Window IDs are numbers, if I am not mistaken. I believe that is not a problem, but Robert should confirm. If I have to make any changes, please let me know.

Thanks Gavin for looking through the code!

Comment 9

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #8)
> Window IDs are numbers, if I am not mistaken. I believe that is not a problem,
> but Robert should confirm. If I have to make any changes, please let me know.
> 
> Thanks Gavin for looking through the code!

Window IDs are numbers.

Comment 10

[Justin Dolske [:Dolske]]

Comment on attachment 461290 [details] [diff] [review]
updated patch + test code

This has bitrotted since the attachment was created [sorry :(], could you update the patch and reflag for review?

Nothing shocking from a quick skim, I'll try to review the updated patch ASAP.

Comment 11

[Mihai Sucan [:msucan]]

Attachment: rebased patch

No problem. I have rebased the patch against today's default branch from mozilla-central.

Looking forward to your review. Thank you!

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 466098 [details] [diff] [review]
rebased patch

Sigh, bitrotted again, sorry. But I reviewed it anyway without testing it, r+ with these changes.

>+++ b/browser/base/content/inspector.js
...
>-    this.inspectorBundle = Services.strings.createBundle("chrome://browser/locale/inspector.properties");
>+
>+    if (!this.inspectorBundle) {
>+      this.inspectorBundle = Services.strings.
>+        createBundle("chrome://browser/locale/inspector.properties");
>+    }

XPCOMUtils.defineLazyGetter, but looks like that's already been done in the tree.

>+    if (!InspectorStore.length) {
>+      gBrowser.tabContainer.addEventListener("TabSelect", this, false);
>+    }

General nit: blocks with just one statement don't need braces.

>+      InspectorStore.addStore(this.winID);
>+      InspectorStore.setValue(this.winID, "selectedNode", null);
>+      this.win.addEventListener("unload", this, false);

Sure you want "unload" here, and not "pagehide"?

>+        if (!InspectorStore.length) {
>+          gBrowser.tabContainer.removeEventListener("TabSelect", this, false);
>+        }

I almost wonder if InspectorStore should handle the addition and removal of its own event listeners, so that these checks don't have to be scattered around every place items are added/removed to the store.

>+  getWindowID: function IUI_getWindowID(aWindow)
>+  {
>+    if (!aWindow) {
>+      return null;
>+    }
>+
>+    let util = {};
>+
>+    try {
>+      util = aWindow.QueryInterface(Ci.nsIInterfaceRequestor).
>+        getInterface(Ci.nsIDOMWindowUtils);
>+    } catch (ex) { }

Why the try/catch? Seems like this shouldn't fail...

>+    return util.currentInnerWindowID;

Setting util to {} is a bit odd, instead just:

  ....getInterface(xxx).currentInnerWindowID?

If needed, the catch() could would then explicitly return null.


>+var InspectorStore = {
>+  store: {},
>+  length: 0,

Maybe add an isEmpty(), since that's what callers are really interested in?

>+  addStore: function IS_addStore(aID)
>+  {
>+    if (!(aID in this.store)) {
>+      this.store[aID] = {};
>+      this.length++;
>+      return true;
>+    } else {
>+      return false;
>+    }
>+  },

Avoid burying the return (here and elsewhere), better to do:

   if (foo) {
     bar;
     return true;
   }
   return false;

Not sure if the returns are really useful or not (they're not checked), this could just be a dumb function that clobbers any existing ID if called twice (or logs it as an error). But, meh, no harm either.

Comment 13

[Mihai Sucan [:msucan]]

Thank you very much for the r+ and for your time!

(In reply to comment #12)
> Comment on attachment 466098 [details] [diff] [review]
> rebased patch
> 
> Sigh, bitrotted again, sorry. But I reviewed it anyway without testing it, r+
> with these changes.

Uh oh, patches bitrot quite quickly. Thanks for reviewing the patch even in this situation.


> >+++ b/browser/base/content/inspector.js
> ...
> >-    this.inspectorBundle = Services.strings.createBundle("chrome://browser/locale/inspector.properties");
> >+
> >+    if (!this.inspectorBundle) {
> >+      this.inspectorBundle = Services.strings.
> >+        createBundle("chrome://browser/locale/inspector.properties");
> >+    }
> 
> XPCOMUtils.defineLazyGetter, but looks like that's already been done in the
> tree.

Yeah, that will be part of the new tree panel patches.


> >+    if (!InspectorStore.length) {
> >+      gBrowser.tabContainer.addEventListener("TabSelect", this, false);
> >+    }
> 
> General nit: blocks with just one statement don't need braces.

I did this according to the MDN coding style guidelines:

"Always brace controlled statements, even a single-line consequent of an if
else else. This is redundant typically, but it avoids dangling else bugs, so
it's safer at scale than fine-tuning."

See
https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Control_Structures

Do you want me to change the code? The Inspector scripts always use braces.


> >+      InspectorStore.addStore(this.winID);
> >+      InspectorStore.setValue(this.winID, "selectedNode", null);
> >+      this.win.addEventListener("unload", this, false);
> 
> Sure you want "unload" here, and not "pagehide"?

When I first heard of "pagehide" I thought it refers to the visual aspect of page hiding (eg. tab switching, window switching, etc). I just checked the docs and yeah, I'll switch to the pagehide event handler.


> >+        if (!InspectorStore.length) {
> >+          gBrowser.tabContainer.removeEventListener("TabSelect", this, false);
> >+        }
> 
> I almost wonder if InspectorStore should handle the addition and removal of its
> own event listeners, so that these checks don't have to be scattered around
> every place items are added/removed to the store.

I would suggest saying no to that. The intent with the InspectorStore is not to handle the multitude of event listeners of each page, tab, window, or whatever we may need now and/or in the future. The InspectorStore's purpose is only to store per-windowID settings/state information, which in the future will be more than what's in this patch.



> >+  getWindowID: function IUI_getWindowID(aWindow)
> >+  {
> >+    if (!aWindow) {
> >+      return null;
> >+    }
> >+
> >+    let util = {};
> >+
> >+    try {
> >+      util = aWindow.QueryInterface(Ci.nsIInterfaceRequestor).
> >+        getInterface(Ci.nsIDOMWindowUtils);
> >+    } catch (ex) { }
> 
> Why the try/catch? Seems like this shouldn't fail...

It can fail if aWindow is of incorrect type.


> >+    return util.currentInnerWindowID;
> 
> Setting util to {} is a bit odd, instead just:
> 
>   ....getInterface(xxx).currentInnerWindowID?
> 
> If needed, the catch() could would then explicitly return null.

I did util = {} for the return to not fail. I wanted to avoid try { return ... } catch (ex) { return null }. Should I change the code to do this?



> >+var InspectorStore = {
> >+  store: {},
> >+  length: 0,
> 
> Maybe add an isEmpty(), since that's what callers are really interested in?

Sure,  will do.

> >+  addStore: function IS_addStore(aID)
> >+  {
> >+    if (!(aID in this.store)) {
> >+      this.store[aID] = {};
> >+      this.length++;
> >+      return true;
> >+    } else {
> >+      return false;
> >+    }
> >+  },
> 
> Avoid burying the return (here and elsewhere), better to do:
> 
>    if (foo) {
>      bar;
>      return true;
>    }
>    return false;
> 
> Not sure if the returns are really useful or not (they're not checked), this
> could just be a dumb function that clobbers any existing ID if called twice (or
> logs it as an error). But, meh, no harm either.

Agreed, will not bury the returns in the upcoming rebased patch. The return values are used by the unit tests.

Thanks again!

Comment 14

[Mihai Sucan [:msucan]]

Attachment: rebased patch (2010-08-25)

Rebased patch for mozilla-central. Included changes requested by Dolske.

Comment 15

[David Dahl :ddahl]

http://hg.mozilla.org/mozilla-central/rev/5acfda4ffd2e