Closed
Bug 688558
Opened 13 years ago
Closed 13 years ago
Context Menu for Inspect Element can Create 2 Highlighters
Categories
(DevTools :: General, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 9
People
(Reporter: rcampbell, Assigned: rcampbell)
References
Details
(Whiteboard: [minotaur][has-patch])
Attachments
(1 file)
5.10 KB,
patch
|
msucan
:
review+
|
Details | Diff | Splinter Review |
During some integration testing, I found that one of my patches made it possible to create two Highlighter instances using the new Context Menu's Inspect Element item landed in bug 587134. I wrote a test and some simple guard code in openInspectorUI() that my patch from bug 650794 breaks.
Attachment #561816 -
Flags: review?(mihai.sucan)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [minotaur][has-patch][needs-review]
Comment 1•13 years ago
|
||
Comment on attachment 561816 [details] [diff] [review] [in-fx-team] re-entrant context menu test We seem to inconsistently use isTreePanelOpen, or null checks of treeLoaded or highlighter for "is the inspector open". It would be nice to have a canonical way to check that and use it consistently.
Assignee | ||
Comment 2•13 years ago
|
||
Gavin, I think checking for the existence of InspectorUI.highlighter is going to be pretty reliable. It gets initialized at the end of everything else and should only be present when the Inspector/Highlighter is actually up and running. That's what I'm looking for in my updated patch for bug 650794. I could create a getter in InspectorUI that verifies the existence of the highlighter and that it's inited properly. Prior to bug 650794, we also rely on the tree panel being properly initialized and that starts to get a little confusing.
Assignee | ||
Comment 3•13 years ago
|
||
I could morph this bug into "create a canonical getter in InspectorUI to determine if the Inspector is Up and Running". Want?
Comment 4•13 years ago
|
||
Comment on attachment 561816 [details] [diff] [review] [in-fx-team] re-entrant context menu test Review of attachment 561816 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks fine, but I agree with Gavin about isTreePanelOpen. In our megapatch we do have isPanelOpen (IIRC), which is the canonical way to check if the Inspector is open or not. So we can use that. It would be nice if this fix would be on top of the megapatch?
Attachment #561816 -
Flags: review?(mihai.sucan) → review+
Assignee | ||
Comment 5•13 years ago
|
||
no, isPanelOpen or isTreePanelOpen are only useful for checking states of... panels! We want to know if the InspectorUI itself is up. I can create a method to do that.
Assignee | ||
Comment 6•13 years ago
|
||
Comment on attachment 561816 [details] [diff] [review] [in-fx-team] re-entrant context menu test https://hg.mozilla.org/integration/fx-team/rev/7e53a3cf73b5
Attachment #561816 -
Attachment description: re-entrant context menu test → [in-fx-team] re-entrant context menu test
Assignee | ||
Updated•13 years ago
|
Whiteboard: [minotaur][has-patch][needs-review] → [minotaur][has-patch][fixed-in-fx-team]
Assignee | ||
Comment 7•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7e53a3cf73b5
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Comment 8•13 years ago
|
||
Backed out because of various new mochitest-browser-chrome leaks. Unfortunately, this landed with a bunch of other interweaved patches. I only felt comfortable ruling out f297a626dd13 and 7d5311c92e04.
Assignee: nobody → rcampbell
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 9•13 years ago
|
||
relanded: https://hg.mozilla.org/integration/fx-team/rev/4afe9d03a4d4
Status: REOPENED → NEW
Target Milestone: Firefox 9 → ---
Comment 10•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4afe9d03a4d4
Status: NEW → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Whiteboard: [minotaur][has-patch][fixed-in-fx-team] → [minotaur][has-patch]
Target Milestone: --- → Firefox 9
Comment 11•13 years ago
|
||
Verified Fixed using Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0a1) Gecko/20111020 Firefox/10.0a1.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•