Closed
Bug 700044
Opened 13 years ago
Closed 13 years ago
Duplicate rule views after tab switching
Categories
(DevTools :: General, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 10
People
(Reporter: dcamp, Unassigned)
Details
(Whiteboard: [fixed-in-fx-team])
Attachments
(1 file, 2 obsolete files)
7.45 KB,
patch
|
msucan
:
review+
|
Details | Diff | Splinter Review |
1) Open the highlighter, show the rule view
2) Open a new tab
3) Switch back to the first tab
The rule view now has two sets of rules.
Reporter | ||
Comment 1•13 years ago
|
||
We call toolShow() once from restoreToolState, and once from its button's click handler. That's not ideal, but the tool should be robust against multiple show() calls.
Reporter | ||
Comment 2•13 years ago
|
||
I can't think of a good way to test this. Easy enough to trigger the bug (test can just call toolShow() twice in a row). But I can't think offhand of a good way to make sure we don't get two load events, without just waiting a long time.
Comment 3•13 years ago
|
||
(In reply to Dave Camp (:dcamp) from comment #2)
> Created attachment 572380 [details] [diff] [review] [diff] [details] [review]
> fix, no test
>
> I can't think of a good way to test this. Easy enough to trigger the bug
> (test can just call toolShow() twice in a row). But I can't think offhand
> of a good way to make sure we don't get two load events, without just
> waiting a long time.
In such cases I usually write a test that does what the STR says, at the end I check the expected result.
Reporter | ||
Comment 4•13 years ago
|
||
(In reply to Mihai Sucan [:msucan] from comment #3)
> In such cases I usually write a test that does what the STR says, at the end
> I check the expected result.
The problem is deciding what "the end" is - I can't think of a good way to wait for "enough time that a second load event would have fired if things were broken" that would be racy.
Comment 5•13 years ago
|
||
I've been using "traps" to detect multiple occurrences or things that shouldn't occur in my tests.
e.g.,
register one handler
when that one is received,
register another handler to your trap
* do something long-winded *
finish
if the trap is reached, dump a failure ok(false) and finish the test with your cleanup.
That seems to work well. If your "do something long-winded" test needs a bit more time, requestLongerTimeout(2) or 4 is usually enough.
Status: NEW → ASSIGNED
Comment 6•13 years ago
|
||
test fails without fix, passes with. I'm calling this a unittest.
Attachment #572499 -
Flags: review?(dcamp)
Reporter | ||
Updated•13 years ago
|
Attachment #572499 -
Flags: review?(dcamp) → review+
Reporter | ||
Comment 7•13 years ago
|
||
Merged the two patches and rebased on top of my landing queue. Set patch's user to rob, since clearly more novelty went into the test than the fix. Still, would like msucan to review because I don't want to r+'ing my own code changes.
Attachment #572380 -
Attachment is obsolete: true
Attachment #572499 -
Attachment is obsolete: true
Attachment #572507 -
Flags: review?(mihai.sucan)
Comment 8•13 years ago
|
||
Comment on attachment 572507 [details] [diff] [review]
merged patch
Review of attachment 572507 [details] [diff] [review]:
-----------------------------------------------------------------
Patch looks good. The usual nits apply (PD boilerplate, arguments.callee).
Attachment #572507 -
Flags: review?(mihai.sucan) → review+
Reporter | ||
Comment 9•13 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Comment 10•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•