Closed Bug 868969 Opened 7 years ago Closed 7 years ago

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

Categories

(SeaMonkey :: MailNews: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.21

People

(Reporter: pppx, Assigned: pppx)

References

Details

Attachments

(2 files, 1 obsolete file)

Patch following
Attached patch Simple copy patch (obsolete) — Splinter Review
Attachment #745831 - Flags: review?(neil)
Attachment #745831 - Flags: review?(mconley)
Assignee: nobody → pppx
Status: NEW → ASSIGNED
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.
Attachment #745831 - Flags: review?(neil) → feedback+
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.
Attachment #745831 - Flags: review?(mconley) → review+
Small change, replaced ' with " to match general style of files, carrying r+
Attachment #751499 - Flags: review+
Keywords: checkin-needed
Attachment #745831 - Attachment is obsolete: true
https://hg.mozilla.org/comm-central/rev/051258fa7c9a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.21
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
> 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...
Flags: needinfo?(pppx)
Yeah, silly copy-paste mistake :(
Flags: needinfo?(pppx)
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Keywords: checkin-needed
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...
Attachment #756199 - Flags: review?(neil)
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...
Attachment #756199 - Flags: review?(neil) → review+
Keywords: checkin-needed
Pushed to comm-central CLOSED TREE
http://hg.mozilla.org/comm-central/rev/697267b0511e
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #751499 - Attachment description: Simple copy patch v2 → Simple copy patch v2 r=mconley [check-in Comment 5]
Attachment #756199 - Attachment description: Followup fix → Followup fix for typo [check-in Comment 11]
Depends on: 882901
You need to log in before you can comment on or make changes to this bug.