Port bug 1408572 to TB: Remove handleCtrlPageUpDown attribute and associated code

RESOLVED FIXED in Thunderbird 58.0

Status

Thunderbird
Toolbars and Tabs
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

unspecified
Thunderbird 58.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

5.75 KB, patch
Jorg K [Almost not working on Thunderbird (some bustage-fix only) due to non-renewal of contract]
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a month ago
Bug 1408572 removed the handleCtrlPageUpDown property. It makes now no sense to let it in our tabmail.xml. Remove it like FX did.
(Assignee)

Comment 1

a month ago
Created attachment 8920874 [details] [diff] [review]
No-handleCtrlPageUpDown-property.patch

This is a port of https://hg.mozilla.org/mozilla-central/rev/8e49aaf48550#l1.12
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8920874 - Flags: review?(jorgk)
Please include IB and SM in your patch. Looks like this code needs to go
https://dxr.mozilla.org/comm-central/rev/afdac93f14107db31e1f9d82b966b1b06e46d777/im/content/tabbrowser.xml#1401-1408
and line 27 in the same file needs adjustment as well as twos file in SM.
(Assignee)

Comment 3

a month ago
Created attachment 8920878 [details] [diff] [review]
No-handleCtrlPageUpDown-property.patch

IB and SM added.

The development of IB stopped and I think we don't need no more look that it builds. TB will be the successor of IB, see http://blog.queze.net/post/2017/10/18/Thunderbird-is-the-next-version-of-Instantbird.
Attachment #8920874 - Attachment is obsolete: true
Attachment #8920874 - Flags: review?(jorgk)
Attachment #8920878 - Flags: review?(jorgk)
Comment on attachment 8920878 [details] [diff] [review]
No-handleCtrlPageUpDown-property.patch

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

You've got a syntax error there.

OK, well, if IB is no longer maintained, we should perhaps remove the code since I personally don't want to find deprecated features in the code base.

::: im/content/tabbrowser.xml
@@ +1397,5 @@
>  #else
>            if (('ctrlKey' in aEvent && aEvent.ctrlKey) &&
>                !('shiftKey' in aEvent && aEvent.shiftKey) &&
>                !('metaKey' in aEvent && aEvent.metaKey)) {
> +            if (aEvent.keyCode == KeyEvent.DOM_VK_F4 {

Closing parentheses missing here. Also, as handleCtrlPageUpDown got removed, the original condition will never be true any more so the entire block should be removed?

What is that doing anyway? Removing a tab when the user presses Ctrl+F4? Maybe that's still useful and you are right only removing handleCtrlPageUpDown from the condition.
(Assignee)

Comment 5

a month ago
Created attachment 8920900 [details] [diff] [review]
22404.patch

(In reply to Jorg K (GMT+2) from comment #4)
> Comment on attachment 8920878 [details] [diff] [review]
> No-handleCtrlPageUpDown-property.patch
> 
> Review of attachment 8920878 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You've got a syntax error there.
> 
> OK, well, if IB is no longer maintained, we should perhaps remove the code
> since I personally don't want to find deprecated features in the code base.

They move now some code to TB. 

> ::: im/content/tabbrowser.xml
> @@ +1397,5 @@
> >  #else
> >            if (('ctrlKey' in aEvent && aEvent.ctrlKey) &&
> >                !('shiftKey' in aEvent && aEvent.shiftKey) &&
> >                !('metaKey' in aEvent && aEvent.metaKey)) {
> > +            if (aEvent.keyCode == KeyEvent.DOM_VK_F4 {
> 
> Closing parentheses missing here. Also, as handleCtrlPageUpDown got removed,
> the original condition will never be true any more so the entire block
> should be removed?

Fixed.

> What is that doing anyway? Removing a tab when the user presses Ctrl+F4?
> Maybe that's still useful and you are right only removing
> handleCtrlPageUpDown from the condition.

I don't know, you asked me to port it also for IB. It's hard to fix something we can't check.
Attachment #8920878 - Attachment is obsolete: true
Attachment #8920878 - Flags: review?(jorgk)
Attachment #8920900 - Flags: review?(jorgk)
Comment on attachment 8920900 [details] [diff] [review]
22404.patch

Thanks. What happened to the patch name? Is this the 22404th patch you're submitting ;-)

(In reply to Richard Marti (:Paenglab) from comment #5)
> I don't know, you asked me to port it also for IB. It's hard to fix
> something we can't check.
Agreed. But if we don't remove use of dead/deprecated M-C features, we will forever have them show up in our searches. That's why I filed bug 1410739.
Attachment #8920900 - Flags: review?(jorgk) → review+
(Assignee)

Comment 7

a month ago
No, it's the 22404th changeset in c-c. Actually there is one less as I have a other patch in front of this one. TortoiseHG proposed this name and it didn't got the correct name.
Keywords: checkin-needed

Comment 8

a month ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a400770aa527
Port bug 1408572 to C-C: Remove handleCtrlPageUpDown attribute and associated code. r=jorgk
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 58.0
You need to log in before you can comment on or make changes to this bug.