Last Comment Bug 740662 - Replace InspectorStore dictionaries with Inspector instances
: Replace InspectorStore dictionaries with Inspector instances
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: 13 Branch
: All All
: -- normal (vote)
: Firefox 14
Assigned To: Dave Camp (:dcamp)
:
: Patrick Brosset <:pbro>
Mentors:
Depends on: 707809
Blocks: 746010
  Show dependency treegraph
 
Reported: 2012-03-29 16:49 PDT by Dave Camp (:dcamp)
Modified: 2012-04-20 02:00 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (23.76 KB, patch)
2012-04-04 16:19 PDT, Dave Camp (:dcamp)
rcampbell: review+
Details | Diff | Splinter Review
v2 (23.68 KB, patch)
2012-04-16 16:00 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review

Description Dave Camp (:dcamp) 2012-03-29 16:49:54 PDT
Bug 707809 exposes a new Inspector object that's supposed to have the same lifetime as an InspectorStore.  Inspector should replace InspectorStore.
Comment 1 Dave Camp (:dcamp) 2012-04-04 16:19:53 PDT
Created attachment 612391 [details] [diff] [review]
v1
Comment 2 Rob Campbell [:rc] (:robcee) 2012-04-05 09:40:09 PDT
Comment on attachment 612391 [details] [diff] [review]
v1

   toggleHTMLPanel: function TP_toggle()
   {
     if (this.treePanel.isOpen()) {
       this.treePanel.close();
       Services.prefs.setBoolPref("devtools.inspector.htmlPanelOpen", false);
-      this.store.setValue(this.winID, "htmlPanelOpen", false);
+      this.currentInspector._htmlPanelOpen = false;
     } else {
       this.treePanel.open();
       Services.prefs.setBoolPref("devtools.inspector.htmlPanelOpen", true);
-      this.store.setValue(this.winID, "htmlPanelOpen", true);
+      this.currentInspector._htmlPanelOpen = true;
     }
   },

this method still bugs me.

I think we could do something like:

toggleHTMLPanel() {
  let wasClosed = !this.treePanel.isOpen();

  if (wasClosed)
    this.treePanel.open();
  else
    this.treePanel.close();

  Services.prefs.setBoolPref("devtools.inspector.htmlPanelOpen", wasClosed);
  this.store.setValue(this.winID, "htmlPanelOpen", wasClosed);
  this.currentInspector._htmlPanelOpen = wasClosed;
}

I'm not sure this is worth the change or if this is more or less confusing than the duplicated methods though. Dealer's choice.

-    this._currentInspector = new Inspector(this);
+    this.initializeStore();

I thought we were getting rid of InspectorStore...

I see what you did there. Repurposing it to store the inspector instances. Looks good.

does browser_inspector_tab_switch.js just work with this new implementation? If so, that's pretty sweet.

Should probably send it through try before landing.
Comment 3 Dave Camp (:dcamp) 2012-04-05 09:48:49 PDT
(In reply to Rob Campbell [:rc] (:robcee) from comment #2)

> this method still bugs me.
> 
> I think we could do something like:
> I'm not sure this is worth the change or if this is more or less confusing
> than the duplicated methods though. Dealer's choice.
> 

Yeah, that makes sense.  I'll do it.

> -    this._currentInspector = new Inspector(this);
> +    this.initializeStore();
> 
> I thought we were getting rid of InspectorStore...
> 
> I see what you did there. Repurposing it to store the inspector instances.
> Looks good.
> 
> does browser_inspector_tab_switch.js just work with this new implementation?
> If so, that's pretty sweet.

On my machine...

> 
> Should probably send it through try before landing.

Indeed.
Comment 4 Dave Camp (:dcamp) 2012-04-16 16:00:09 PDT
Created attachment 615517 [details] [diff] [review]
v2

Unbitrot (jimb and I spent a little while trying to find a reverse of 'rot', but this is the best we could find)
Comment 5 Dave Camp (:dcamp) 2012-04-19 10:23:30 PDT
https://tbpl.mozilla.org/?tree=Try&rev=9bded5c0b43a
Comment 7 Tim Taubert [:ttaubert] 2012-04-20 02:00:08 PDT
https://hg.mozilla.org/mozilla-central/rev/db5627efe52f

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