Duplicate rule views after tab switching

RESOLVED FIXED in Firefox 10

Status

()

Firefox
Developer Tools
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dcamp, Unassigned)

Tracking

10 Branch
Firefox 10
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 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

6 years ago
Created attachment 572380 [details] [diff] [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 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

6 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.
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
Created attachment 572499 [details] [diff] [review]
test

test fails without fix, passes with. I'm calling this a unittest.
Attachment #572499 - Flags: review?(dcamp)
(Reporter)

Updated

6 years ago
Attachment #572499 - Flags: review?(dcamp) → review+
(Reporter)

Comment 7

6 years ago
Created attachment 572507 [details] [diff] [review]
merged patch

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+
(Reporter)

Comment 9

6 years ago
https://hg.mozilla.org/integration/fx-team/rev/f57c833bb23e
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f57c833bb23e
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
You need to log in before you can comment on or make changes to this bug.