Port duplicateTabIn introduced in bug 448546

RESOLVED FIXED in seamonkey2.1b1

Status

SeaMonkey
Tabbed Browser
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Misak Khachatryan, Assigned: Philip Chee)

Tracking

(Blocks: 1 bug)

Trunk
seamonkey2.1b1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
bug 448546 introduces duplicateTabIn method, which will be nice to have for API compatibility.
(Assignee)

Updated

7 years ago
Blocks: 467867
No longer depends on: 467867
(Assignee)

Comment 1

7 years ago
Created attachment 477954 [details] [diff] [review]
Patch v1.0 add Firefox compatible duplicateTabIn()

Purely for Firefox compatibility (otherwise totally unnecessary)
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #477954 - Flags: review?(neil)
Attachment #477954 - Flags: feedback?(misak.bugzilla)

Comment 2

7 years ago
Comment on attachment 477954 [details] [diff] [review]
Patch v1.0 add Firefox compatible duplicateTabIn()

>+  aTab = aTab ? aTab : getBrowser().selectedTab;
At a quick glance, you could use aTab || getBrowser().selectedTab instead.

>+  var delta = aHistoryIndex == null ? currentIndex :
>+                                      aHistoryIndex - currentIndex;
Surely if aHistoryIndex is null then delta should be zero?
(Assignee)

Comment 3

7 years ago
>>+  var delta = aHistoryIndex == null ? currentIndex :
>>+                                      aHistoryIndex - currentIndex;
> Surely if aHistoryIndex is null then delta should be zero?

Yeah that's what I assumed at first too, but I couldn't match Firefox observed behaviour that way. Unfortunately I have to be compatible with their brain damaged implementation, so this silly construct.
(Reporter)

Updated

7 years ago
Attachment #477954 - Flags: feedback?(misak.bugzilla) → feedback+
(Assignee)

Updated

7 years ago
Attachment #477954 - Attachment is obsolete: true
Attachment #477954 - Flags: review?(neil)
(Assignee)

Comment 4

7 years ago
Created attachment 478223 [details] [diff] [review]
Patch 1.1 fix issues.

>>+  aTab = aTab ? aTab : getBrowser().selectedTab;
> At a quick glance, you could use aTab || getBrowser().selectedTab instead.
Fixed.

>>+  var delta = aHistoryIndex == null ? currentIndex :
>>+                                      aHistoryIndex - currentIndex;
> Surely if aHistoryIndex is null then delta should be zero?
Sigh. Brain fade at 2am. Cut and pasted the wrong block into my active patch.
Fixed.
Attachment #478223 - Flags: superreview?(neil)
Attachment #478223 - Flags: review?(neil)

Comment 5

7 years ago
Comment on attachment 478223 [details] [diff] [review]
Patch 1.1 fix issues.

>+/* Firefox compatibility shim *
>+ * duplicateTabIn duplicates tab in a place specified by the parameter |where|.
[This should have been a doc comment of course. Silly Firefox.]

>+ *  "tabfocused"  same as "tab" but override any background preferences and
>+                  focus the new tab
Nit: missing * on this line.

>+  var currentIndex = aTab.linkedBrowser.sessionHistory.index;
>+  var delta = aHistoryIndex == null ? 0 :
>+                                      aHistoryIndex - currentIndex;
Nit: fits in 80 characters without wrapping.

[duplicateTabIn(null, where) would have been nice, but not required for r+]
Attachment #478223 - Flags: superreview?(neil)
Attachment #478223 - Flags: superreview+
Attachment #478223 - Flags: review?(neil)
Attachment #478223 - Flags: review+
(In reply to comment #3)
> >>+  var delta = aHistoryIndex == null ? currentIndex :
> >>+                                      aHistoryIndex - currentIndex;
> > Surely if aHistoryIndex is null then delta should be zero?
> 
> Yeah that's what I assumed at first too, but I couldn't match Firefox observed
> behaviour that way. Unfortunately I have to be compatible with their brain
> damaged implementation, so this silly construct.

But you do realize that you can file bugs on Firefox code, don't you?
(Assignee)

Comment 7

7 years ago
Created attachment 478281 [details] [diff] [review]
Patch v1.1a Patch as pushed. r=Neil sr=Neil

>>+ *  "tabfocused"  same as "tab" but override any background preferences and
>>+                  focus the new tab
> Nit: missing * on this line.
Fixed.

>>+  var currentIndex = aTab.linkedBrowser.sessionHistory.index;
>>+  var delta = aHistoryIndex == null ? 0 :
>>+                                      aHistoryIndex - currentIndex;
> Nit: fits in 80 characters without wrapping.
Fixed.

> [duplicateTabIn(null, where) would have been nice, but not required for r+]
Fixed using:
+  aTab = aTab || getBrowser().selectedTab;

> Dão Gottwald      2010-09-24 08:09:24 PDT
> 
> (In reply to comment #3)
>> behaviour that way. Unfortunately I have to be compatible with their brain
>> damaged implementation, so this silly construct.
Oops. Apologies for this rude comment.

> But you do realize that you can file bugs on Firefox code, don't you?
I think I've done that before, but not recently.
(Assignee)

Comment 8

7 years ago
Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/ae798bbc3965

Also reapplied the patch from Bug 593683 which I accidentally clobbered.
http://hg.mozilla.org/comm-central/rev/4d36594ea0e8
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b1
Version: unspecified → Trunk
(Assignee)

Updated

6 years ago
Blocks: 636110
You need to log in before you can comment on or make changes to this bug.