Closed Bug 740662 Opened 13 years ago Closed 13 years ago

Replace InspectorStore dictionaries with Inspector instances

Categories

(DevTools :: Inspector, defect)

13 Branch
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 14

People

(Reporter: dcamp, Assigned: dcamp)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 707809 exposes a new Inspector object that's supposed to have the same lifetime as an InspectorStore. Inspector should replace InspectorStore.
Summary: Merge Inspector and InspectorStore objects → Replace InspectorStore dictionaries with Inspector instances
Attached patch v1 (obsolete) — Splinter Review
Attachment #612391 - Flags: review?(rcampbell)
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.
Attachment #612391 - Flags: review?(rcampbell) → review+
(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.
Attached patch v2Splinter Review
Unbitrot (jimb and I spent a little while trying to find a reverse of 'rot', but this is the best we could find)
Attachment #612391 - Attachment is obsolete: true
Blocks: 746010
Whiteboard: [fixed-in-fx-team]
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 14
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: