Closed Bug 700044 Opened 8 years ago Closed 8 years ago

Duplicate rule views after tab switching

Categories

(DevTools :: General, defect, P1)

10 Branch
x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 10

People

(Reporter: dcamp, Unassigned)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 file, 2 obsolete files)

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.
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.
Attached patch fix, no test (obsolete) — Splinter Review
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 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.
(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.
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
Attached patch test (obsolete) — Splinter Review
test fails without fix, passes with. I'm calling this a unittest.
Attachment #572499 - Flags: review?(dcamp)
Attachment #572499 - Flags: review?(dcamp) → review+
Attached patch merged patchSplinter Review
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 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+
https://hg.mozilla.org/mozilla-central/rev/f57c833bb23e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.