868969 - Port changes from | Bug 649216 - Remove unnecessary delay when clicking tab close buttons sequentially | to comm-central

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: General, defect)

Description

[Phoenix]

Patch following

Comment 1

[Phoenix]

Attachment: Simple copy patch

Comment 2

[[e-mail]]

Comment on attachment 745831 [details] [diff] [review]
Simple copy patch

>+              var target = event.originalTarget;
>+              if (target.className == 'tab-close-button')
>+                target._ignoredClick = true;
This bit appears to depend on the patch for bug 409215.

As for the rest of the patch, I don't understand how or even whether the existing code works, but I think I understand how this patch improves it, so I'll give you an f+ anyway.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 745831 [details] [diff] [review]
Simple copy patch

Review of attachment 745831 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not currently in a position to test this, but I assume you have. The code makes sense, and by inspection, I think it's doing the right thing.

Comment 4

[Phoenix]

Attachment: Simple copy patch v2 r=mconley [check-in Comment 5]

Small change, replaced ' with " to match general style of files, carrying r+

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/051258fa7c9a

Comment 6

[Hartmut Figge]

I am not sure if the patch is responsible for a bug i just discovered, because a backout is not possible. In MailNews of SM, closing a tab fails with

Error: SyntaxError: syntax error
Source File: chrome://messenger/content/tabmail.xml
Line: 1548, Column: 12
Source Code:
            else
             ?

and the regression window is

Last good: 2013-05-19 16:13:00 PDT
First bad: 2013-05-20 16:09:00 PDT

Comment 7

[rsx11m]

> http://hg.mozilla.org/comm-central/file/051258fa7c9a/suite/mailnews/tabmail.xml
>   1538                 if (!clickedOnce) {
>   1539                 {
>   1540                   clickedOnce = true;
>   1541                   return;
>   1542                 }

Looks like a duplicated '{' to me, my editor indicates a mismatch of parentheses levels there. The mail/ part seems to be fine at that location.

Phoenix, can you verify, then reopen and post a quick follow-up patch if that's the case? Should be trivial...

Comment 8

[Phoenix]

Attachment: Followup fix for typo [check-in Comment 11]

Yeah, silly copy-paste mistake :(

Comment 9

[rsx11m]

Comment on attachment 756199 [details] [diff] [review]
Followup fix for typo [check-in Comment 11]

As trivial as it may be, you'll still need a review for this patch...

Comment 10

[[e-mail]]

Comment on attachment 756199 [details] [diff] [review]
Followup fix for typo [check-in Comment 11]

Mnyromyr must like giving braces their own line or something...

Comment 11

[Philip Chee]

Pushed to comm-central CLOSED TREE
http://hg.mozilla.org/comm-central/rev/697267b0511e