Last Comment Bug 700044 - Duplicate rule views after tab switching
: Duplicate rule views after tab switching
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: 10 Branch
: x86 Mac OS X
: P1 normal (vote)
: Firefox 10
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-05 10:33 PDT by Dave Camp (:dcamp)
Modified: 2011-11-07 14:58 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix, no test (700 bytes, patch)
2011-11-06 20:01 PST, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
test (6.77 KB, patch)
2011-11-07 09:24 PST, Rob Campbell [:rc] (:robcee)
dcamp: review+
Details | Diff | Splinter Review
merged patch (7.45 KB, patch)
2011-11-07 09:51 PST, Dave Camp (:dcamp)
mihai.sucan: review+
Details | Diff | Splinter Review

Description Dave Camp (:dcamp) 2011-11-05 10:33:38 PDT
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.
Comment 1 Dave Camp (:dcamp) 2011-11-06 19:56:47 PST
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.
Comment 2 Dave Camp (:dcamp) 2011-11-06 20:01:05 PST
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.
Comment 3 Mihai Sucan [:msucan] 2011-11-07 03:20:10 PST
(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.
Comment 4 Dave Camp (:dcamp) 2011-11-07 06:54:36 PST
(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 Rob Campbell [:rc] (:robcee) 2011-11-07 07:22:48 PST
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.
Comment 6 Rob Campbell [:rc] (:robcee) 2011-11-07 09:24:17 PST
Created attachment 572499 [details] [diff] [review]
test

test fails without fix, passes with. I'm calling this a unittest.
Comment 7 Dave Camp (:dcamp) 2011-11-07 09:51:03 PST
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.
Comment 8 Mihai Sucan [:msucan] 2011-11-07 10:21:30 PST
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).
Comment 10 Rob Campbell [:rc] (:robcee) 2011-11-07 14:58:25 PST
https://hg.mozilla.org/mozilla-central/rev/f57c833bb23e

Note You need to log in before you can comment on or make changes to this bug.